Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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
src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php
47

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.
84–89

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

src/applications/auth/engine/PhabricatorAuthSessionEngine.php
532–542

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.
562–566

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

src/applications/auth/factor/PhabricatorAuthFactor.php
235–238

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

445

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

488

Both Duo and SMS use this.

494–510

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

src/applications/auth/future/PhabricatorDuoFuture.php
115–118

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.
src/applications/auth/factor/PhabricatorAuthFactor.php
455–474

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
src/applications/auth/factor/PhabricatorAuthFactor.php
455–474

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.