Fix CWE-352 CSRF protection weakness. #1375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
theseanything
approved these changes
Jan 25, 2024
|
thanks! |
|
@sengi just been looking at this again - I don't think we memoize session data anywhere and aren't affected by this security issue right now. It's definitely sensible to throw an exception on missing/invalid CSRF token and protects against future code changes. However, this behaviour breaks the deployment API when the request is authenticated via bearer token (because we don't need a CSRF token, but still check for one). To fix that we need implement a way to control the authentication strategies of gds-sso per controller - currently can only disable session authentication for whole Rails app. I propose we revert this change as to fix the deployment API and allow Release to show up to date deployment information, whilst we work on gds-sso to remove the need for CSRF tokens for the deployment API. |
|
No probs, sorry thought this would be an easy win but I was wrong :( Thanks for investigating! |
chrisroos
added a commit
that referenced
this pull request
Feb 7, 2024
This was previously enabled in PR #1375 but then reverted in PR #1383 when it was discovered that enabling it had silently broken the ability for Release API clients to create deployment records because they don't (and shouldn't need to) send the CSRF token in the request. I'm avoiding the problem seen in PR #1375 by skipping forgery protection if we detect a Bearer token in the Authorization HTTP Header. I'm doing this in the same way that GDS::SSO checks whether the `gds_sso` Warden strategy is valid[2] or whether it needs to use the `gds_bearer_token` strategy instead[3] so I'm fairly confident this is good enough to distinguish an API request from a browser request. One of the two new controller tests should fail if we either: - remove the `with: :exception` option to `protect_from_forgery` - require the CSRF token when an API client authenticates using a Bearer token and creates a deployment Note that I'm assuming that `Deployments#create` is the only API endpoint. If it turns out there are more API endpoints then we'll need to make the same change to those. I was interested to learn that raising an exception is the default behaviour in new Rails apps[3] but that we were overriding that default with our explicit call to `protect_from_forgery` without any arguments. Adding `protect_from_forgery with: :exception` is the same as removing `protect_from_forgery` entirely, although I think the former is more explicit. [1]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L38 [2]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L64 [3]: https://guides.rubyonrails.org/security.html#required-security-token
chrisroos
added a commit
that referenced
this pull request
Feb 7, 2024
This was previously enabled in PR #1375 but then reverted in PR #1383 when it was discovered that enabling it had silently broken the ability for Release API clients to create deployment records because they don't (and shouldn't need to) send the CSRF token in the request. I'm avoiding the problem seen in PR #1375 by skipping forgery protection if we detect a Bearer token in the Authorization HTTP Header. I'm doing this in the same way that GDS::SSO checks whether the `gds_sso` Warden strategy is valid[1] or whether it needs to use the `gds_bearer_token` strategy instead[2] so I'm fairly confident this is good enough to distinguish an API request from a browser request. One of the two new controller tests should fail if we either: - remove the `with: :exception` option to `protect_from_forgery` - require the CSRF token when an API client authenticates using a Bearer token and creates a deployment Note that I'm assuming that `Deployments#create` is the only API endpoint. If it turns out there are more API endpoints then we'll need to make the same change to those. I was interested to learn that raising an exception is the default behaviour in new Rails apps[3] but that we were overriding that default with our explicit call to `protect_from_forgery` without any arguments. Adding `protect_from_forgery with: :exception` is the same as removing `protect_from_forgery` entirely, although I think the former is more explicit. [1]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L38 [2]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L64 [3]: https://guides.rubyonrails.org/security.html#required-security-token
AgaDufrat
pushed a commit
that referenced
this pull request
May 17, 2024
This was previously enabled in PR #1375 but then reverted in PR #1383 when it was discovered that enabling it had silently broken the ability for Release API clients to create deployment records because they don't (and shouldn't need to) send the CSRF token in the request. I'm avoiding the problem seen in PR #1375 by skipping forgery protection if we detect a Bearer token in the Authorization HTTP Header. I'm doing this in the same way that GDS::SSO checks whether the `gds_sso` Warden strategy is valid[1] or whether it needs to use the `gds_bearer_token` strategy instead[2] so I'm fairly confident this is good enough to distinguish an API request from a browser request. One of the two new controller tests should fail if we either: - remove the `with: :exception` option to `protect_from_forgery` - require the CSRF token when an API client authenticates using a Bearer token and creates a deployment Note that I'm assuming that `Deployments#create` is the only API endpoint. If it turns out there are more API endpoints then we'll need to make the same change to those. I was interested to learn that raising an exception is the default behaviour in new Rails apps[3] but that we were overriding that default with our explicit call to `protect_from_forgery` without any arguments. Adding `protect_from_forgery with: :exception` is the same as removing `protect_from_forgery` entirely, although I think the former is more explicit. [1]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L38 [2]: https://github.com/alphagov/gds-sso/blob/8cc1427bfcd3cbfa24904040c8eaccff45434322/lib/gds-sso/warden_config.rb#L64 [3]: https://guides.rubyonrails.org/security.html#required-security-token
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://www.veracode.com/blog/managing-appsec/when-rails-protectfromforgery-fails
Addresses https://github.com/alphagov/release/security/code-scanning/1.