-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 session based CSRF token middleware #14825
Conversation
The current double submit cookie CSRF middleware has a few potential weaknesses: 1. The tokens it uses are portable between users. If a user has their browser compromised, CSRF tokens can be re-used between other compromised users. 2. Tokens don't have an expiration time. If a CSRF token is leaked and a user is compromised an old token can be used again. These changes introduce an alternative CSRF middleware that stores the compare token in the session. This removes the need to do HMAC signing as the compare value is in the session where an attacker cannot change it. Furthermore, it also couples CSRF tokens to the user, and to the specific session. This mitigates both weaknesses with our existing CSRF middleware. I didn't want to make the existing CSRF middleware more complicated and risk breaking backwards compatibility which is why this is a new middleware. A separate middleware makes opt-ing into session based tokens explicit.
* | ||
* @var array | ||
*/ | ||
protected $_config = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected $_config = [ | |
protected $config = [ |
What is the cakephp policy for underscore protected? Are we transitioning away? It seems we randomly avoid them and randomly copy them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally avoid the underscore for new protected properties but for $_config
I would prefer keeping it for consistency with other classes.
We'll have to live with this till v5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could start making things better now so there are fewer things to change in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my personal code I still use underscore prefix for protected, so I don't really mind it :)
This essentially only protects other users, right? If the browser is compromised then the user's session is essentially compromised. If they do fix the browser exploit then they are only vulnerable until the session is changed. |
Co-authored-by: othercorey <corey.taylor.fl@gmail.com>
May I ask why this did not land with 4.1.3 or 4.1.4 release? |
New features generally go into the next point release. This makes it easier to document changes. |
CakePHP follows semver. New features will be in next minor release (4.2.0). |
If you need it right now you could copy/paste the code as an application middleware until 4.2 ships. |
Thank you both for explanation :) |
The current double submit cookie CSRF middleware has a few potential weaknesses:
These changes introduce an alternative CSRF middleware that stores the compare token in the session. This removes the need to do HMAC signing as the compare value is in the session where an attacker cannot change it. Furthermore, it also couples CSRF tokens to the user, and to the specific session. This mitigates both weaknesses with our existing CSRF middleware.
I didn't want to make the existing CSRF middleware more complicated and risk breaking backwards compatibility which is why this is a new middleware. A separate middleware makes opt-ing into session based tokens explicit.