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
F13377310: D8079.diff
Sat, Jun 29, 3:45 AM
F13353666: D8079.diff
Sun, Jun 23, 10:47 PM
F13318627: D8079.diff
Thu, Jun 13, 11:19 AM
F13310764: D8079.id18281.diff
Mon, Jun 10, 6:23 PM
F13310763: D8079.id18278.diff
Mon, Jun 10, 6:23 PM
F13310721: D8079.id.diff
Mon, Jun 10, 5:38 PM
F13310704: D8079.diff
Mon, Jun 10, 5:27 PM
F13280740: D8079.diff
Jun 2 2024, 9:14 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

Lint
Lint Skipped
Unit
Tests Skipped

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.