[Bug]: a wrong password will trigger badPwdCount twice in ldap

[mightymop]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• Nextcloud Server is running on 64bit capable CPU, PHP and OS.
• I agree to follow Nextcloud's Code of Conduct.

Bug description

If i enter a wrong password the login will fail > as expected
But the "bad password counter" allways get raised +2 instead of +1

I am using the User_ldap app and have debugged this bug to this method:

server/lib/private/User/Manager.php

Lines 255 to 291 in 232866d

	public function checkPasswordNoLogging($loginName, $password) {
	$loginName = str_replace("\0", '', $loginName);
	$password = str_replace("\0", '', $password);
	$cachedBackend = $this->cache->get($loginName);
	if ($cachedBackend !== null && isset($this->backends[$cachedBackend])) {
	$backends = [$this->backends[$cachedBackend]];
	} else {
	$backends = $this->backends;
	}
	foreach ($backends as $backend) {
	if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) {
	/** @var ICheckPasswordBackend $backend */
	$uid = $backend->checkPassword($loginName, $password);
	if ($uid !== false) {
	return $this->getUserObject($uid, $backend);
	}
	}
	}
	// since http basic auth doesn't provide a standard way of handling non ascii password we allow password to be urlencoded
	// we only do this decoding after using the plain password fails to maintain compatibility with any password that happens
	// to contain urlencoded patterns by "accident".
	$password = urldecode($password);
	foreach ($backends as $backend) {
	if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) {
	/** @var ICheckPasswordBackend|UserInterface $backend */
	$uid = $backend->checkPassword($loginName, $password);
	if ($uid !== false) {
	return $this->getUserObject($uid, $backend);
	}
	}
	}
	return false;
	}

Here this method first tries to login with the given credentials.
After fail a second try is happen to prove for non ascii passwords...

I think this second one have to be configurable, because if someone has a fail to ban protection in ldap @ 3 wrong passwords the user get locked out/banned after only 2 wrong password tries

Steps to reproduce

1.install nextcloud (version does not matter, cause version 18-24 are affected)
2. install / setup user_ldap app
3. login with existing user but bad password
4. look into Active directory user / group management console and search for the user
5. look into the attribute editor und find badPwdCount raised to "2"

Expected behavior

decribed above

Installation method

No response

Operating system

Debian/Ubuntu

PHP engine version

PHP 7.4

Web server

Nginx

Database engine version

SQlite

Is this bug present after an update or on a fresh install?

fresh install and update

Are you using the Nextcloud Server Encryption module?

no

What user-backends are you using?

• Default user-backend (database)
• LDAP/ Active Directory
• SSO - SAML
• Other

Configuration report

{
    "system": {
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "192.168.1.3"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "sqlite3",
        "version": "25.0.0.6",
        "overwrite.cli.url": "http:\/\/192.168.1.3\/nextcloud",
        "installed": true,
        "ldapProviderFactory": "OCA\\User_LDAP\\LDAPProviderFactory"
    }
}

List of activated Apps

Enabled:
  - cloud_federation_api: 1.8.0
  - dav: 1.24.0
  - federatedfilesharing: 1.15.0
  - files: 1.20.0
  - files_trashbin: 1.15.0
  - files_versions: 1.18.0
  - lookup_server_connector: 1.13.0
  - oauth2: 1.13.0
  - provisioning_api: 1.15.0
  - settings: 1.7.0
  - systemtags: 1.15.0
  - twofactor_backupcodes: 1.14.0
  - updatenotification: 1.15.0
  - user_ldap: 1.15.0
  - user_status: 1.5.0
  - workflowengine: 2.7.0
Disabled:
  - admin_audit
  - bruteforcesettings
  - comments: 1.15.0
  - contactsinteraction: 1.6.0
  - dashboard: 7.5.0
  - encryption
  - federation: 1.15.0
  - files_external
  - files_sharing: 1.17.0
  - sharebymail: 1.15.0
  - testing
  - theming: 2.0.0
  - weather_status: 1.5.0

Nextcloud Signing status

Nextcloud Logs

not provided, not needed due to logical bug in source...
see in description above

Additional info

affected Nextcloud versions:

• 18
• 19
• 20
• 21
• 22
• 23
• 24
• latest

[mightymop]

The USERNAME:PASSWORD should also be sent via http header...

Authorization: Basic <BASE64-ENCODED-FORM-OF_USERNAME:PASSWORD>

if login fails on first attempt the header val should be base64 decoded and compared to the post val...
if they are the same, do not try to login again. Otherwhise a second try with the decoded data should be made...
Or simply only use header data for login...

[asomy23]

Thank you mightymop
we have strong policy if you enter password 4 times your account will be locked, so this issue made issue for us.

i remove this function which you mentioned and it's resolved our issue:
"
$password = urldecode($password);

foreach ($backends as $backend) { 
	if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) { 
		/** @var ICheckPasswordBackend|UserInterface $backend */ 
		$uid = $backend->checkPassword($loginName, $password); 
		if ($uid !== false) { 
			return $this->getUserObject($uid, $backend); 
		} 
	} 
} "

Thanks again & BR

[mightymop]

@asomy23 yes as i said it is a logical bug in the source code... which should either be configurable or fixed by a patch like my suggestion

[mightymop]

ping

[mightymop]

@solracsf Do you need more information to fix the bug?

[szaimen]

Hi, please update to 24.0.9 or better 25.0.3 and report back if it fixes the issue. Thank you!

My goal is to add a label like e.g. 25-feedback to this ticket of an up-to-date major Nextcloud version where the bug could be reproduced. However this is not going to work without your help. So thanks for all your effort!

If you don't manage to reproduce the issue in time and the issue gets closed but you can reproduce the issue afterwards, feel free to create a new bug report with up-to-date information by following this link: https://github.com/nextcloud/server/issues/new?assignees=&labels=bug%2C0.+Needs+triage&template=BUG_REPORT.yml&title=%5BBug%5D%3A+

[mightymop]

Hi, please update to 24.0.9 or better 25.0.3 and report back if it fixes the issue. Thank you!

My goal is to add a label like e.g. 25-feedback to this ticket of an up-to-date major Nextcloud version where the bug could be reproduced. However this is not going to work without your help. So thanks for all your effort!

If you don't manage to reproduce the issue in time and the issue gets closed but you can reproduce the issue afterwards, feel free to create a new bug report with up-to-date information by following this link: https://github.com/nextcloud/server/issues/new?assignees=&labels=bug%2C0.+Needs+triage&template=BUG_REPORT.yml&title=%5BBug%5D%3A+

Is this a default question for long time pending issues? Because the source around the bug did not change... so yes of course it is still reproduceable.

The entire following code block is causing the bug because the password will be checked a second time after a first failed attempt before:

server/lib/private/User/Manager.php

Lines 276 to 289 in ecad09b

	// since http basic auth doesn't provide a standard way of handling non ascii password we allow password to be urlencoded
	// we only do this decoding after using the plain password fails to maintain compatibility with any password that happens
	// to contain urlencoded patterns by "accident".
	$password = urldecode($password);
	foreach ($backends as $backend) {
	if ($backend instanceof ICheckPasswordBackend || $backend->implementsActions(Backend::CHECK_PASSWORD)) {
	/** @var ICheckPasswordBackend|UserInterface $backend */
	$uid = $backend->checkPassword($loginName, $password);
	if ($uid !== false) {
	return $this->getUserObject($uid, $backend);
	}
	}
	}