`checkPassword` function is incomplete

[summersab]

While performing testing of the following PRs, a bug was detected in the checkPassword function: https://github.com/nextcloud/server/pull/27929 https://github.com/nextcloud/user_saml/pull/537

Steps to reproduce

1. Check out and apply the PRs noted above.
2. Configure the user_saml app to map a password/secret sent from the IdP (in this case, I'm using Keycloak).
3. Create a new user in the IdP and log into NC.
4. As an example, access the Files app. After five minutes, a call to getstoragestats.php will be triggered resulting in a session authentication check.
5. To manually force the behavior, reset the user's authtoken last_check field by executing UPDATEoc_authtokenSET last_check = 0; in the database. Then, run $.getJSON(OC.filePath('files','ajax','getstoragestats.php')) in the browser's console.

Expected behaviour

The user's session should stay authenticated without issue.

Actual behaviour

When any call is made that triggers a session authentication check, the user's page is automatically redirected back to the IdP. However, since the IdP session is still valid, the IdP then automatically re-authenticates with NC and logs the user back in without any action needed from the user.

Server configuration

Operating system: Debian 11

Web server: nginx

Database: MariaDB

PHP version: 8.0

Nextcloud version: (see Nextcloud admin page) 22.1.0

Where did you install Nextcloud from: Downloaded from https://download.nextcloud.com/server/releases/

List of activated apps:

encryption (Default encryption module)
user_saml (SSO & SAML authentication)

Nextcloud configuration:

{
    "system": {
        "version": "22.1.0.1",
        "installed": true,
        "dbtype": "mysql",
        "dbport": "",
        "dbtableprefix": "oc_",
        "mysql.utf8mb4": true,
        "maintenance": false,
    }
}

Client configuration

Browser: Chromium 90

Operating system: Debian 11

IdP: Keycloak v15

Logs

Nextcloud log (data/owncloud.log)

{"reqId":"GzoPbebScVMN0QFOnFQ0","level":2,"time":"2021-09-15T04:15:52+00:00","remoteAddr":"::1","user":"testuser","app":"core","method":"GET","url":"/index.php/css/core/cbe0-8fa8-server.css?v=d41d8cd98f00b204e9800998ecf8427e-","message":"Login failed: 'testuser' (Remote IP: '::1')","userAgent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36","version":"22.1.0.1"}
{"reqId":"GzoPbebScVMN0QFOnFQ0","level":3,"time":"2021-09-15T04:15:52+00:00","remoteAddr":"::1","user":"--","app":"PHP","method":"GET","url":"/index.php/css/core/cbe0-8fa8-server.css?v=d41d8cd98f00b204e9800998ecf8427e-","message":"session_start(): A session had already been started - ignoring at /var/www/nextcloud/lib/private/Session/Internal.php#206","userAgent":"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36","version":"22.1.0.1","exception":{"Exception":"Error","Message":"session_start(): A session had already been started - ignoring at /var/www/nextcloud/lib/private/Session/Internal.php#206","Code":0,"Trace":[{"function":"onAll","class":"OC\\Log\\ErrorHandler","type":"::"},{"function":"session_start"},{"file":"/var/www/nextcloud/lib/private/Session/Internal.php","line":206,"function":"call_user_func_array"},{"file":"/var/www/nextcloud/lib/private/Session/Internal.php","line":216,"function":"invoke","class":"OC\\Session\\Internal","type":"->"},{"file":"/var/www/nextcloud/lib/private/Session/Internal.php","line":106,"function":"startSession","class":"OC\\Session\\Internal","type":"->"},{"file":"/var/www/nextcloud/lib/private/Session/CryptoSessionData.php","line":149,"function":"clear","class":"OC\\Session\\Internal","type":"->"},{"file":"/var/www/nextcloud/lib/private/User/Session.php","line":933,"function":"clear","class":"OC\\Session\\CryptoSessionData","type":"->"},{"file":"/var/www/nextcloud/lib/private/User/Session.php","line":270,"function":"logout","class":"OC\\User\\Session","type":"->"},{"file":"/var/www/nextcloud/lib/private/User/Session.php","line":243,"function":"validateSession","class":"OC\\User\\Session","type":"->"},{"file":"/var/www/nextcloud/apps/user_saml/appinfo/app.php","line":108,"function":"getUser","class":"OC\\User\\Session","type":"->"},{"file":"/var/www/nextcloud/lib/private/legacy/OC_App.php","line":303,"args":["/var/www/nextcloud/apps/user_saml/appinfo/app.php"],"function":"require_once"},{"file":"/var/www/nextcloud/lib/private/legacy/OC_App.php","line":185,"function":"requireAppFile","class":"OC_App","type":"::"},{"file":"/var/www/nextcloud/lib/private/legacy/OC_App.php","line":139,"function":"loadApp","class":"OC_App","type":"::"},{"file":"/var/www/nextcloud/lib/base.php","line":979,"function":"loadApps","class":"OC_App","type":"::"},{"file":"/var/www/nextcloud/index.php","line":36,"function":"handleRequest","class":"OC","type":"::"}],"File":"/var/www/nextcloud/lib/private/Log/ErrorHandler.php","Line":99,"CustomMessage":"--"}}

Suggested solution

I have identified the source of the problem, but I'm not entirely sure if my solution is the best method. Before opening a PR, I wanted to have a discussion in an issue submission.

In OCA\User_SAML\UserBackend, replace the checkPassword function with the following (including the two additional private functions):

public function checkPassword($uid, $password) {
    /* @var $qb IQueryBuilder */
    $qb = $this->db->getQueryBuilder();
    $qb->select('token', 'private_key', 'password')
        // Previously used the 'user_saml_auth_token' table, but it is never used
        ->from('authtoken')
        ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
        ->setMaxResults(1000);
    $result = $qb->execute();
    $data = $result->fetchAll();
    $result->closeCursor();

    $secret = $this->config->getSystemValue('secret');
    $instanceid = $this->config->getSystemValue('instanceid');
    $token = \OC::$server->getRequest()->getCookie($instanceid);
    $hashedToken = hash('sha512', $token . $secret);

    // I'm not sure how extensive these checks need to be. Is it required to
    // decrypt all the way down to the stored private key and password? No
    // idea, but I went to that extent for completeness.
    foreach($data as $passwords) {
        if ($hashedToken == $passwords['token']) {
            $privateKey = $this->decrypt($passwords['private_key'], $token);

            if (!is_null($password)) {
                $decryptedPassword = $this->decryptPassword($passwords['password'], $privateKey);

                if ($decryptedPassword == $password) {
                    return $uid;
                }
            }
            else {
                return $uid;
            }
        }
    }

    return false;
}

/**
 * @throws InvalidTokenException
 * Adapted from OC\Authentication\Token\PublicKeyTokenProvider
 */
private function decrypt(string $cipherText, string $token): string {
    $secret = $this->config->getSystemValue('secret');
    $crypto = \OC::$server->getCrypto();
    $provider = \OC::$server->query('OC\Authentication\Token\PublicKeyTokenProvider');

    try {
        return $crypto->decrypt($cipherText, $token . $secret);
    } catch (\Exception $ex) {
        // Delete the invalid token
        $provider->invalidateToken($token);
        throw new \OC\Authentication\Exceptions\InvalidTokenException("Could not decrypt token password: " . $ex->getMessage(), 0, $ex);
    }
}

/**
 * From OC\Authentication\Token\PublicKeyTokenProvider
 */
private function decryptPassword(string $encryptedPassword, string $privateKey): string {
    $encryptedPassword = base64_decode($encryptedPassword);
    openssl_private_decrypt($encryptedPassword, $password, $privateKey, OPENSSL_PKCS1_OAEP_PADDING);

    return $password;
}

[MichaIng]

I edited code highlighting into the code blocks, before you wonder, I hope you don't mind 😉.

Could you explain what the issue was/is? Is checkPassword also called without passing a password and it is expected then that only the session token is verified?

And/or is it that password_verify does not work here? To me it looks highly undesirable to decrypt everything down manually to compare plain text passwords, while there is a native PHP function to verify the given function against the stored hash. Of course the stored hash then also needs to be created via password_hash, respectively follow the same format with one of the supported hash algorithms, no sure how storing the password with this backend is handled.

One suggestion, if this remains being required:

        if ($hashedToken == $passwords['token']) {
            if (!is_null($password)) {
                $privateKey = $this->decrypt($passwords['private_key'], $token);
                $decryptedPassword = $this->decryptPassword($passwords['password'], $privateKey);

• Decrypt the private key only when there is actually a password to decrypt.

[summersab]

All good points. I agree that it is highly undesirable to decrypt everything, but I did so simply to be thorough. From a security standpoint, I'm not sure what counts as "good enough" to ensure that someone is authenticated.

To respond to your questions and provide some further clarification:

• The checkPassword function is only ever called if a password is provided from the IdP, so having a password provided to the function is not the problem.
• password_verify doesn't work because a password hash is never created from the password sent from the IdP - there's nothing in the DB to use to compare.

Here is an alternative solution that I believe is better than what I previously proposed. On the initial SAML login, if a password is passed from the IdP, use the existing NC hash functions to create and store a password hash. This would require a few changes:

Add a password field to the oc_user_saml_users table to store the hashes.

ALTER TABLE `oc_user_saml_users` ADD COLUMN password VARCHAR(255);

Modify OCA\User_SAML\UserBackend::createUserIfNotExists to generate and store the hash on first login.

(The bottom half from $userSecret = $this->getUserSecret($uid, $attributes); downward is what has been changed).

public function createUserIfNotExists($uid, array $attributes = array()) {
    if(!$this->userExistsInDatabase($uid)) {
        $values = [
            'uid' => $uid,
        ];

        // Try to get the mapped home directory of the user
        try {
            $home = $this->getAttributeValue('saml-attribute-mapping-home_mapping', $attributes);
        } catch (\InvalidArgumentException $e) {
            $home = '';
        }

        if ($home !== '') {
            //if attribute's value is an absolute path take this, otherwise append it to data dir
            //check for / at the beginning or pattern c:\ resp. c:/
            if(   '/' !== $home[0]
               && !(3 < strlen($home) && ctype_alpha($home[0])
                   && $home[1] === ':' && ('\\' === $home[2] || '/' === $home[2]))
            ) {
                $home = $this->config->getSystemValue('datadirectory',
                        \OC::$SERVERROOT.'/data' ) . '/' . $home;
            }

            $values['home'] = $home;
        }

        $userSecret = $this->getUserSecret($uid, $attributes);

        if ($userSecret !== null) {
            $hasher = \OC::$server->getHasher();
            $hashedPassword = $hasher->hash($userSecret);
            $values['password'] = $hashedPassword;
        }

        /* @var $qb IQueryBuilder */
        $qb = $this->db->getQueryBuilder();
        $qb->insert('user_saml_users');
        foreach($values as $column => $value) {
            $qb->setValue($column, $qb->createNamedParameter($value));
        }
        $qb->execute();

        // If we use per-user encryption the keys must be initialized first
        if ($userSecret !== null) {
            // Emit a post login action to initialize the encryption module with the user secret provided by the idp.
            \OC_Hook::emit('OC_User', 'post_login', ['run' => true, 'uid' => $uid, 'password' => $userSecret, 'isTokenLogin' => false]);
        }
        $this->initializeHomeDir($uid);

    }
}

Update query in checkPassword function to select the correct fields and use the NC Hasher class

public function checkPassword($uid, $password) {
    /* @var $qb IQueryBuilder */
    $qb = $this->db->getQueryBuilder();
    $qb->select('password')
        ->from('user_saml_users')
        ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
        ->setMaxResults(1000);
    $result = $qb->execute();
    $data = $result->fetchAll();
    $result->closeCursor();

    foreach($data as $passwords) {
        if(\OC::$server->getHasher()->verify($password, $passwords['password'])) {
            return $uid;
        }
    }

    return false;
}

I have tested the code above, and it works. This solution requires minimal changes and is more in-line with the way the default database authentication backend handles session verification. Thoughts?

[MichaIng]

Yet another alternative is to encrypt the passed password for comparison, if that is possible with a public key and the result is consistent? But I lack insights about which algorithms are used here 😄.

I personally would prefer the second solution, even that it means an additional database column. But let's see what others think about it.

[summersab]

Good thoughts and feedback. Unfortunately, your suggestion won't work because you cannot compare ciphertexts from encryption - even when the password remains the same, the resulting ciphertext changes every time the algorithm is executed. Here's an example:

public function checkPassword($uid, $password) {
    /* @var $qb IQueryBuilder */
    $qb = $this->db->getQueryBuilder();
    $qb->select('password', 'public_key')
        ->from('authtoken')
        ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
        ->setMaxResults(1000);
    $result = $qb->execute();
    $data = $result->fetchAll();
    $result->closeCursor();

    // Every time this is executed, $encryptedPassword will be different.
    // You can only compare hashes, not ciphertexts from encryption algorithms.
    openssl_public_encrypt($password, $encryptedPassword, $el['public_key'], OPENSSL_PKCS1_OAEP_PADDING);
    $encryptedPassword = base64_encode($encryptedPassword);

    // Running this a second time will result in a completely different $encryptedPassword.
    openssl_public_encrypt($password, $encryptedPassword, $el['public_key'], OPENSSL_PKCS1_OAEP_PADDING);
    $encryptedPassword = base64_encode($encryptedPassword);

    // There is no way to recreate $encryptedPassword to match what is in the DB.
    foreach($data as $el) {
        if($el['password'] == $encryptedPassword) {
            return $uid;
        }
    }

    return false;
}

Instead, you have to compare a hash of the password, but currently, the user_saml app doesn't create and store a hash. The only thing I can do with the data currently available is compare the stored authtoken and the session cookie. That doesn't really check the password, though, and that's the whole point of this function. Here's an example of what that would look like:

public function checkPassword($uid, $password) {
    /* @var $qb IQueryBuilder */
    $qb = $this->db->getQueryBuilder();
    $qb->select('token')
        ->from('authtoken')
        ->where($qb->expr()->eq('uid', $qb->createNamedParameter($uid)))
        ->setMaxResults(1000);
    $result = $qb->execute();
    $data = $result->fetchAll();
    $result->closeCursor();

    $secret = $this->config->getSystemValue('secret');
    $instanceid = $this->config->getSystemValue('instanceid');
    $cookie = \OC::$server->getRequest()->getCookie($instanceid);

    foreach($data as $el) {
        if($el['token'] == hash('sha512', $cookie . $secret)) {
            return $uid;
        }
    }

    return false;
}

In conclusion:

• I agree that my original suggestion is a bad idea. It's unwise to decrypt the password from the DB fields even if it is all done in memory - that's just not secure.
• It's impossible to use the available data to recreate the ciphertext stored in the DB (third example).
• Comparing the authtoken and session cookie is insufficient and doesn't accomplish what the function is supposed to do (fourth example).
• My second idea more closely replicates the functionality of OC\User\Database::checkPassword. It is comparing a hash of the password that is stored in the oc_users table: https://github.com/nextcloud/server/blob/06ad953047fa00f93117a033384bec1c79dde14a/lib/private/User/Database.php#L331 To do this, we need to add a field to the oc_user_saml_users table to store the same information used with the default backend.

[MichaIng]

It's impossible to use the available data to recreate the ciphertext stored in the DB (third example).

Yes I though about this, thanks for clarifying. I'd also vote for storing and using the hash then. The method is used in other cases, uses the Nextcloud API designed for this purpose and makes use of config.php settings to tune hash algorithm parameters, so it feels like the right way. But I'm far away from an in-depth security guy, so others need to decide.

Added a few assignments to ping some hopefully fitting guys here 🙂.

[summersab]

Since there have been no comments, does anyone object to me submitting a PR to implement this feature?

[mndeveloper]

Hi,

I'm not sure if I understand this correct - may I'm wrong - then I'm sorry.

But I have a problem with this plug in with NC authtoken (table oc_authtoken). If I generate such token for for CalDAV or notices all this authtoken expiere after some short time. If I disable "user_saml" it works.

Could it be that this is the same problem?

Best regards.

[summersab]

@mndeveloper - this is a pretty advanced and custom integration we're discussing, so I rather doubt that you're experiencing the same issue. However, if you wanted to give it a whirl, apply the changes mentioned in this comment from above: https://github.com/nextcloud/user_saml/issues/547#issuecomment-920165994

Let me know if you have any questions, and I'll see if I can help!

[mndeveloper]

@summersab Thanks, I tried the patch but it did't work for my problem.

[immerda]

I am trying to follow what is happening here. If I understand you @summersab correctly, then checkPassword was never called without the SSO secret patch? Or in other words this was mostly dead code?

Basically my question is how is this currently working and is it even necessary? If this function is bypassed in the case where the idp does not set a user secret, why would it be necessary now (or conversely, wouldn't it be also necessary in the case without a secret)?

Also, in case it really is necessary, wouldn't it be enough the keep a hash of the secret in the session, rather than have it persisted in the DB?

[summersab]

Ah, @immerda - the man who started it all! :-) Sorry for the delay.

So, the basic principle is that NC needs to validate that the user's session is authenticated after a period of time (I believe it's 10 minutes after login). Right now, there is nothing in the user_saml backend to validate the user. Something HAS to be stored inside the DB in order to check if the user's session is valid, but there is nothing there. As a result:

1. NC does the "Refresh in 5, 4, 3, 2, 1" notification
2. The user is logged out
3. The user's browser is immediately redirected to the IdP
4. The IdP validates the user's session cookies and immediately redirects the browser back to NC

The user never has to actively do anything to re-authenticate, but every 10 minutes, the page refreshes as they are kicked out of NC. This is pretty inconvenient.

This patch creates a hash in the oc_user_saml_users table so that the user's session can be authenticated. It mimics the existing functionality of OC\User\Database::checkPassword (which is used for users created with the NC database as the authentication backend).

The best way to do this might be to make a call to the IdP in order to validate the user, but that would be platform-specific (Keycloak, OneLogin, etc). Perhaps direct integrations could be added down the road, but this would work for all systems right now.

[immerda]

Hey, thanks for the explanation.

What I don't understand is what about the case where there is no idp secret? I mean that is going to be the most common case, only few users will configure NC with this idp provided secrets. In the normal case the "password" is empty, right? So this pw check will always succeed and does not provide any additional "validation". That's why I wonder if it is the right approach.

[summersab]

Ah, I see. In this case, it would still create a hash and check the hash after a timeout (10 minutes or whatever), and even without a password, it would still work. Of course, I haven't expressly tested that use-case. I'll need to do that before creating a PR.

[immerda]

Right, it would work, but it is equivalent to return true in that case. Maybe that's ok. And maybe it would even in the case the password is non-empty be ok to just return true. But maybe it also hints at an issue with the SSO module, that it should maybe set the password to a random string, or I don't know. Basically I don't know enough about how it "should work", that's why I am asking stupid questions :)

also have you considered what happens if the secret changes?

[summersab]

Having the secret change is indeed something I have tested. Basically, the hash in oc_user_saml_users is updated at login, so it functions just like a password change with the built-in database user backend.

Feel free to keep asking stupid questions - it ensures that I cover all of the bases. :-)

[immerda]

I dug some more into this and indeed currently checkPassword is never called, because user_saml creates "passwordless tokens".

The current implementation of checkPassword is completetly bogus and can be changed to return false, without any change in functionality. The table user_saml_auth_token is always empty, there is no code to insert anything into it...

I agree with the general direction of basically copying the Database user backend, and store a hashed version of the secret somewhere. You could re-use the user_saml_auth_token table for that, or add a field to user_saml_users...

[summersab]

I dug some more into this and indeed currently checkPassword is never called, because user_saml creates "passwordless tokens".

Hmm . . . I'm not sure I agree. Based on my testing, it is called every 5 (or maybe 10?) minutes whenever NC does an authentication check. Without this fix in place, these two PRs don't quite work properly: https://github.com/nextcloud/user_saml/pull/537 https://github.com/nextcloud/server/pull/27929

I should probably submit a new PR to supersede https://github.com/nextcloud/server/pull/27929 and include this fix. Thoughts?

[immerda]

Without this fix in place, these two PRs don't quite work properly:

We are in agreement here. But without the PRs you mention, the function is never called.

I should probably submit a new PR to supersede nextcloud/server#27929 and include this fix. Thoughts?

I've been contemplating the same, but this PR is already superseeding my original attempt and I am afraid of adding more confusion.