`checkPassword` function is incomplete

[summersab]

While performing testing of the following PRs, a bug was detected in the checkPassword function:
nextcloud/server#27929
#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 UPDATE oc_authtoken SET 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: #547 (comment)

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