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

CSRF and PDO improvements #7938

Merged
merged 4 commits into from Jan 1, 2016
Merged

CSRF and PDO improvements #7938

merged 4 commits into from Jan 1, 2016

Conversation

markstory
Copy link
Member

@ionas Sent me an IRC log snippet from ##php where @Bittarman had some good feedback for CakePHP.

Defaulting CSRF tokens to HttpOnly might cause upgrade issues, but I think its important for us to get new applications up and running in a safer mode. This change would be something highlighted in the migration guide for 3.2

I've enabled native prepares for the drivers that it seems to work on. I had very little luck in getting MySQL to use native prepares as there were issues preparing statements like SELECT 1 + 1 which doesn't give me a ton of confidence.

Any request with a 'post' data should be subject to CSRF token
validation. This lets us catch scenarios with OPTIONS has a request
body. Also force commonly methods that commonly mutate state to require
a CSRF token.

Based on feedback from @Bittarman
While this _could_ be backwards incompatible. I think it is a safer
default to start new applications with.
I could not enable native prepares for MySQL as it caused a number of
tests to fail. From looking at other projects this is due to MySQL's
native prepare methods having a few gotchas.
@markstory markstory added this to the 3.2.0 milestone Dec 31, 2015
@@ -56,7 +56,7 @@ class CsrfComponent extends Component
'cookieName' => 'csrfToken',
'expiry' => 0,
'secure' => false,
'httpOnly' => false,
'httpOnly' => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate on the reasons for this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually pretty sure this is not needed, but I'm no security expert. What would be the attack vector?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario is that the host page/application has a XSS issue, or a sketchy 3rd party script. In this case your CSRF cookie can be read and used to do CSRF attacks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The token is already readable in one of the form hidden inputs, the same xss attack would be able to display the same value. Whereas a 3rd party script would not be able to read cookies because of the same origin restriction enforced by browsers.

But what if a XSS attack can read the cookie? Well, nothing bad would happen, as what the CSRF component is protecting from, is the case of an involuntary post from another domain. Since the XSS attacks are only relevant for the same domain, there is no way this attack can be exploited from another domain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question is, do we really need a CSRF hidden input field or won't a submitted cookie be enough?

When looking at what my Forms actually do generate. Hidden fields with _Token[fields] and _Token[unlocked].
Only when adding CSRFComponent I get
What's the point of that if a cookie can be read.

I'd say CSRF-protection should work two ways:
a) DEFAULT: httpOnly cookie (which can be switched to always show something)
b) OPTIONAL: http header and html meta tag - which can be read by javascript even if you are not using cake bake form helper

Interim because of BC the current input CSRF Token would still be rendered but disable-able.
When disabling the CSRF Token, httpOnly would also be set to true (so one switch = default future safety)

Why should XSS attacks be only relevant for the same domain? An XSS attack could read the CSRF key and send it over the wire via javascript to some other server which then sets up a prepared cross site request forgery attack to be executed by some user joe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's imagine that the attacker can get the value of the cookie... or more simply done, the value of the hidden input.. How would he be able to use it? They would require the session cookie as well in order to actually be logged in on the site and make changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorenzo If the attacker submits a form, then the browser will forward the user's session cookie. I agree that an XSS exploit will let the attacker get the token out of the form.

By having the cookie be HttpOnly they cannot modify or overwrite the cookie we set. When implementing the CSRF approach I tried to follow the double submit cookies approach which implies that the cookie should not be writable from the client, but missed the httponly part of the cookie.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would the attacker submit the form? The submit needs to be initiated by the user's browser (the one having the session information).

Again, let's imagine that the attacker is able to run any code from the cake page and send the information to their servers... What is the difference between being able to read the cookie or simply read the hidden value in the form?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found this: http://security.stackexchange.com/questions/59470/double-submit-cookies-vulnerabilities

In short, if your page is vulnerable to XSS, then basically no CSRF protection can be enforced... and think I missed before how obvious that is... If you can run arbitrary code in the same origin, there is no use for reading the cookies or trying to bypass CSRF protection at all, you are already in and can submit whatever you like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in theory. Taken you submit a HTTP only cookie via HTTPS only, how would XSS interact with that?

Edit: added "cookie"

@markstory
Copy link
Member Author

Windows tests don't seem to be related to this change.

The reason this was switched was due to concern around XSS attacks
allowing access to the CSRF token through `document.cookie`. However, if
there is a successful XSS attack, the attacker doesn't need the CSRF
cookie, as they can use Javascript to submit forms. In this situation
the browser will forward the cookies automatically. Another situation
where httponly could be useful is 3rd party scripts. But those scripts
could also fetch the token from a hidden input and submit forms without
access to the cookie value.
@markstory
Copy link
Member Author

@lorenzo I've reverted off the httponly flag change as we talked about in IRC. c46244d has the conclusions we came to in case we are wrong later on 😄

@lorenzo
Copy link
Member

lorenzo commented Jan 1, 2016

Good stuff 👍

lorenzo added a commit that referenced this pull request Jan 1, 2016
@lorenzo lorenzo merged commit fa86150 into 3.2 Jan 1, 2016
@lorenzo lorenzo deleted the csrf-improvements branch January 1, 2016 23:00
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.

None yet

3 participants