Page MenuHomePhabricator

Add CSRF to SMS challenges, and pave the way for more MFA types (including Duo)

Authored by epriestley on Jan 24 2019, 10:21 PM.



Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.

Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.

Test Plan

Implemented Duo, elsewhere.

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 24 2019, 10:21 PM
epriestley requested review of this revision.Jan 24 2019, 10:23 PM
epriestley added inline comments.Jan 24 2019, 10:28 PM

This appears later on in greater depth, but "continue" responses are "click the button to continue".

Two cases for them:

  • You hit SMS/Duo/push without CSRF. We don't want to text you if you got <img src="..." />'d, so you have to click the button.
  • For pure push like Duo, you don't enter a code; instead, you click on your phone, then "Continue" in the UI.

I streamlined this text slightly and made the header less scary.


Factors may now produce a result directly from the challenge step. This happens when:

  • SMS/Duo, but you need CSRF.
  • Duo and your junk is 100% broken.
  • (Some future diff: SMS but an administrator un-configured SMS, or we somehow lost your contact number.)
  • Duo, and you clicked the "Approve" button recently.

Issuing challenges may now find an approval (Duo/push), so an early result doesn't necessarily mean a validity issue anymore.


For hard error results, like "ur duo is v. broken".


Duo needs QR so I lifted this up to the base class.


Both Duo and SMS use this.


Everything that requires you to type something can share this stuff.


Duo is particular about this. /auth_status is a GET request with parameters.

  • Add SMS error checking for "all SMS got un-configured so we can't send you messages" and "your contact number got lost somehow".
amckinley accepted this revision.Jan 24 2019, 11:03 PM
amckinley added inline comments.

I think it's kind of hilarious that we use phpqrcode to generate the bitmap and then use a huge <table> element to render it.

This revision is now accepted and ready to land.Jan 24 2019, 11:03 PM
epriestley added inline comments.Jan 24 2019, 11:07 PM

On the one hand this is completely ridicluous, but it doesn't require gd, doesn't have issues with data: URI length, doesn't require a separate <img /> request or a QR endpoint, scales well to different resolutions...

And a QR code is likely never going to have so much data that this completely fails, since phones still have to be able to resolve them with the camera.

This revision was automatically updated to reflect the committed changes.