Page MenuHomePhabricator

Allow applications to require a High Security token without doing a session upgrade
ClosedPublic

Authored by epriestley on Nov 27 2018, 1:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 1:30 AM
Unknown Object (File)
Feb 3 2024, 8:03 PM
Unknown Object (File)
Jan 8 2024, 3:49 AM
Unknown Object (File)
Dec 29 2023, 3:40 PM
Unknown Object (File)
Dec 28 2023, 3:23 PM
Unknown Object (File)
Dec 28 2023, 12:34 PM
Unknown Object (File)
Dec 26 2023, 7:38 PM
Unknown Object (File)
Dec 22 2023, 5:10 AM
Subscribers
None

Details

Summary

Ref T13222. See PHI873. Currently, when applications prompt users to enter MFA, their session upgrades as a side effect.

In some cases (like managing your email addresses) it makes sense to upgrade your session for a little while since it's common to make multiple edits in sequence (add a new address, make it primary, remove an old address). We generally want MFA to stay out of the way and not feel annoying.

In other cases, we don't expect multiple high-security actions in a row. Notably, PHI873 looks at more "one-shot" use cases where a prompt is answering a specific workflow. We already have at least one of these in the upstream: answering an MFA prompt when signing a Legalpad document.

Introduce a "token" workflow (in contrast to the existing "session") workflow that just does a one-shot prompt without upgrading your session statefully. Then, make Legalpad use this new workflow.

Note that this workflow has a significant problem: if the form submission is invalid for some other reason, we re-prompt you on resubmit. In Legalpad, this workflow looks like:

  • Forget to check the "I agree" checkbox.
  • Submit the form.
  • Get prompted for MFA.
  • Answer MFA prompt.
  • Get dumped back to the form with an error.
  • When you fix the error and submit again, you have to do another MFA check.

This isn't a fatal flaw in Legalpad, but would become a problem with wider adoption. I'll work on fixing this (so the MFA token sticks to the form) in the next set of changes.

Roughly, this is headed toward "MFA sticks to the form/workflow" instead of "MFA sticks to the user/session".

Test Plan
  • Signed a legalpad document with MFA enabled.
  • Was prompted for MFA.
  • Session no longer upgraded (no purple "session in high security" badge).
  • Submitted form with error, answered MFA, fixed error, submitted form again.
    • Bad behavior: got re-prompted for MFA. In the future, MFA should stick to the form.

Diff Detail

Repository
rP Phabricator
Branch
mfa1
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/auth/engine/PhabricatorAuthSessionEngine.php:422XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 21231
Build 28878: Run Core Tests
Build 28877: arc lint + arc unit

Event Timeline

epriestley added inline comments.
src/applications/legalpad/storage/LegalpadDocument.php
123–125

I couldn't find any callers to getViewURI() so I swapped this to the slightly more conventional getURI().

When you fix the error and submit again, you have to do another MFA check.

I can see this being kind of desirable. Use case:

  • Forget to fill out the required "Where do you see yourself in five years?" 1000px-tall free-text field.
  • Submit the form.
  • Get prompted for MFA.
  • Answer MFA prompt.
  • Get dumped back to the form with an error.
  • Walk away from your laptop in a rage.
  • Someone fills out the field on your unlocked laptop and submits it without needing to pass another MFA check.

Or alternately, just having the MFA token expire before you can correct the form. (Not sure if you were planning on tying the token itself to the form, or just a piece of state that says "this form submission has already been MFA'd").

This revision is now accepted and ready to land.Nov 28 2018, 7:09 PM

Ah, that's a good point.

I'm still working out the pieces in the next diff, but it currently looks like we're going to track responses to specific factors on a factor-by-factor basis, i.e. "This token proves I answered challenge X from factor Y at timestep T, so don't re-prompt me if the response I gave is still a valid response". The actual factor implementations (TOTP, SMS) get to choose if they're willing to accept the previous response or reject it as out-of-date or not reusable.

With TOTP, that should mean the form will automatically time out in ~45 seconds plus or minus a bit of clock skew, and you'll get another challenge if it takes longer than that to make edits. That behavior seems okay to me, but other cases like SMS might need have much longer default time windows (we might need to allow 5-10 minutes for SMS to arrive on phones, conceivably) and the "get up in the middle of a form" attack window could be larger if the code uses "time you can spend responding to the challenge" as the default duration of "how long a previously-answered challenge remains good for".

My initial thought is just to tweak the API so the "skip re-prompt" window is explicit, does not default to the duration of other parts of the challenge/response workflow, and has a short maximum duration (no more than a few minutes) -- so you don't get re-prompted if you just forgot to check a box, but always get re-prompted if you forgot to answer a required essay question.

A slightly more extreme approach might be to add a pop-up badge to these pages similar to the "you're in high-security" which says something more a long the lines of "this page is POWERED UP!!~" to warn you to lock your terminal if you get up for coffee. But I think this is probably unnecessary if the validity window is short enough. Since you can only end up in this state by making errors with form submission, tricking another user into unwittingly abandoning a "powered up" form is also probably hard, even if you're an evil coworker acting on behalf of a foreign state. You'd have to trick them into submitting the form with errors, answering MFA, then abandoning the machine without you, all without arousing suspicion. Maybe you do this:

  • Since the tokens aren't bound to particular forms/workflows, convince them to fill out any other (MFA-requiring) form in some erroneous way.
  • They answer MFA.
  • When the page comes back with an error, spill coffee all over them.
  • Say "gross, you smell like coffee, go clean yourself up" so they leave to go to the bathroom without locking their terminal.
  • Take the token from their erroneous form, move it to a different form, use it to Do Evil.
  • Spill a second cup of coffee all over their computer.
  • Say "oops I spilled coffee all over your computer I guess it doesn't work now".
  • Shrug innocently.

We could tighten this up slightly by binding tokens to particular workflows (so you can't take a "Legalpad Signature" token and reuse it elsewhere). This would mean that you have to trick them into erroneously filling out the exact same form you want to attack.

Popping a purple "this form is powered up" doesn't seem to do much to defuse this attack.

We could email you whenever you answer one-shot MFA but the attacker can probably delete the email before they nuke your machine.

Also probably reasonable:

  • We should generally fire MFA prompts as late in the process as possible, to make it maximally difficult to force an error. We can't prevent all errors (e.g., unique key collisions) but we currently prompt very early in the Legalpad case.
  • We could let users configure factors to never allow token reuse if they want to be paranoid about this stuff. Filling out the MFA dialog again isn't that big a deal, the problem is just that with TOTP, if we lock out responses ala T9770, the dialog isn't "type in another 6 digit number", it's "wait 15-45 seconds for a new challenge window, then type in another 6 digit number". Hitting this feels like it would be wildly annoying, and I'd like to at least have tools to avoid telling the user "sit there and wait for 30 seconds just in case an attacker is standing over you with a cup of coffee".

(I think this change is overall a net positive even on its own -- it makes Legalpad slightly more secure and only very very slightly more annoying -- so I'm going to land it even though the more useful followup parts aren't ready yet.)

This revision was automatically updated to reflect the committed changes.