Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Security] Implement stateless headers/cookies-based CSRF protection#58095

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekasnicolas-grekas commentedAug 26, 2024•
edited
Loading

QA
Branch?7.2
Bug fix?no
New feature?yes
Deprecations?no
Issues#13464
LicenseMIT

#54705 made me think about our CSRF protection and I wrote the attached CSRF token manager to implement stateless headers/cookies-based validation.

By defaults, the existing stateful manager is used.
In order to leverage this new stateless manager, one needs to list the token ids that should be managed this way:

framework:csrf_protection:stateless_token_ids:[my_stateless_token_id]
 * This CSRF token manager uses a combination of cookie and headers to validate non-persistent tokens. * * This manager is designed to be stateless and compatible with HTTP-caching. * * First, we validate the source of the request using the Origin/Referer headers. This relies * on the app being able to know its own target origin. Don't miss configuring your reverse proxy to * send the X-Forwarded-* / Forwarded headers if you're behind one. * * Then, we validate the request using a cookie and a CsrfToken. If the cookie is found, it should * contain the same value as the CsrfToken. A JavaScript snippet on the client side is responsible * for performing this double-submission. The token value should be regenerated on every request * using a cryptographically secure random generator. * * If either double-submit or Origin/Referer headers are missing, it typically indicates that * JavaScript is disabled on the client side, or that the JavaScript snippet was not properly * implemented, or that the Origin/Referer headers were filtered out. * * Requests lacking both double-submit and origin information are deemed insecure. * * When a session is found, a behavioral check is added to ensure that the validation method does not * downgrade from double-submit to origin checks. This prevents attackers from exploiting potentially * less secure validation methods once a more secure method has been confirmed as functional. * * On HTTPS connections, the cookie is prefixed with "__Host-" to prevent it from being forged on an * HTTP channel. On the JS side, the cookie should be set with samesite=strict to strengthen the CSRF * protection. The cookie is always cleared on the response to prevent any further use of the token. * * The $checkHeader argument allows the token to be checked in a header instead of or in addition to a * cookie. This makes it harder for an attacker to forge a request, though it may also pose challenges * when setting the header depending on the client-side framework in use. * * When a fallback CSRF token manager is provided, only tokens listed in the $tokenIds argument will be * managed by this manager. All other tokens will be delegated to the fallback manager.

Since it's stateless, end users won't loose their content if they take time to submit a form: even if the session is destroyed while they populate their form, remember-me will reconnect them and the form will be accepted.

Recipe update atsymfony/recipes#1337

IndraGunawan and Crovitche-1623 reacted with heart emojismnandre, Crovitche-1623, damienalexandre, Kocal, oleg-andreyev, tugrul, alquerci, and rynhndrcksn reacted with eyes emoji
@smnandre
Copy link
Member

(cc@tucksaun )

@nicolas-grekasnicolas-grekasforce-pushed thecsrf-double-submit branch 5 times, most recently from5b81f2f to601b9baCompareAugust 27, 2024 12:38
@nicolas-grekasnicolas-grekasforce-pushed thecsrf-double-submit branch 2 times, most recently from0e8e0da to42233a3CompareAugust 27, 2024 12:55
@jderusse
Copy link
Member

AWASP discourage using this pattern and recommands using a hash/crypted value coupled to the session (or JWT lifetime) instead

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#naive-double-submit-cookie-pattern-discouraged

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedAug 27, 2024•
edited
Loading

I've read that, and what I'm proposing here is significantly different from what they discourage to be considered seriously IMHO, esp with the new cookie attributes and with the style of double-submit I'm using here (which is not discussed on that page).

Aka I'd be interested in an open-minded threat model analysis of what this protects against (and what are the drawbacks)

@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedAug 27, 2024•
edited
Loading

For additional background: the OWASP page links tohttps://owasp.org/www-chapter-london/assets/slides/David_Johansson-Double_Defeat_of_Double-Submit_Cookie.pdf to give more insights about vulnerabilities with the "naive double-submit".

I read that doc too, and they don't apply to this PR:

  • MITM are ruled out by HTTPS nowadays (and no CSRF techniques can resist to MITM anyway)
  • HTTPS is enforced for cookies by the__Host- prefix, which prevents overriding them from HTTP
  • overriding the cookie isn't enough since the double-submit happensvia a custom header (which is listed as a valid CSRF-protection on that PDF and on the OWASP page)
  • the Origin header is checked also when available

Binding CSRF tokens to any server-side state is ruling out statelessness/cacheability of the strategy so I'm not looking for anything hashed/signed/etc.

jderusse reacted with thumbs up emoji

@nicolas-grekasnicolas-grekasforce-pushed thecsrf-double-submit branch 3 times, most recently from09d6d78 to66ae045CompareAugust 28, 2024 09:45
@wouterj
Copy link
Member

Fyi: I'm planning to look at this on Thursday. Need some focus time to properly digest all information in here :)

I don't mind if this gets merged before it, there is enough time till feature freeze to tweak it in a follow up PR if necessary.

nicolas-grekas reacted with heart emoji

Copy link
Member

@wouterjwouterj left a comment

Choose a reason for hiding this comment

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

I feel like it may be the case that all my comments make no sense whatsoever, I had quite the struggle following the logic. Please be patient with me 😉

One of the struggles is the name of the feature. If I'm correct, the stance of Symfony 7.2 would be: verify origin and do not use tokens for CSRF protection.
Then, for cases where origin headers might not be set (e.g. corporate environments), we fallback to CSRF token protection using a modified double-submit pattern.

If that is the case, I think we should not use "double-submit" as the main name of the new feature (it's just the fallback). Settingdouble_submit_token_ids to enable origin checks for some token IDs sounds wrong. Let's brainstorm a better name if you agree.

In addition, I'm not sure if we should default to all the complexity the double-submit brings if this is just a fallback. Do we want to push JS code to the users that they probably don't even need? Or should we somehow make the recipe optional and have some docs that says "runcomposer install csrf-double-submit-pack if you can't (always) use origin checks".

Finally, by generating the CSRF token and creating the cookie in the JS code moves a security-critical part of the double-submit flow to the application code. I'm always much more in favor of keeping security-critical parts in the vendor code (3500 contributors have more security knowledge than most dev teams using Symfony). We must make it very clear that a user should not change the cryptographically secure random generator algorithm, otherwise they open themselves to vulnerabilities (as attackers can see the algorithm, they might be able to predict the value).

Comment on lines 30 to 33
* Then, we validate the request using a cookie and a CsrfToken. If the cookie is found, it should
* contain the same value as the CsrfToken. A JavaScript snippet on the client side is responsible
* for performing this double-submission. The token value should be regenerated on every request
* using a cryptographically secure random generator.
Copy link
Member

Choose a reason for hiding this comment

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

If the cookie is found, it should contain the same value as the CsrfToken

If I'm reading the implementation correctly, this is not the case. The name of the cookie must be equal to the CsrfToken prefixed with the cookie name, the value can be anything you like.

I can't directly think of a reason why the implementation was chosen to be like this, but we need to update the comment if this is indeed the logic.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Putting the token in the name of the cookie prevents race conditions.
About the wording, it's approximate, but not incorrect. The cookie is both its name and its value. And either of them can contain the token.

Copy link
Contributor

Choose a reason for hiding this comment

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

is there any doc or howto (not talking about the recipe) for thisJavaScript snippet on the client side?
if not bounded for example with turbo or whatever high level JS lib ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The recipe is the doc on the topic. We can add a link after its merged.

94noni reacted with thumbs up emoji

Choose a reason for hiding this comment

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

@nicolas-grekas for projects that implement manual CSRF protection (e.g. with the csrf_token() twig function to manually generate a token, and some verification in the controllers), it would be great to update the recipe with an example of a Twig template showing how to construct the hidden crsf form field, so that the sample .js in the recipe can work as is (e.g. what initial values to use for the required data-attributes?) - I'm trying to get it to work on a simple login form, but I'm having trouble with the cookie value that seems to be ignored during the validation.

@nicolas-grekas
Copy link
MemberAuthor

@wouterj this double-submit is not only a fallback. It's also a defense in-depth. What I mean is that ifOrigin is broken for any reasons, users won't be immediately exposed.

About the naming, "double-submit" suits me, but I also wondered about naming this "same-origin":SameOriginCsrfManager,same_origin_token_ids, etc. I could change if that's the consensus.

@nicolas-grekas
Copy link
MemberAuthor

Any other feedback @symfony/mergers ? I'm going to merge as is without any :)

@Tobion
Copy link
Contributor

I agree that same-origin is a better name to me than double-submit because it's the main idea and is the better known term that is part of the http spec.

@javiereguiluzjaviereguiluz added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 3, 2024
@wouterj
Copy link
Member

I also prefer "same origin CSRF manager" over double-submit 👍

Other than that, this PR is fine with me. But we need to write detailed docs about the pros and cons of all CSRF protection mechanism during stabilization phase :)

@stof
Copy link
Member

stof commentedOct 7, 2024

One argument for renaming: it will avoid getting more reports in the future from people telling us that OWASP discourages the double submit pattern, because they miss that they only discouragenaive double-submit.

chalasr and alquerci reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas changed the title[Security] Implement double-submit CSRF protection[Security] Implement stateless headers/cookies-based CSRF protectionOct 8, 2024
@nicolas-grekas
Copy link
MemberAuthor

nicolas-grekas commentedOct 8, 2024•
edited
Loading

Renamed toSameSiteCsrfTokenManager.

The option is namedstateless_token_ids:

framework:csrf_protection:stateless_token_ids:[my_stateless_token_id]

@stof
Copy link
Member

stof commentedOct 8, 2024

@nicolas-grekas is it same site or same origin ? Those have different meaning.

@wouterj
Copy link
Member

Good one, I believe it's indeed same-origin (exact match of scheme+hostname, including subdomain).

@nicolas-grekas
Copy link
MemberAuthor

Indeed, good catch, fixed!

@nicolas-grekasnicolas-grekas merged commitf57a6de intosymfony:7.2Oct 8, 2024
7 of 10 checks passed
@nicolas-grekasnicolas-grekas deleted the csrf-double-submit branchOctober 8, 2024 13:14
@fabpotfabpot mentioned this pull requestOct 27, 2024
@@ -73,6 +76,7 @@ public function finishView(FormView $view, FormInterface $form, array $options):
$csrfForm = $factory->createNamed($options['csrf_field_name'], HiddenType::class, $data, [
'block_prefix' => 'csrf_token',
'mapped' => false,
'attr' => $this->fieldAttr + ['autocomplete' => 'off'],
Copy link
Member

@GromNaNGromNaNDec 24, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Theautocomplete attribute with valueoff on anhidden field seems invalid:#59294

Copy link
Member

Choose a reason for hiding this comment

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

nicolas-grekas added a commit that referenced this pull requestFeb 10, 2025
… default CSRF token id apply only to the app; not to bundles (nicolas-grekas)This PR was merged into the 7.2 branch.Discussion----------[Form][FrameworkBundle] Use auto-configuration to make the default CSRF token id apply only to the app; not to bundles| Q             | A| ------------- | ---| Branch?       | 7.2| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITAfterEasyCorp/EasyAdminBundle#6724, I realized I made a mistake in#58095:The `framework.form.csrf_protection.token_id` config option should not configure the default CSRF token id for *all* forms. Instead, we want this option to apply only to forms managed by the app. Bundles shouldn't be affected.This is what this PR does: it switches from global config to auto-configured form types only (which means app's form types).Commits-------bf1e312 [Form][FrameworkBundle] Use auto-configuration to make the default CSRF token id apply only to the app; not to bundles
kbond added a commit to symfony/maker-bundle that referenced this pull requestMar 26, 2025
…double-submit CSRF protection (nicolas-grekas)This PR was merged into the 1.x-dev branch.Discussion----------[make:security:form-login] Tweak login forms to enable double-submit CSRF protectionWill be leveraged bysymfony/symfony#58095Does no harm being enabled regardless of the Symfony version in use.---Waiting on:- [x]symfony/symfony#58095Commits-------b13f0fb Generate data-controller="csrf-protection" on CSRF fields
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@alquercialquercialquerci left review comments

@OskarStarkOskarStarkOskarStark left review comments

@stofstofstof left review comments

@jderussejderussejderusse left review comments

@94noni94noni94noni left review comments

@Crovitche-1623Crovitche-1623Crovitche-1623 left review comments

@GromNaNGromNaNGromNaN left review comments

@ro0NLro0NLro0NL left review comments

@coatpontcoatpontcoatpont left review comments

@smnandresmnandresmnandre left review comments

@alexander-schranzalexander-schranzalexander-schranz left review comments

@dunglasdunglasdunglas left review comments

@ycerutoycerutoyceruto approved these changes

@chalasrchalasrchalasr approved these changes

@TobionTobionTobion approved these changes

@wouterjwouterjwouterj approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuhxabbuh is a code owner

Assignees
No one assigned
Labels
FeatureReadySecurity❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: Reviewed
Projects
None yet
Milestone
7.2
Development

Successfully merging this pull request may close these issues.

21 participants
@nicolas-grekas@smnandre@jderusse@alexander-schranz@tugrul@alquerci@dunglas@wouterj@Tobion@stof@GromNaN@OskarStark@ro0NL@94noni@xabbuh@yceruto@chalasr@coatpont@Crovitche-1623@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp