Skip to content
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

Fix for https://www.pivotaltracker.com/story/show/163470469 #2237

Merged
merged 7 commits into from Jun 17, 2019

Conversation

2 participants
@ntwyman
Copy link
Contributor

commented Jun 6, 2019

Description

The bug report describes how the callback link from a session authenticated by a Bad Actor could be used in a phishing attack to have the victim upload their move data to another account. This fix adds checking on the original state nonce used to authenticate, to block the victims browser from using the Bad Actors session.

Adds code in /auth/login-gov to set a cookie containing the hashed value of the state param passed to login.gov.

Code in /auth/login-gov/callback then checks the state param sent back with the callback and makes sure the hash of that matches the cookie.

Reviewer Notes

I struggled to find a good unit-test for this code. Both handlers are so intimately tied to login.gov that I couldn't see an easy way to test this. That said, I verified that it failed in the case where the callback gets a different state from the one passed to login.gov.

Setup

No particular setup. You should run client and server and confirm you can still log in. If you want to check that the code works as expected, look in dev tools for the callback URL passed in when returning from login.gov, e.g. http://milmovelocal:3000/auth/login-gov/callback?code=2a9<...snip...>&state=C8vFgxI-sphn<...snip...>%3D

If you paste that unchanged into the browser you should see a normal session. If you edit the URL to change the state value passed back in you should see a server error and an ERROR in the log.

Code Review Verification Steps

  • Code follows the guidelines for Logging
    ** [ ] This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@ntwyman ntwyman changed the title WIP Fix for https://www.pivotaltracker.com/story/show/163470469 Fix for https://www.pivotaltracker.com/story/show/163470469 Jun 10, 2019

@ntwyman ntwyman requested a review from mikena-truss Jun 10, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

I have a request to make the cookie name similar to the session cookie name. But otherwise I think this looks fine.

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀

@codecov

This comment has been minimized.

Copy link

commented Jun 17, 2019

Codecov Report

Merging #2237 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2237      +/-   ##
==========================================
- Coverage   59.26%   59.12%   -0.14%     
==========================================
  Files         255      255              
  Lines       14412    14445      +33     
==========================================
  Hits         8540     8540              
- Misses       4858     4891      +33     
  Partials     1014     1014

@ntwyman ntwyman merged commit b696312 into master Jun 17, 2019

17 of 19 checks passed

codecov/patch 0% of diff hit (target 59.26%)
Details
codecov/project/go 58.95% (-0.14%) compared to 9e02276
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details

@ntwyman ntwyman deleted the nt-#163470469 branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.