Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Passwordless authtokens should remain passwordless #30894

Open
7 of 8 tasks
mdoggydog opened this issue Jan 28, 2022 · 7 comments
Open
7 of 8 tasks

[Bug]: Passwordless authtokens should remain passwordless #30894

mdoggydog opened this issue Jan 28, 2022 · 7 comments
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap 25-feedback bug feature: authentication needs review Needs review to determine if still applicable

Comments

@mdoggydog
Copy link

⚠️ 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.
  • I agree to follow Nextcloud's Code of Conduct.

Bug description

On (one of) my system(s), this bug is manifesting itself in two related ways:

  1. When logged in to the web UI, via SAML SSO (user_saml), after some shorter-than-usual timeout (< 1 hour), the web UI appears to have logged out and will go through the SAML auth handshake again. (Since the user still has valid credentials with the SAML IdP, this is pretty automatic, but the user will still lose any work in progress, etc.)
  2. Applications using app tokens (e.g., desktop sync client, or Android app) are logged out and the app tokens cease working. After logging back in, they are logged out again within about an hour. (Super annoying and defeats the whole purpose of having app tokens.)

What appears to be happening is:

  • Something is calling PublicKeyTokenProvider::updatePasswords(). (I'm not sure what code flow is leading to this; maybe a CALDAV client logging in?)
  • updatePasswords() scans all of the user's authtokens and updates all of them, including password-less tokens (those with a null password field) such that they are no longer password-less tokens.
  • This causes a cascade of problems later on when a formerly password-less token is used/checked again. For example:
    • A client tries to use its app token, and some NC app calls Session::getUser().
    • Session::getUser() calls Session::validateSession().
    • Session::validateSession() calls Session::validateToken().
    • Session::validateToken() calls Session::checkTokenCredentials().
    • Session::checkTokenCredentials() asks the PublicKeyTokenProvider for the token's password (i.e., getPassword()).
    • Ooops... the formerly password-less token now has a token! Session::checkTokenCredentials() calls Manager::checkPassword() to check that password!
    • Manager::checkPassword() calls Manager::checkPasswordNoLogging().
    • I'm not even sure what the password is that is being checked at this point (since there shouldn't even be one), but Manager::checkPasswordNoLogging() ends up calling User_LDAP::checkPassword() which calls User_LDAP::getLDAPUserByLoginName()... which throws an exception No user available for the given login name... because the $loginName being provided is the login_name from the token (Nextcloud's internal user-id), not the username that a user types-in to login.
    • Manager::checkPassword() fails, and the client fails to authenticate with its app token.

It is possible that, if a system is using LDAP and is using users' public LDAP user-id's as the NC internal uid (e.g., instead of using the LDAP entry UUID as the NC internal uid) that this problem would not show itself (because User_LDAP::checkPassword() could succeed, even though it should never have been called to begin with).

However, I believe the underlying problem is that password-less app tokens are being unintentionally transformed into password-bearing tokens.

Indeed, I added a simple if (is_null($t->getPassword())) { continue; } to PublicKeyTokenProvider::updatePasswords() and that appears to have fixed the problems that I was seeing.

Steps to reproduce

See above.

Expected behavior

See above.

Installation method

Manual installation

Operating system

Debian/Ubuntu

PHP engine version

PHP 7.4

Web server

Apache (supported)

Database engine version

PostgreSQL

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

Updated from a minor version (ex. 22.2.3 to 22.2.4)

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

{
    "system": {
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "localhost",
            "XXXXX.org"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "pgsql",
        "version": "22.2.3.0",
        "overwrite.cli.url": "https:\/\/XXXXX.org",
        "htaccess.RewriteBase": "\/",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "default_phone_region": "US",
        "installed": true,
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "memcache.local": "\\OC\\Memcache\\APCu",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpmode": "sendmail",
        "mail_sendmailmode": "smtp",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "ldapIgnoreNamingRules": false,
        "ldapProviderFactory": "OCA\\User_LDAP\\LDAPProviderFactory",
        "maintenance": false,
        "enabledPreviewProviders": [
            "OC\\Preview\\PNG",
            "OC\\Preview\\JPEG",
            "OC\\Preview\\GIF",
            "OC\\Preview\\HEIC",
            "OC\\Preview\\BMP",
            "OC\\Preview\\XBitmap",
            "OC\\Preview\\MP3",
            "OC\\Preview\\TXT",
            "OC\\Preview\\MarkDown",
            "OC\\Preview\\OpenDocument",
            "OC\\Preview\\Krita",
            "OC\\Preview\\Movie",
            "OC\\Preview\\PDF",
            "OC\\Preview\\Postscript"
        ],
        "theme": "",
        "loglevel": 0,
        "app_install_overwrite": [
            "files_texteditor"
        ]
    }
}

List of activated Apps

Enabled:
  - accessibility: 1.8.0
  - activity: 2.15.0
  - announcementcenter: 6.1.1
  - apporder: 0.14.0
  - calendar: 3.0.5
  - circles: 22.1.1
  - cloud_federation_api: 1.5.0
  - comments: 1.12.0
  - contacts: 4.0.7
  - contactsinteraction: 1.3.0
  - dashboard: 7.2.0
  - dav: 1.19.0
  - external: 3.9.0
  - federatedfilesharing: 1.12.0
  - federation: 1.12.0
  - files: 1.17.0
  - files_markdown: 2.3.5
  - files_pdfviewer: 2.3.1
  - files_rightclick: 1.1.0
  - files_sharing: 1.14.0
  - files_texteditor: 2.14.0
  - files_trashbin: 1.12.0
  - files_versions: 1.15.0
  - files_videoplayer: 1.11.0
  - firstrunwizard: 2.11.0
  - groupfolders: 10.0.2
  - logreader: 2.7.0
  - lookup_server_connector: 1.10.0
  - maps: 0.1.10
  - metadata: 0.15.0
  - nextcloud_announcements: 1.11.0
  - notifications: 2.10.1
  - notify_push: 0.3.0
  - oauth2: 1.10.0
  - password_policy: 1.12.0
  - photos: 1.4.0
  - privacy: 1.6.0
  - provisioning_api: 1.12.0
  - quota_warning: 1.13.0
  - recommendations: 1.1.0
  - richdocuments: 4.2.3
  - serverinfo: 1.12.0
  - settings: 1.4.0
  - sharebymail: 1.12.0
  - spreed: 12.2.2
  - support: 1.5.0
  - survey_client: 1.10.0
  - systemtags: 1.12.0
  - tasks: 0.14.2
  - theming: 1.13.0
  - twofactor_backupcodes: 1.11.0
  - updatenotification: 1.12.0
  - user_ldap: 1.12.1
  - user_saml: 4.1.1
  - user_status: 1.2.0
  - viewer: 1.6.0
  - weather_status: 1.2.0
  - workflowengine: 2.4.0
Disabled:
  - admin_audit
  - encryption
  - files_external
  - ldap_write_support
  - ldapcontacts
  - mail
  - notes
  - text

Nextcloud Signing status

No errors have been found.

Nextcloud Logs

{"reqId":"IBYIC7RBOfDmXlaHGmH3","level":0,"time":"2022-01-27T06:11:09+00:00","remoteAddr":"X.X.X.X","user":"LDAP-ENTRY-UUID-GOES-HERE","app":"user_ldap","method":"PROPFIND","url":"/remote.php/dav/files/LDAP-ENTRY-UUID-GOES-HERE/","message":"No user available for the given login name on localhost:389","userAgent":"Mozilla/5.0 (Linux) mirall/3.3.5-1 (Nextcloud, debian-5.15.0-3-amd64 ClientArchitecture: x86_64 OsArchitecture: x86_64)","version":"22.2.3.0","exception":{"Exception":"OCA\\User_LDAP\\Exceptions\\NotOnLDAP","Message":"No user available for the given login name on localhost:389","Code":0,"Trace":[{"file":"/srv/XXXX.org/nextcloud/apps/user_ldap/lib/User_LDAP.php","line":176,"function":"getLDAPUserByLoginName","class":"OCA\\User_LDAP\\User_LDAP","type":"->"},{"function":"checkPassword","class":"OCA\\User_LDAP\\User_LDAP","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/srv/XXXX.org/nextcloud/apps/user_ldap/lib/User_Proxy.php","line":81,"function":"call_user_func_array"},{"file":"/srv/XXXX.org/nextcloud/apps/user_ldap/lib/Proxy.php","line":171,"function":"walkBackends","class":"OCA\\User_LDAP\\User_Proxy","type":"->"},{"file":"/srv/XXXX.org/nextcloud/apps/user_ldap/lib/User_Proxy.php","line":224,"function":"handleRequest","class":"OCA\\User_LDAP\\Proxy","type":"->"},{"file":"/srv/XXXX.org/nextcloud/lib/private/User/Manager.php","line":251,"function":"checkPassword","class":"OCA\\User_LDAP\\User_Proxy","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/srv/XXXX.org/nextcloud/lib/private/User/Manager.php","line":228,"function":"checkPasswordNoLogging","class":"OC\\User\\Manager","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/srv/XXXX.org/nextcloud/lib/private/User/Session.php","line":764,"function":"checkPassword","class":"OC\\User\\Manager","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/srv/XXXX.org/nextcloud/lib/private/User/Session.php","line":804,"function":"checkTokenCredentials","class":"OC\\User\\Session","type":"->"},{"file":"/srv/XXXX.org/nextcloud/lib/private/User/Session.php","line":268,"function":"validateToken","class":"OC\\User\\Session","type":"->","args":["*** sensitive parameters replaced ***"]},{"file":"/srv/XXXX.org/nextcloud/lib/private/User/Session.php","line":243,"function":"validateSession","class":"OC\\User\\Session","type":"->"},{"file":"/srv/XXXX.org/nextcloud/apps/user_saml/appinfo/app.php","line":108,"function":"getUser","class":"OC\\User\\Session","type":"->"},{"file":"/srv/XXXX.org/nextcloud/lib/private/legacy/OC_App.php","line":303,"args":["/srv/XXXX.org/nextcloud/apps/user_saml/appinfo/app.php"],"function":"require_once"},{"file":"/srv/XXXX.org/nextcloud/lib/private/legacy/OC_App.php","line":185,"function":"requireAppFile","class":"OC_App","type":"::"},{"file":"/srv/XXXX.org/nextcloud/lib/private/legacy/OC_App.php","line":139,"function":"loadApp","class":"OC_App","type":"::"},{"file":"/srv/XXXX.org/nextcloud/remote.php","line":150,"function":"loadApps","class":"OC_App","type":"::"}],"File":"/srv/XXXX.org/nextcloud/apps/user_ldap/lib/User_LDAP.php","Line":161,"CustomMessage":"No user available for the given login name on localhost:389"}}

Additional info

See above.

@mdoggydog mdoggydog added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Jan 28, 2022
mdoggydog added a commit to mdoggydog/server that referenced this issue Jan 28, 2022
…rds()`

Fixes nextcloud#30894 (at least, it is supposed to).

Signed-off-by: Matt Marjanovic <maddog@mir.com>
@mdoggydog
Copy link
Author

I just submitted a tiny PR with the fix which I mentioned at the end of my bug description above.

I also forgot to mention that this issue is probably related to #26502 and its cousins (e.g., pulsejet/nextcloud-oidc-login#148).

@szaimen szaimen added 2. developing Work in progress and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Jan 28, 2022
mdoggydog added a commit to mdoggydog/server that referenced this issue Jan 28, 2022
…rds()`

Fixes nextcloud#30894 (at least, it is supposed to).

Signed-off-by: Matt Marjanovic <maddog@mir.com>
@chris246
Copy link

chris246 commented Apr 16, 2022

I think I can confirm this observation:

@mdoggydog: Something is calling PublicKeyTokenProvider::updatePasswords(). (I'm not sure what code flow is leading to this; maybe a CALDAV client logging in?)

I disabled DAV Access (not using App Passwords) to my account yesterday and I stay logged in now.

@ikogan
Copy link

ikogan commented Apr 19, 2022

I think I'm seeing something similar, it tends to happen every time my LDAP server dies for some reason. What generally happens is that the LDAP server, for some reason, starts to fail to authenticate Nextcloud. When a user logs in, this is flagged as a bad authentication by the user and all tokens are revoked. This includes SAML sessions as well as app passwords and means that users have to not only log into every single app again, but they also have to regenerate every app token.

I can create a different ticket if this isn't related but it seems to be.

@ChristophWurst
Copy link
Member

I'm not even sure what the password is that is being checked at this point

It is the password of another login. But in your case this is suspicious. Is SAML the only authentication mechanism?

because the $loginName being provided is the login_name from the token (Nextcloud's internal user-id), not the username that a user types-in to login

That smells like a bug. The token's login name should be what the user entered during login. This can, but doesn't have to be, the internal user ID.

@szaimen

This comment was marked as resolved.

@szaimen szaimen added needs info 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 2. developing Work in progress labels Jan 23, 2023
@davidmehren
Copy link

I can still reproduce this problem with 25.0.3.
I was also able to fix it via the patch from #26502 (comment)

@szaimen
Copy link
Contributor

szaimen commented Jan 29, 2023

cc @ChristophWurst @juliushaertl

@joshtrichards joshtrichards added the needs review Needs review to determine if still applicable label Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0. Needs triage Pending check for reproducibility or if it fits our roadmap 25-feedback bug feature: authentication needs review Needs review to determine if still applicable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants