Skip to content

[Security] Randomize CSRF token to harden BREACH attacks #39919

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 23, 2021
Merged

[Security] Randomize CSRF token to harden BREACH attacks #39919

merged 1 commit into from
Jan 23, 2021

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Jan 21, 2021 •
edited by chalasr
Loading

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR TODO

This PR randomize the CSRF token in each request in order to hardening the BREACH attack

@jderusse jderusse changed the title Randomize response length to harden BREACH attacks Randomize CSRF token to harden BREACH attacks Jan 21, 2021
@carsonbot carsonbot changed the title Randomize CSRF token to harden BREACH attacks [Security] Randomize CSRF token to harden BREACH attacks Jan 21, 2021
@nicolas-grekas nicolas-grekas added this to the 5.x milestone Jan 21, 2021
@nicolas-grekas
Copy link
Member

(failure is related)

@jderusse jderusse force-pushed the sec-breach branch 4 times, most recently from e24c286 to cb16125 Compare January 21, 2021 14:37
@jderusse
Copy link
Member Author

last failed test is about tests in branch < 5.3 working with my patch and failed.
The test tries to generte 2 logout links (with CSRF token) and expect there are identical which is exactly what my PR prevent.

Once this PR merge, I've to fix the test in downstream branches

@jderusse jderusse mentioned this pull request Jan 21, 2021
Merged
jderusse added a commit that referenced this pull request Jan 21, 2021
@jderusse
minor #39923 [SecurityBundle] Remove wrong test (jderusse)
039fe6a 
This PR was merged into the 4.4 branch.

Discussion
----------

[SecurityBundle] Remove wrong test

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This tests, asserts that all links to logout are identical, which is wrong and incompatible with BREACH mitigation #39919

Commits
-------

91c360e Remove wrong test
@jderusse
Copy link
Member Author

green \o/

@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit 4c2375f into symfony:5.x Jan 23, 2021
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 23, 2021
Closed
@jderusse jderusse deleted the sec-breach branch January 25, 2021 09:16
@javiereguiluz javiereguiluz mentioned this pull request Jan 26, 2021
Closed
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
@javiereguiluz javiereguiluz mentioned this pull request May 2, 2021
Closed
@liweiyi88
Copy link

Hi Team, sorry to add a post-merge comment but I am learning and curious about the implementation choice used in this PR.
I am not a security expert on encryption but I read that people mentioned XOR encryption is not that secure compared with AES related algorithm. I wonder if anyone could tell if it is more secure/better to achieve encryption by using openssl related method like what has been implemented in Laravel Encryption?

@jderusse
Copy link
Member Author

jderusse commented May 17, 2021 •
edited
Loading

BREACH attack is all about guessing a secret the content of an encrypted page by comparing the size of a compressed page.

There is nothing secret here (as an evidence: the KEY used to XOR is included in the payload).
We only need to generate random content

@stof
Copy link
Member

stof commented May 17, 2021

To be clear, this PR does not even implement encryption at all (so there is no question about whether the encryption is secure or no)

@liweiyi88
Copy link

Cool, thanks for the explainations!

@allan-simon allan-simon mentioned this pull request Jun 14, 2021
Closed
@ausi ausi mentioned this pull request Jan 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@jderusse @nicolas-grekas @liweiyi88 @stof @javiereguiluz @carsonbot