Page MenuHomePhabricator

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

Authored by epriestley on Jan 3 2019, 2:08 PM.



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

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 3 2019, 2:08 PM
epriestley requested review of this revision.Jan 3 2019, 2:10 PM
amckinley accepted this revision.Jan 3 2019, 8:18 PM
This revision is now accepted and ready to land.Jan 3 2019, 8:18 PM
This revision was automatically updated to reflect the committed changes.