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
Unknown Object (File)
Wed, Apr 17, 3:14 PM
Unknown Object (File)
Thu, Apr 11, 7:22 AM
Unknown Object (File)
Sat, Mar 30, 7:17 AM
Unknown Object (File)
Fri, Mar 22, 9:56 PM
Unknown Object (File)
Fri, Mar 22, 9:56 PM
Unknown Object (File)
Fri, Mar 22, 9:56 PM
Unknown Object (File)
Thu, Mar 21, 12:00 PM
Unknown Object (File)
Thu, Mar 21, 3:54 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable