Skip to content

Fix GHSA-49jp-pjc3-2532: Strengthen CSRF Protections #18769

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

Merged
merged 2 commits into from
Aug 13, 2023

Conversation

U8NWXD
Copy link
Member

@U8NWXD U8NWXD commented Aug 12, 2023

Overview

  1. This PR fixes GHSA-49jp-pjc3-2532, an un-disclosed security vulnerability.

  2. This PR does the following: Adds the following protections:

    • A nonce to prevent collisions. This is best practice per OWASP.
    • A constant-time comparison of expected and received tokens. This prevents timing attacks.
    • HMAC-SHA256 instead of HMAC-MD5. Even though HMAC-MD5 is not broken yet, attacks against MD5 have been getting better, and there's no reason not to switch to SHA256.

    For more details on CSRF and protections, see OWASP's guidance.

  3. (For bug-fixing PRs only) The original bug occurred because: See security advisory (once disclosed).

Essential Checklist

  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...",
    followed by a short, clear summary of the changes.
  • I have followed the instructions for making a code change.
  • The linter/Karma presubmit checks have passed on my local machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
  • I have assigned the correct reviewers to this PR (or will leave a
    comment with the phrase "@{{reviewer_username}} PTAL" if I don't have
    permissions to assign reviewers directly).

Proof that changes are correct

Screen.Recording.2023-08-12.at.09.59.13.mov

PR Pointers

  • Never force push! If you do, your PR will be closed.
  • Make sure your PR follows the style guide, otherwise this will lead to review delays.
  • Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
  • See the Code Owner's wiki page for what code owners will expect.

Adds the following protections:
* A nonce to prevent collisions. This is best practice per OWASP.
* A constant-time comparison of expected and received tokens.
* HMAC-SHA256 instead of HMAC-MD5. Even though HMAC-MD5 is not broken
  yet, attacks against MD5 have been getting better, and there's no
  reason not to switch to SHA256.
@U8NWXD U8NWXD requested a review from a team as a code owner August 12, 2023 14:26
@U8NWXD U8NWXD requested a review from seanlip August 12, 2023 14:26
@oppiabot
Copy link

oppiabot bot commented Aug 12, 2023

Hi @U8NWXD please assign the required reviewer(s) for this PR. Thanks!

@U8NWXD U8NWXD assigned seanlip and unassigned U8NWXD Aug 12, 2023
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@seanlip seanlip assigned U8NWXD and unassigned seanlip Aug 12, 2023
@oppiabot oppiabot bot added the PR: LGTM label Aug 12, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 12, 2023

Hi @U8NWXD, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@U8NWXD U8NWXD added this pull request to the merge queue Aug 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 12, 2023
@U8NWXD U8NWXD added this pull request to the merge queue Aug 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 12, 2023
@U8NWXD U8NWXD added this pull request to the merge queue Aug 12, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2023
@U8NWXD U8NWXD added this pull request to the merge queue Aug 13, 2023
Merged via the queue into oppia:develop with commit b89bf80 Aug 13, 2023
@U8NWXD U8NWXD deleted the strengthen-csrf branch August 13, 2023 02:54
@kevintab95 kevintab95 added the PR: Needs to be hotfixed PR needs to be hotfixed for current release label Aug 14, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 14, 2023

Hi, @oppia/release-coordinators flagging this pull request for for your attention since this is labelled as a hotfix PR. Please ensure that you add the "PR: for current release" label if the next release is in progress. Thanks!

@kevintab95 kevintab95 added PR: released and removed PR: Needs to be hotfixed PR needs to be hotfixed for current release labels Aug 14, 2023
kevintab95 pushed a commit that referenced this pull request Aug 14, 2023
* Strengthen CSRF Protections

Adds the following protections:
* A nonce to prevent collisions. This is best practice per OWASP.
* A constant-time comparison of expected and received tokens.
* HMAC-SHA256 instead of HMAC-MD5. Even though HMAC-MD5 is not broken
  yet, attacks against MD5 have been getting better, and there's no
  reason not to switch to SHA256.

* Fix base controller tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants