Page MenuHomePhabricator

Upgrade an old "weakDigest()" inside TOTP synchronization code
ClosedPublic

Authored by epriestley on Dec 13 2018, 8:19 PM.
Tags
None
Referenced Files
F12826778: D19884.diff
Thu, Mar 28, 9:37 AM
Unknown Object (File)
Fri, Mar 22, 9:44 PM
Unknown Object (File)
Fri, Mar 22, 9:44 PM
Unknown Object (File)
Thu, Mar 21, 11:53 AM
Unknown Object (File)
Feb 3 2024, 8:16 PM
Unknown Object (File)
Jan 25 2024, 1:37 AM
Unknown Object (File)
Jan 6 2024, 12:43 PM
Unknown Object (File)
Dec 23 2023, 12:09 AM
Subscribers
None

Details

Summary

Ref T13222. Ref T12509. When you add a new MFA TOTP authenticator, we generate a temporary token to make sure you're actually adding the key we generated and not picking your own key.

That is, if we just put inputs in the form like key=123, response=456, users could pick their own keys by changing the value of key and then generating the correct response. That's probably fine, but maybe attackers could somehow force users to pick known keys in combination with other unknown vulnerabilities that might exist in the future. Instead, we generate a random key and keep track of it to make sure nothing funny is afoot.

As an additional barrier, we do the standard "store the digest, not the real key" sort of thing so you can't force a known value even if you can read the database (although this is mostly pointless since you can just read TOTP secrets directly if you can read the database). But it's pretty standard and doesn't hurt anything.

Update this from SHA1 to SHA256. This will break any TOTP factors which someone was in the middle of adding during a Phabricator upgrade, but that seems reasonable. They'll get a sensible failure mode.

Test Plan

Added a new TOTP factor.

Diff Detail

Repository
rP Phabricator
Branch
mfa5
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21331
Build 29028: Run Core Tests
Build 29027: arc lint + arc unit