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

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

Details

Reviewers

amckinley

Maniphest Tasks

T13222: 2018 Week 48-51 Bonus Content
T13231: Duo MFA Support

Commits

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

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

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jan 24 2019, 10:21 PM

Harbormaster completed remote builds in B21682: Diff 47829.Jan 24 2019, 10:23 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".

Harbormaster completed remote builds in B21683: Diff 47830.Jan 24 2019, 10:34 PM

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.