Handle session closure properly for redirecting to a controller in a post_login hook #1303

GitHubUser4234 · 2016-09-07T10:36:20Z

When a redirect to a controller is performed within the method of a post_login hook, an internal error occurs and gets printed in the webserver log (not NC log):

PHP Fatal error:  
Uncaught exception 'Exception' with message 'Session has been closed - no further changes to the session are allowed' in /server/lib/private/Session/Internal.php:154
Stack trace:
#0 /server/lib/private/Session/Internal.php(64): OC\\Session\\Internal->validateSession()
#1 /server/lib/private/Session/CryptoSessionData.php(164): OC\\Session\\Internal->set('encrypted_sessi...', 'e08a3041c452d86...')
#2 /server/lib/private/Session/CryptoSessionData.php(67): OC\\Session\\CryptoSessionData->close()
#3 [internal function]: OC\\Session\\CryptoSessionData->__destruct()
#4 {main}
  thrown in /server/lib/private/Session/Internal.php on line 154

Here is an example code of this happening:

Redirect in the method of a post_login hook
Target method of the controller

The problem is caused by the CryptoSessionData trying to close a session that is already closed. Even if the example code didn't include any session related code, the error would still occur (this has been tested).

The issue has been discussed with @LukasReschke with the result that further investigation would be necessary on whether session closure is actually necessary in this situation and if yes, how to handle session closure properly.

PR #1023 depends on this, any help is appreciated, thanks 😃

Edit: A subsequent call to the next method within the controller requiring CSRF fails saying "CSRF check failed", even though a request token was posted to the server correctly. Probably the token got lost somehow because for this call an attempt to close the session is made yet again, resulting in the same stacktrace as above. So it looks like not trying to close the session would be of a help?

The text was updated successfully, but these errors were encountered:

GitHubUser4234 · 2016-09-07T10:51:34Z

Not sure whether it is of any help, by coincidence I saw that the recently reported #1282 has the same stacktrace, but in a different testing scenario. Maybe there is some correlation? If yes, one fix could close two issues 😋

nickvergessen · 2016-11-10T10:54:10Z

@GitHubUser4234 I ran into a similar issue latly, so please retry on master, whether 2cd92d0 fixes the issue

GitHubUser4234 · 2016-11-10T11:07:39Z

@nickvergessen : Thanks for the hint, I just retried, but unfortunately the error is still there. Line numbers of the stacktrace remain unchanged.

GitHubUser4234 · 2016-11-11T10:50:45Z

As discussed with @nickvergessen in IRC, this can quickly be reproduced by adding the following lines to

server/apps/dav/lib/HookManager.php

Line 143 in ce964f0

}

:

header('Location: '.\OC::$server->getURLGenerator()->linkToRouteAbsolute('core.login.showLoginForm'));
exit();

The error would then occur in the server log (NOT nc log), for instance on Apache in the error_log / ssl_error_log file.

tcitworld · 2016-11-14T18:03:02Z

Ran into the same issue while trying to have #1893 in a separate app.

tcitworld · 2016-11-14T18:12:53Z

More informations before the session issue itself happens. The default encryption module app is enabled but not used.

[Mon Nov 14 18:58:12 2016] User "tcit" removed from group "admin"
[Mon Nov 14 18:58:12 2016] unlink(/home/tcit/dev/server/data/tcit/files_encryption/OC_DEFAULT_MODULE/tcit.privateKey): Permission denied at /home/tcit/dev/server/lib/private/legacy/helper.php#215
[Mon Nov 14 18:58:12 2016] unlink(/home/tcit/dev/server/data/tcit/files_encryption/OC_DEFAULT_MODULE/tcit.publicKey): Permission denied at /home/tcit/dev/server/lib/private/legacy/helper.php#215
[Mon Nov 14 18:58:12 2016] rmdir(/home/tcit/dev/server/data/tcit/files_encryption/OC_DEFAULT_MODULE): Permission denied at /home/tcit/dev/server/lib/private/legacy/helper.php#213
[Mon Nov 14 18:58:12 2016] rmdir(/home/tcit/dev/server/data/tcit/files_encryption): Directory not empty at /home/tcit/dev/server/lib/private/legacy/helper.php#213
[Mon Nov 14 18:58:12 2016] rmdir(/home/tcit/dev/server/data/tcit): Directory not empty at /home/tcit/dev/server/lib/private/legacy/helper.php#219
[Mon Nov 14 18:58:12 2016] User deleted: "tcit"
[Mon Nov 14 18:58:12 2016] unlink(/home/tcit/dev/server/data/tcit/files_encryption/OC_DEFAULT_MODULE/tcit.publicKey): Permission denied at /home/tcit/dev/server/lib/private/Files/Storage/Local.php#230
[Mon Nov 14 18:58:12 2016] Logout occurred
[Mon Nov 14 18:58:12 2016] session_regenerate_id(): Cannot regenerate session id - session is not active at /home/tcit/dev/server/lib/private/Session/Internal.php#115

GitHubUser4234 · 2016-11-15T09:31:02Z

Ran into the same issue while trying to have #1893 in a separate app.

Exact the same stacktrace as in my first post appears in your logs? And also a similar situation (post_login hook -> Controller)?

MorrisJobke · 2016-11-15T09:31:49Z

@LukasReschke and @nickvergessen want to look into this again for Nextcloud 11

tcitworld · 2016-11-15T09:39:02Z

Exact the same stacktrace as in my first post appears in your logs?

Yes.

And also a similar situation (post_login hook -> Controller)?

No, trying to delete an user and disconnect him.

GitHubUser4234 · 2016-11-15T09:39:48Z

@LukasReschke and @nickvergessen want to look into this again for Nextcloud 11

Very happy, thanks guys :)

GitHubUser4234 · 2016-11-15T09:42:12Z

Exact the same stacktrace as in my first post appears in your logs?

Yes.

And also a similar situation (post_login hook -> Controller)?

No, trying to delete an user and disconnect him.

Interesting information, so seems the problem might have a wider impact, hm...

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

LukasReschke · 2016-11-15T11:53:43Z

Minimal example is in the branch minimal-example-for-1303. See 0191995

To test this try to login and look at /var/log/apache2/error.log:

[Tue Nov 15 12:51:59.122300 2016] [:error] [pid 3689] [client 10.211.55.2:61533] PHP Fatal error:  Uncaught Exception: Session has been closed - no further changes to the session are allowed in /media/psf/stable9/lib/private/Session/Internal.php:154\nStack trace:\n#0 /media/psf/stable9/lib/private/Session/Internal.php(64): OC\\Session\\Internal->validateSession()\n#1 /media/psf/stable9/lib/private/Session/CryptoSessionData.php(164): OC\\Session\\Internal->set('encrypted_sessi...', '2189243c78b2480...')\n#2 /media/psf/stable9/lib/private/Session/CryptoSessionData.php(67): OC\\Session\\CryptoSessionData->close()\n#3 [internal function]: OC\\Session\\CryptoSessionData->__destruct()\n#4 {main}\n  thrown in /media/psf/stable9/lib/private/Session/Internal.php on line 154

cc @rullzer

If the session is cleared and closed for whatever reason the loadVersion will write to the session anyways. This will lead to an exception. This should fix #1303 Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

MorrisJobke · 2016-11-15T22:14:31Z

@GitHubUser4234 🙌

GitHubUser4234 · 2016-11-16T09:55:06Z

Thanks a lot @MorrisJobke @nickvergessen @LukasReschke @rullzer for the fix :))

The fix solves the major part of the problem, however a flaw has been found which I'm currently discussing with @LukasReschke :

Remove @NoCSRFRequired from thetryLogin() method in the LoginController and then run the minimal-example-for-1303 (see above): The first time when clicking the login button AFTER the post_login hook was called, CSRF fails. However when clicking the browser's back button and trying it again, the error doesn't occur again.

GitHubUser4234 · 2016-11-16T09:55:43Z

A reminder for everyone still running into the same problem even after applying the fix: In case the methods involved work with sessions, you also need add the "@usesession" annotation for them.

rullzer · 2016-11-17T14:34:52Z

Mmmm @GitHubUser4234 I think this is because we do write a requesttoken into the session. Then the second time we want to compare that but it fails hard. I will discuss with @LukasReschke and see if we can fix this.

GitHubUser4234 · 2016-11-17T15:13:00Z

Mmmm @GitHubUser4234 I think this is because we do write a requesttoken into the session. Then the second time we want to compare that but it fails hard. I will discuss with @LukasReschke and see if we can fix this.

Awesome :))

rullzer · 2016-11-24T21:26:27Z

As discussed today on IRC. Lets try to solve this somewhat safe early in the 12 cycle.

jospoortvliet · 2017-02-28T09:39:48Z

'early' would be now, I suppose :D

GitHubUser4234 · 2017-02-28T11:15:44Z

'early' would be now, I suppose :D

Hehe, yeah would be really great if someone could have a look at it :)

GitHubUser4234 · 2017-03-13T07:34:55Z

@rullzer Hem, hem :)

rullzer · 2017-03-13T15:14:08Z

Ah yes. sorry this slipped off my radar. No promises but I have it on the list for this week... Now we just pray nothing comes up

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

This is a hacky way to allow the use case of #1303. What happens is 1. User tries to login 2. PreLoginHook kicks in and figures out that the user need to change their LDAP password or whatever => redirects user 3. While loading the redirect some logic of ours kicks in and logouts the user (thus clearing the session). 4. We render the new page but now the session and the page disagree about the CSRF token This is kind of hacky but I don't think it introduces new attack vectors. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Do not clear CSRF token on logout (fix for #1303)

GitHubUser4234 · 2017-03-31T06:16:56Z

Cool, it has been merged, thanks a lot!!!

GitHubUser4234 mentioned this issue Sep 7, 2016

Handle password expiry in user_ldap #1023

Merged

nickvergessen added the bug label Sep 12, 2016

MorrisJobke mentioned this issue Nov 15, 2016

Nextcloud 11 overview #745

Closed

47 tasks

MorrisJobke added this to the Nextcloud 11.0 milestone Nov 15, 2016

LukasReschke added a commit that referenced this issue Nov 15, 2016

Example for #1303

0191995

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

rullzer mentioned this issue Nov 15, 2016

Do not cache version info in the session #2144

Merged

MorrisJobke closed this as completed in #2144 Nov 15, 2016

GitHubUser4234 mentioned this issue Nov 16, 2016

Authentication problem after upgrade to 10.0.0 #1282

Closed

LukasReschke reopened this Nov 17, 2016

rullzer modified the milestones: Nextcloud 12.0, Nextcloud 11.0 Nov 24, 2016

rullzer assigned rullzer and LukasReschke Nov 24, 2016

nickvergessen mentioned this issue Dec 14, 2016

Nextcloud 12 overview #2672

Closed

59 tasks

andyboeh mentioned this issue Feb 16, 2017

Login delayed after upgrade to NC11 nextcloud/user_saml#89

Closed

rullzer pushed a commit that referenced this issue Mar 13, 2017

Example for #1303

4261a6b

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

rullzer mentioned this issue Mar 13, 2017

Do not clear CSRF token on logout (fix for #1303) #3832

Merged

1 task

rullzer added a commit that referenced this issue Mar 30, 2017

Merge pull request #3832 from nextcloud/fix_1303

548871a

Do not clear CSRF token on logout (fix for #1303)

GitHubUser4234 closed this as completed Mar 31, 2017

reswild mentioned this issue Jun 9, 2017

Has CAS_user supported Nextcloud? owncloud-archive/user_cas#39

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle session closure properly for redirecting to a controller in a post_login hook #1303

Handle session closure properly for redirecting to a controller in a post_login hook #1303

GitHubUser4234 commented Sep 7, 2016 •

edited

Loading

GitHubUser4234 commented Sep 7, 2016

nickvergessen commented Nov 10, 2016

GitHubUser4234 commented Nov 10, 2016 •

edited

Loading

GitHubUser4234 commented Nov 11, 2016 •

edited

Loading

tcitworld commented Nov 14, 2016

tcitworld commented Nov 14, 2016

GitHubUser4234 commented Nov 15, 2016

MorrisJobke commented Nov 15, 2016

tcitworld commented Nov 15, 2016

GitHubUser4234 commented Nov 15, 2016

GitHubUser4234 commented Nov 15, 2016

LukasReschke commented Nov 15, 2016

MorrisJobke commented Nov 15, 2016

GitHubUser4234 commented Nov 16, 2016

GitHubUser4234 commented Nov 16, 2016 •

edited

Loading

rullzer commented Nov 17, 2016

GitHubUser4234 commented Nov 17, 2016

rullzer commented Nov 24, 2016

jospoortvliet commented Feb 28, 2017

GitHubUser4234 commented Feb 28, 2017 •

edited

Loading

GitHubUser4234 commented Mar 13, 2017

rullzer commented Mar 13, 2017

GitHubUser4234 commented Mar 31, 2017

Handle session closure properly for redirecting to a controller in a post_login hook #1303

Handle session closure properly for redirecting to a controller in a post_login hook #1303

Comments

GitHubUser4234 commented Sep 7, 2016 • edited Loading

GitHubUser4234 commented Sep 7, 2016

nickvergessen commented Nov 10, 2016

GitHubUser4234 commented Nov 10, 2016 • edited Loading

GitHubUser4234 commented Nov 11, 2016 • edited Loading

tcitworld commented Nov 14, 2016

tcitworld commented Nov 14, 2016

GitHubUser4234 commented Nov 15, 2016

MorrisJobke commented Nov 15, 2016

tcitworld commented Nov 15, 2016

GitHubUser4234 commented Nov 15, 2016

GitHubUser4234 commented Nov 15, 2016

LukasReschke commented Nov 15, 2016

MorrisJobke commented Nov 15, 2016

GitHubUser4234 commented Nov 16, 2016

GitHubUser4234 commented Nov 16, 2016 • edited Loading

rullzer commented Nov 17, 2016

GitHubUser4234 commented Nov 17, 2016

rullzer commented Nov 24, 2016

jospoortvliet commented Feb 28, 2017

GitHubUser4234 commented Feb 28, 2017 • edited Loading

GitHubUser4234 commented Mar 13, 2017

rullzer commented Mar 13, 2017

GitHubUser4234 commented Mar 31, 2017

GitHubUser4234 commented Sep 7, 2016 •

edited

Loading

GitHubUser4234 commented Nov 10, 2016 •

edited

Loading

GitHubUser4234 commented Nov 11, 2016 •

edited

Loading

GitHubUser4234 commented Nov 16, 2016 •

edited

Loading

GitHubUser4234 commented Feb 28, 2017 •

edited

Loading