server icon indicating copy to clipboard operation
server copied to clipboard
verified publisher icon nextcloud

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

Open Samonitari opened this issue 3 years ago • 9 comments

⚠️ This issue respects the following points: ⚠️

  • [X] This is a bug, not a question or a configuration/webserver/proxy issue.
  • [X] This issue is not already reported on Github (I've searched it).
  • [X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
  • [x] Nextcloud Server is running on 64bit capable CPU, PHP and OS.
  • [X] 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](https://github.com/nextcloud/server/blob/baf74f0aa1b8947272b10532a3c3a1cff16b6085/apps/user_ldap/lib/User/User.php#L651) 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)
  • [X] 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

Samonitari avatar Nov 15 '22 10:11 Samonitari

Which nc version?

szaimen avatar Nov 15 '22 11:11 szaimen

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 avatar Nov 15 '22 11:11 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

Samonitari avatar Nov 15 '22 11:11 Samonitari

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

szaimen avatar Nov 19 '22 11:11 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+

szaimen avatar May 22 '23 09:05 szaimen

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.

nextcloud-command avatar Jun 25 '23 00:06 nextcloud-command

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?

mario-spitze avatar Oct 02 '23 13:10 mario-spitze

We're seeing a possibly related error in our 26.0.4 Enterprise.

Users, newly created in LDAP cannot log in and instead get an "Internal Server Error".

The logs say:

[[index] Fehler: Exception: array_key_exists(): Argument #2 ($array) must be of type array, null given in file '/var/www/html/apps/user_ldap/lib/User/User.php' line 653 at <<closure>>

0. /var/www/html/lib/private/AppFramework/App.php line 183
   OC\AppFramework\Http\Dispatcher->dispatch(["OC\\Core\\Controller\\LoginController"], "tryLogin")
1. /var/www/html/lib/private/Route/Router.php line 315
   OC\AppFramework\App::main("OC\\Core\\Controller\\LoginController", "tryLogin", ["OC\\AppFramewo ... "], ["core.login.tryLogin"])
2. /var/www/html/lib/base.php line 1065
   OC\Route\Router->match("/login")
3. /var/www/html/index.php line 36
   OC::handleRequest()

Caused by:

TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given at <<closure>>

 0. /var/www/html/apps/user_ldap/lib/User/User.php line 653
    array_key_exists("pwdpolicysubentry", "*** sensitive parameters replaced ***")
 1. /var/www/html/lib/private/legacy/OC_Hook.php line 105
    OCA\User_LDAP\User\User->handlePasswordExpiry([true,"829b70fe- ... "])
 2. /var/www/html/lib/private/Server.php line 626
    OC_Hook::emit("OC_User", "post_login", [true,"829b70fe- ... "])
 3. <<closure>>
    OC\Server->OC\{closure}("*** sensitive parameters replaced ***")
 4. /var/www/html/lib/private/Hooks/EmitterTrait.php line 105
    call_user_func_array(["Closure"], ["*** sensitive  ... "])
 5. /var/www/html/lib/private/Hooks/PublicEmitter.php line 40
    OC\Hooks\BasicEmitter->emit("\\OC\\User", "postLogin", ["*** sensitive  ... "])
 6. /var/www/html/lib/private/User/Session.php line 397
    OC\Hooks\PublicEmitter->emit("\\OC\\User", "postLogin", ["*** sensitive  ... "])
 7. /var/www/html/lib/private/Authentication/Login/CompleteLoginCommand.php line 39
    OC\User\Session->completeLogin("*** sensitive parameters replaced ***")
 8. /var/www/html/lib/private/Authentication/Login/ALoginCommand.php line 39
    OC\Authentication\Login\CompleteLoginCommand->process(["OC\\Authentication\\Login\\LoginData"])
 9. /var/www/html/lib/private/Authentication/Login/LoggedInCheckCommand.php line 60
    OC\Authentication\Login\ALoginCommand->processNextOrFinishSuccessfully(["OC\\Authentication\\Login\\LoginData"])
10. /var/www/html/lib/private/Authentication/Login/ALoginCommand.php line 39
    OC\Authentication\Login\LoggedInCheckCommand->process(["OC\\Authentication\\Login\\LoginData"])
11. /var/www/html/lib/private/Authentication/Login/EmailLoginCommand.php line 68
    OC\Authentication\Login\ALoginCommand->processNextOrFinishSuccessfully(["OC\\Authentication\\Login\\LoginData"])
12. /var/www/html/lib/private/Authentication/Login/ALoginCommand.php line 39
    OC\Authentication\Login\EmailLoginCommand->process(["OC\\Authentication\\Login\\LoginData"])
13. /var/www/html/lib/private/Authentication/Login/UidLoginCommand.php line 53
    OC\Authentication\Login\ALoginCommand->processNextOrFinishSuccessfully(["OC\\Authentication\\Login\\LoginData"])
14. /var/www/html/lib/private/Authentication/Login/ALoginCommand.php line 39
    OC\Authentication\Login\UidLoginCommand->process(["OC\\Authentication\\Login\\LoginData"])
15. /var/www/html/lib/private/Authentication/Login/UserDisabledCheckCommand.php line 57
    OC\Authentication\Login\ALoginCommand->processNextOrFinishSuccessfully(["OC\\Authentication\\Login\\LoginData"])
16. /var/www/html/lib/private/Authentication/Login/ALoginCommand.php line 39
    OC\Authentication\Login\UserDisabledCheckCommand->process(["OC\\Authentication\\Login\\LoginData"])
17. /var/www/html/lib/private/Authentication/Login/PreLoginHookCommand.php line 52
    OC\Authentication\Login\ALoginCommand->processNextOrFinishSuccessfully(["OC\\Authentication\\Login\\LoginData"])
18. /var/www/html/lib/private/Authentication/Login/Chain.php line 107
    OC\Authentication\Login\PreLoginHookCommand->process(["OC\\Authentication\\Login\\LoginData"])
19. /var/www/html/core/Controller/LoginController.php line 326
    OC\Authentication\Login\Chain->process(["OC\\Authentication\\Login\\LoginData"])
20. /var/www/html/lib/private/AppFramework/Http/Dispatcher.php line 230
    OC\Core\Controller\LoginController->tryLogin("*** sensitive parameters replaced ***")
21. /var/www/html/lib/private/AppFramework/Http/Dispatcher.php line 137
    OC\AppFramework\Http\Dispatcher->executeController(["OC\\Core\\Controller\\LoginController"], "tryLogin")
22. /var/www/html/lib/private/AppFramework/App.php line 183
    OC\AppFramework\Http\Dispatcher->dispatch(["OC\\Core\\Controller\\LoginController"], "tryLogin")
23. /var/www/html/lib/private/Route/Router.php line 315
    OC\AppFramework\App::main("OC\\Core\\Controller\\LoginController", "tryLogin", ["OC\\AppFramewo ... "], ["core.login.tryLogin"])
24. /var/www/html/lib/base.php line 1065
    OC\Route\Router->match("/login")
25. /var/www/html/index.php line 36
    OC::handleRequest()

POST /index.php/login
from 10.216.212.196 by 829b70fe-f300-103d-947c-49ba57c88a4d at 2023-10-10T16:52:03+00:00]

kernstock avatar Oct 10 '23 17:10 kernstock

This Bug still exists in NC28. Also circumventing it atm with a null check:

if (is_null($result) || !is_array($result) || empty($result[0]) || !is_array($result[0])) {
    return;
}

I'm also not a php coder but this does the trick for me at the moment.

R053NR07 avatar Oct 11 '24 09:10 R053NR07

This seams to be solved with #53714 , isn't it?

mario-spitze avatar Dec 01 '25 07:12 mario-spitze