Skip to content

Make releaseAllGrades, withdrawAllGrades, etc. POST to prevent CSRF attacks#1512

Merged
20wildmanj merged 3 commits intomasterfrom
csrf-fix
Apr 30, 2022
Merged

Make releaseAllGrades, withdrawAllGrades, etc. POST to prevent CSRF attacks#1512
20wildmanj merged 3 commits intomasterfrom
csrf-fix

Conversation

@20wildmanj
Copy link
Copy Markdown
Contributor

This PR aims to fix the CSRF vulnerability for 'releaseAllGrades' along with other vulnerable endpoints that previously used GET requests instead of POST.

Thanks to Damien and Fan Pu for the discussion about the security flaw and the useful article here about CSRF and rails:
https://guides.rubyonrails.org/security.html#csrf-countermeasures

Description

  • change releaseAllGrades, withdrawAllGrades, and releaseSectionGrades in the
    assessment page to be POST requests, from GET requests.
  • turn link_to in the frontend for these actions to be forms with button_to
  • changed styling of input classes for danger-zone to look like links, to match
    the style of the other actions on the page

Motivation and Context

We were sent this issue by huntr.dev, where a contributor pointed out this flaw.

How Has This Been Tested?

Tested mainly through local host, seeing if we can directly access http://localhost:3000/courses/AutoPopulated/assessments/homework0/releaseAllGrades from the browser, which should no longer work (should get an error saying there doesn't exist a GET request with that endpoint).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

Other issues / help required

@damianhxy

@victorhuangwq victorhuangwq requested a review from damianhxy April 28, 2022 20:21
@victorhuangwq
Copy link
Copy Markdown
Contributor

I'm assuming that you have tested to ensure that releaseAllGrades and withdrawAllGrades still work?

Copy link
Copy Markdown
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

Checked that GET routes no longer exist, and POST routes work as expected.

However, it seems that CSRF protection isn't working -- even after deleting the authenticity_token fields in a form (or filling them with garbage) and submitting, the request still succeeds. This needs further investigation

@20wildmanj
Copy link
Copy Markdown
Contributor Author

20wildmanj commented Apr 29, 2022

I'm assuming that you have tested to ensure that releaseAllGrades and withdrawAllGrades still work?

Both methods still work, has been tested.

Checked that GET routes no longer exist, and POST routes work as expected.

However, it seems that CSRF protection isn't working -- even after deleting the authenticity_token fields in a form (or filling them with garbage) and submitting, the request still succeeds. This needs further investigation

I can confirm that CSRF doesn't seem to be working as intended. I first tried using postman to send a POST request, which actually failed (didn't release grades), but by making just an html file with a form and using that to send a POST request, I was able to release all grades without being on autolab. This definitely seems to be a bit more tricky than originally thought.

@20wildmanj
Copy link
Copy Markdown
Contributor Author

20wildmanj commented Apr 29, 2022

I've added protect_from_forgery to each of the methods (releaseAllGrades etc.) and it now seems that if I try to make a POST request outside of Autolab it should fail:
Screen Shot 2022-04-29 at 0 53 36

But also I'm not sure if this is ideal if it gives out an error like this?

EDIT: After restarting the server it seems to give a nicer error message:
Screen Shot 2022-04-29 at 1 00 39

@damianhxy
Copy link
Copy Markdown
Member

I've added protect_from_forgery to each of the methods (releaseAllGrades etc.) and it now seems that if I try to make a POST request outside of Autolab it should fail: Screen Shot 2022-04-29 at 0 53 36

But also I'm not sure if this is ideal if it gives out an error like this?

EDIT: After restarting the server it seems to give a nicer error message: Screen Shot 2022-04-29 at 1 00 39

Confirmed that this fixes the issue. However, it seems weird that protect_from_forgery has to be repeated so many times, and there might be some other underlying issue.

Error message shouldn't appear in prod, so that's fine

action_auth_level :set_repo, :instructor
action_auth_level :import_svn, :instructor

protect_from_forgery with: :exception
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would be good to figure out why protect_from_forgery inside application_controller.rb isn't sufficient

Copy link
Copy Markdown
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

LGTM!

@20wildmanj 20wildmanj merged commit a5fa31e into master Apr 30, 2022
@20wildmanj 20wildmanj deleted the csrf-fix branch April 30, 2022 20:47
@damianhxy damianhxy mentioned this pull request May 14, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants