Page MenuHomePhabricator

Fix some security issues with email password resets
ClosedPublic

Authored by epriestley on Jan 27 2014, 9:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 10:52 AM
Unknown Object (File)
Sat, Dec 21, 5:16 PM
Unknown Object (File)
Sat, Dec 21, 9:09 AM
Unknown Object (File)
Fri, Dec 13, 12:22 PM
Unknown Object (File)
Mon, Dec 9, 7:05 AM
Unknown Object (File)
Sun, Dec 1, 10:47 PM
Unknown Object (File)
Nov 21 2024, 8:34 AM
Unknown Object (File)
Nov 17 2024, 7:11 AM
Subscribers
Tokens
"Grey Medal" token, awarded by btrahan.

Details

Summary

Via HackerOne, there are two related low-severity issues with this workflow:

  • We don't check if you're already logged in, so an attacker can trick a victim (whether they're logged in or not) into clicking a reset link for an account the attacker controls (maybe via an invisible iframe) and log the user in under a different account.
  • We don't check CSRF tokens either, so after fixing the first thing, an attacker can still trick a logged-out victim in the same way.

It's not really clear that doing this opens up any significant attacks afterward, but both of these behaviors aren't good.

I'll probably land this for audit in a few hours if @btrahan doesn't have a chance to take a look at it since he's probably on a plane for most of the day, I'm pretty confident it doesn't break anything.

Test Plan
  • As a logged-in user, clicked another user's password reset link and was not logged in.
  • As a logged-out user, clicked a password reset link and needed to submit a form to complete the workflow.

Diff Detail

Repository
rP Phabricator
Branch
csrfemail
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/auth/controller/PhabricatorEmailTokenController.php:104XHP16TODO Comment
Unit
No Test Coverage

Event Timeline

Alright, I'm going to land this since it's security-related and I'm pretty confident that the change is correct / not dangerous.