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.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 5:50 PM
Unknown Object (File)
Sat, Mar 23, 5:50 PM
Unknown Object (File)
Sat, Mar 23, 4:50 PM
Unknown Object (File)
Thu, Mar 14, 8:04 PM
Unknown Object (File)
Feb 3 2024, 8:59 PM
Unknown Object (File)
Dec 22 2023, 1:22 AM
Unknown Object (File)
Dec 14 2023, 1:02 PM
Unknown Object (File)
Dec 14 2023, 6:56 AM
Subscribers
None

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
Branch
mfa22
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21683
Build 29572: Run Core Tests
Build 29571: arc lint + arc unit

Event Timeline

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 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
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.