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

Add CSRF check on login and logout #8443

Merged
merged 3 commits into from
Jun 2, 2014
Merged

Conversation

LukasReschke
Copy link
Member

This is a minor issue and not worth a backport in my opinion as it could break more things than it's worth having it.

This is a minor issue and not worth a backport in my opinion as it could break more things than it's worth having it.
@ghost
Copy link

ghost commented May 4, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4550/

@LukasReschke
Copy link
Member Author

@PVince81 @bantu @karlitschek
May I ask you to review this? - Testing is trivial. Just verify that the login and logout is still working :-)
(Additionally you can remove the token from the request to see the CSRF exception triggered)

The reason for this change is that an attacker may force the victim to send a crafted logout request and then a login request with the attackers credentials. A users could then be tricked to upload/modify data without knowing that he is logged into another account => the attacker is able to access this data too.
Since this involves massive user interaction I do not consider this worth a backport.

@karlitschek
Copy link
Contributor

makes sense. Can´t test at the moment 👍

@PVince81
Copy link
Contributor

PVince81 commented May 7, 2014

Is there some regression testing to be done here as well ?
In the past we had cases where people had the "remember me" cookie set but after upgrading they got a message about a possible CSRF and couldn't login any more.

I suppose this here might need upgrade testing as well, then ?

@PVince81
Copy link
Contributor

PVince81 commented May 7, 2014

Ok, it looks like this one doesn't involve cookies.
I'll test logging in and out.

@PVince81
Copy link
Contributor

PVince81 commented May 7, 2014

In general it seems to work but I found a corner case:

  1. Login
  2. Clear cookies to kill the session
  3. From the still visible OC window, select "Log out"
  4. In the login page, login again
  5. A JSON message appears "Token expired".

This is because the login page tries to redirect to the "logout" URL from step 3)

This is needed to prevent "Token expired" messages while login if a session is expired
@see #8443 (comment)
@LukasReschke
Copy link
Member Author

@PVince81 Nice catch! - Could have created some confusions in case a session was invalidated due to timeout/whatever.

I've added a commit to prevent that situation. Please review.

@ghost
Copy link

ghost commented May 11, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4688/

Conflicts:
	core/templates/login.php
@DeepDiver1975
Copy link
Member

rebased - tested 👍

@scrutinizer-notifier
Copy link

The inspection completed: 4 new issues

@ghost
Copy link

ghost commented May 19, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/4870/

@LukasReschke
Copy link
Member Author

@blizzz My 🔮 said you'd be happy to review this!

@DeepDiver1975
Copy link
Member

@owncloud-bot retest this please

@ghost
Copy link

ghost commented May 30, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5178/

@DeepDiver1975
Copy link
Member

@owncloud-bot retest this please

@ghost
Copy link

ghost commented May 30, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5179/

@PVince81
Copy link
Contributor

PVince81 commented Jun 2, 2014

👍 works, also works with the case mentionned in #8443 (comment)

PVince81 pushed a commit that referenced this pull request Jun 2, 2014
@PVince81 PVince81 merged commit 4e957c7 into master Jun 2, 2014
@PVince81 PVince81 deleted the csrf-on-login-and-logout branch June 2, 2014 09:27
@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants