[Bug]: user_ldap crashes during login, password policy needs missing null check(s)

[Samonitari]

⚠️ 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

function⋅handlePasswordExpiry($params) in apps/user_ldap/lib/User/User.php fail to handle multiple corener cases:

• First is after this (line 651](

  server/apps/user_ldap/lib/User/User.php

  Line 651 in baf74f0

  $result = $this->access->search('objectclass=*', $this->dn, ['pwdpolicysubentry', 'pwdgraceusetime', 'pwdreset', 'pwdchangedtime']);

  )
  This returns nothing if user never changed password (and he isn't on a grace time/reset)!
• I presume (although I did NOT try it) this is wrong too (although it is null checked
  line 668
  Existing password policy doesn't mean we must have these attributes - pwdPolicy MAY have these attributes
  Although I - and most others - will have all MAY attributes set, I actually don't use these, as I think forcing users to change their passwords often leads to passwords more likely to be written down.
  But pwdCheckQuality and pwdMinLength is not zero on my server, and on other setups, these could be the only attributes set!
  So these could be neither in cache, nor in search result!

Steps to reproduce

1. Setup LDAP server, with a password policy that only has pwdCheckQuality and pwdMinLength attributes set.
2. Connect that user backend to Nextcloud
3. Create a new LDAP user. Set their password during creation, not in a separate operation
4. Try to login with the new user

Expected behavior

Newly created user should be able to log in!
If no pwdChangedTime attribute does not exist for a given user, it is not necessarily a grace login.
It it doesn't exists for a user

1. If no pwdMaxAge and pwdExpireWarning is set in policy, then nothing to check
   LDAP User could be created with throwaway token based registration, where he gets to choose his password during user creation! Maybe they are just asked at a helpdesk PC to type in a password, I know, it's horror....
2. If these policy attrs are set, and pwdChangedTime is empty, use createTimeStamp

Furthermore, don't blindly override ppolicy cache with a search result that doesn't return every attribute

Installation method

Community Manual installation with Archive

Operating system

Other

PHP engine version

PHP 8.0

Web server

Other

Database engine version

PostgreSQL

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

Fresh Nextcloud Server install

Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

What user-backends are you using?

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

Configuration report

Irrelevant

List of activated Apps

Irrelevant

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

No response

[szaimen]

Which nc version?

[Samonitari]

Which nc version?

24.0.7

I attach my hotfix:

diff -urN apps.orig/user_ldap/lib/User/User.php apps/user_ldap/lib/User/User.php
--- apps.orig/user_ldap/lib/User/User.php       2022-11-15 10:29:04.362933828 +0100
+++ apps/user_ldap/lib/User/User.php    2022-11-15 12:31:25.966933828 +0100
@@ -650,16 +650,16 @@
                        //retrieve relevant user attributes
                        $result = $this->access->search('objectclass=*', $this->dn, ['pwdpolicysubentry', 'pwdgraceusetime', 'pwdreset', 'pwdchangedtime']);

-                       if (array_key_exists('pwdpolicysubentry', $result[0])) {
+                       if (array_key_exists('0', $result) && array_key_exists('pwdpolicysubentry', $result[0])) {
                                $pwdPolicySubentry = $result[0]['pwdpolicysubentry'];
                                if ($pwdPolicySubentry && (count($pwdPolicySubentry) > 0)) {
                                        $ppolicyDN = $pwdPolicySubentry[0];//custom ppolicy DN
                                }
                        }

-                       $pwdGraceUseTime = array_key_exists('pwdgraceusetime', $result[0]) ? $result[0]['pwdgraceusetime'] : [];
-                       $pwdReset = array_key_exists('pwdreset', $result[0]) ? $result[0]['pwdreset'] : [];
-                       $pwdChangedTime = array_key_exists('pwdchangedtime', $result[0]) ? $result[0]['pwdchangedtime'] : [];
+                       $pwdGraceUseTime = (array_key_exists('0', $result) && array_key_exists('pwdgraceusetime', $result[0])) ? $result[0]['pwdgraceusetime'] : [];
+                       $pwdReset = (array_key_exists('0', $result) && array_key_exists('pwdreset', $result[0])) ? $result[0]['pwdreset'] : [];
+                       $pwdChangedTime = (array_key_exists('0', $result) && array_key_exists('pwdchangedtime', $result[0])) ? $result[0]['pwdchangedtime'] : [];

                        //retrieve relevant password policy attributes
                        $cacheKey = 'ppolicyAttributes' . $ppolicyDN;
@@ -669,9 +669,9 @@
                                $this->connection->writeToCache($cacheKey, $result);
                        }

-                       $pwdGraceAuthNLimit = array_key_exists('pwdgraceauthnlimit', $result[0]) ? $result[0]['pwdgraceauthnlimit'] : [];
-                       $pwdMaxAge = array_key_exists('pwdmaxage', $result[0]) ? $result[0]['pwdmaxage'] : [];
-                       $pwdExpireWarning = array_key_exists('pwdexpirewarning', $result[0]) ? $result[0]['pwdexpirewarning'] : [];
+                       $pwdGraceAuthNLimit = (array_key_exists('0', $result) && array_key_exists('pwdgraceauthnlimit', $result[0])) ? $result[0]['pwdgraceauthnlimit'] : [];
+                       $pwdMaxAge = (array_key_exists('0', $result) && array_key_exists('pwdmaxage', $result[0])) ? $result[0]['pwdmaxage'] : [];
+                       $pwdExpireWarning = (array_key_exists('0', $result) && array_key_exists('pwdexpirewarning', $result[0])) ? $result[0]['pwdexpirewarning'] : [];

                        //handle grace login
                        if (!empty($pwdGraceUseTime)) { //was this a grace login?

[Samonitari]

That hotfix doesn't solve the case if pwdChangedTime doesn't exists for the user AND pwdMaxAge does in policies.

Offtopic: I never wrote PHP (and I am glad I didn't), except in times like this, when something broke and had to comment-out/quick-fix-smthg-with-general-programming-knowhow

[szaimen]

Hi, can you please openn a PR with your patch? This will make the discussion around it much easier. Thank you! :)

[szaimen]

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

My goal is to add a label like e.g. 26-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+

[nextcloud-command]

This issue has been automatically marked as stale because it has not had recent activity and seems to be missing some essential information. It will be closed if no further activity occurs. Thank you for your contributions.

[mario-spitze]

This bug is still present in version 26.0.5. This part in the User.php isn't changed right now.

Quick and dirty fix is a null check and a return. I think a patch like from @Samonitari will help. Or it's better to fix the search function?