Page MenuHomePhabricator

Remove "phabricator.csrf-key" and upgrade CSRF hashing to SHA256
ClosedPublic

Authored by epriestley on Jan 3 2019, 2:08 PM.
Tags
None
Referenced Files
F13226929: D19946.id.diff
Mon, May 20, 12:15 AM
F13224329: D19946.id47638.diff
Sun, May 19, 7:48 AM
F13194293: D19946.diff
Sun, May 12, 9:28 PM
F13184724: D19946.id47616.diff
Fri, May 10, 10:43 PM
F13177208: D19946.diff
Wed, May 8, 7:32 PM
Unknown Object (File)
Thu, May 2, 12:21 AM
Unknown Object (File)
Mon, Apr 29, 1:59 PM
Unknown Object (File)
Mon, Apr 29, 3:59 AM
Subscribers
None

Details

Summary

Ref T12509.

  • Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
  • Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
  • Extract the CSRF logic from PhabricatorUser to a standalone engine.

I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.

For example, it computed time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency instead of time_block = (epoch / cycle_frequency) + offset. These are equivalent but the former was kind of silly.

It also computed substr(hmac(substr(hmac(secret)).salt)) instead of substr(hmac(secret.salt)). These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).

This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.

Test Plan
  • As a logged-in user, submitted forms normally (worked).
  • As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
  • As a logged-out user, hit the success and error cases.
  • Visually inspected tokens for correct format.

Diff Detail

Repository
rP Phabricator
Branch
mfa6
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21470
Build 29242: Run Core Tests
Build 29241: arc lint + arc unit