Page MenuHomePhabricator

Implement SMS MFA

Authored by epriestley on Jan 23 2019, 6:40 PM.



Depends on D20021. Ref T13222. This has a few rough edges, including:

  • The challenges theselves are CSRF-able.
  • You can go disable/edit your contact number after setting up SMS MFA and lock yourself out of your account.
  • SMS doesn't require MFA so an attacker can just swap your number to their number.

...but mostly works.

Test Plan
  • Added SMS MFA to my account.
  • Typed in the number I was texted.
  • Typed in some other different numbers (didn't work).
  • Cancelled/resumed the workflow, used SMS in conjunction with other factors, tried old codes, etc.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 23 2019, 6:40 PM
epriestley requested review of this revision.Jan 23 2019, 6:42 PM
epriestley added inline comments.Jan 23 2019, 6:42 PM

(Unrelated bugfix.)

amckinley accepted this revision.Jan 23 2019, 7:46 PM
amckinley added inline comments.

FWIW I usually see six-digit SMS codes in the wild, but I don't see anything wrong with using a bit more entropy.


Is there any way to inform the user if this daemon task fails, like if Twilio is unavailable or whatever?

This revision is now accepted and ready to land.Jan 23 2019, 7:46 PM
epriestley added inline comments.Jan 23 2019, 7:59 PM

That was pretty much my line of thought.

We could also do XXXX-YYYY or something to make them slightly easier to read/remember.


"Yes", but it's pretty complicated. We can save the message ID as part of the challenge and query for the message status, then change the UI to show "queued" vs "sent" vs "error".

However, when we render the form it will probably always be "queued" (usually, the daemons haven't picked it up and delivered it in the few milliseconds between the send and the form render), and users who don't get a message won't necessarily guess that submitting the form will give them more detail, so I think we'd need to do some javascript magic to live-update it without requiring user interaction. Seems like a lot of work until we have more evidence that SMS services routinely have long delays.

I think there's also no real next step for users, since knowing where in the queue things are gummed up doesn't really let them do anything other than keep waiting. Maybe slightly useful for administrators evaluating different SMS gateways, but probably an "average time spent in queue" readout somewhere would be more helpful for that. Also helps deflect blame away from us if Twilio or SNS or whatever are actually just awful, but Twilio seems pretty good for me locally, at least.

The one thing it would help with is "user entered a bogus number which Twilio won't deliver to", but I think the longer-term remedy for that is probably verification rather than making this flow try to cover things end-to-end. We could still get stuck in some cases like "you verified on Twilio, but SNS doesn't recognize the number", of course. None of that stuff has really cropped up for email too seriously, but maybe phone numbers are a wilder west.

Another attack we could take on this is to make bounces feed back to the user after T13115, so you get a notification whenever one of your address or contact numbers end up on the bounce list. That's a less surgical solution, but solves a wider set of problems.

amckinley added inline comments.Jan 23 2019, 8:03 PM

Yeah that makes sense. One change though is that we should probably send an MFA challenge when someone adds a contact number ins, just to make sure that end-to-end SMS delivery is working correctly. Arguably that would add friction in the future where we also make actual phone calls to users, but right now I don't think there's anything in scope for phones aside from sending SMS's.

Maybe just a "send a test sms" button, at least until we build verification? That's easy and seems fairly useful. My only hesitance around tool buildout here is that I want to avoid ending up with an abuse channel, and SMS feels relatively ripe (repeatedly add and remove the same number to spam it, send "test" messages with custom text that tell users to click, etc., etc., for a million other abuse pathways that maybe I haven't thought of).

We can do like a system-wide rate-limited test button with a static message pretty easily until verification gets built, though.

This revision was automatically updated to reflect the committed changes.