Page MenuHomePhabricator

Expand MFA checks to all email operations
Closed, ResolvedPublic

Description

Via HackerOne. We currently MFA on making an email address primary, but don't on most other address operations.

It isn't technically necessary to MFA users on these other operations (at least today, these operations do not directly support any meaningful attacks, since non-primary addresses do very little) but they're generally reasonable to MFA. I believe I declined to MFA them originally because I was worried about fatiguing users with MFA prompts, but we haven't seen feedback to this effect (i.e., users don't seem to find MFA prompts burdensome in practice) and these operations are rare.

This primarily just aligns MFA behavior with user and researcher expectations, and hardens these workflows against possible future bugs or errors which might make them more dangerous than they are today.

Event Timeline

In Settings(Personal Account Settings)Email Addresses, you can take several actions with email addresses associated with your account, including adding new addresses.

If you configure MFA (SettingsMulti-Factor Auth), you'll be prompted to enter your MFA code when making an email address primary, but not when adding or removing an address.

Although there is technically no attack here today (non-primary addresses are not used for anything meaningful), protecting these actions better aligns with user and "security researcher" expectations, generally seems reasonable, may plausibly defuse some future attack, and is unlikely to cause any harm (we haven't seen users express concern that they feel like they're getting prompted for MFA too often, and MFA doesn't subjectively feel too intrusive today, and these actions are very rare).

This code all lives in PhabricatorEmailAddressesSettingsPanel.php. You can add an MFA check with this snippet, present in returnPrimaryAddressResponse():

$token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
  $viewer,
  $request,
  $this->getPanelURI());

Specifically:

  • Copy the check into returnNewAddressResponse() and returnDeleteAddressResponse() in the same file.
  • (Optionally, move it to a private method and call it from all three places, although the amount of code duplication is so tiny that this isn't necessarily too valuable.)

To verify this:

  • With MFA enabled on your account, try to add an email address. You should get prompted to enter your MFA token.
  • Leave high security, then try to remove an address. Same deal.