Skip to content

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Aug 31, 2022

Either because of concurrency or due to a malicious actor, it is possible that Nextcloud receives a request with set, but invalid cookies. Those are nc_username, nc_token and nc_session_id. \OC::handleLogin triggers this logic.

The verification logic works as expected. We don't let a request pass that doesn't have valid values for those three cookies.

However, the error handling is bogus. If a legitimate user runs into the situation where the nc_token does no longer exist in the stored user keys, they are sent to the login page in an infinite loop.

How to test

In the browser

This scenario is hard to trigger inside a browser, but not impossible.

  1. Check out master or this branch
  2. Open a private window
  3. Log in
  4. Restart your webserver to kill sessions, and run DELETE FROM oc_preferences WHERE appid = 'login_token'; to clear login tokens from the database
  5. Reload the page
  6. See the login page
  7. Try to log in

On master: endless login loop. This doesn't happen every time. But if it happens the user is doomed until the clear cookies.
On this branch: login works at the second attempt. That is because the first login detects the invalid token and restarts the session. That generates new tokens and the second login works. This isn't great either, but the users have a way out of the login loop.

With Curl

echo > nc_cookies.txt
curl -k https://localhost/login -b nc_cookies.txt -c nc_cookies.txt
echo "#HttpOnly_localhost	FALSE	/	TRUE	0	nc_username	admin" >> nc_cookies.txt
echo "#HttpOnly_localhost	FALSE	/	TRUE	0	nc_token	abcdef" >> nc_cookies.txt
echo "#HttpOnly_localhost	FALSE	/	TRUE	0	nc_session_id	12345" >> nc_cookies.txt
curl -k -X POST https://localhost/login -d "user=admin&password=admin&timezone=Europe/Vienna&timezone_offset=2&requesttoken=abc" -b nc_cookies.txt -c nc_cookies.txt -v

On master: the failed login request does not clear nc_* cookies.
On this branch the (possibly invalid but certainly useless) cookies are cleared.

* Replaced cookie nc_username="deleted" for domain localhost, path /, expire 1
< set-cookie: nc_username=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; secure; HttpOnly
* Replaced cookie nc_token="deleted" for domain localhost, path /, expire 1
< set-cookie: nc_token=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; secure; HttpOnly
* Replaced cookie nc_session_id="deleted" for domain localhost, path /, expire 1
< set-cookie: nc_session_id=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; secure; HttpOnly
* Added cookie nc_username="deleted" for domain localhost, path /, expire 1
< set-cookie: nc_username=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
* Added cookie nc_token="deleted" for domain localhost, path /, expire 1
< set-cookie: nc_token=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
* Added cookie nc_session_id="deleted" for domain localhost, path /, expire 1
< set-cookie: nc_session_id=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; HttpOnly
* Replaced cookie oc4uhez60xks="acc3823309202d99421398e91f5ecf12" for domain localhost, path /, expire 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants