Upgrade sessions digests to HMAC256, retaining compatibility with old digests
ClosedPublic
Actions

Authored by epriestley on Dec 13 2018, 7:31 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13222: 2018 Week 48-51 Bonus Content
T13225: Complete session digest migration from SHA1 to SHA256

Commits

rP1d34238dc945: Upgrade sessions digests to HMAC256, retaining compatibility with old digests

Summary

Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.

Users with access to the database can probably do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.

We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:

Always write new digests.
We still match sessions with either digest.
When we read a session with an old digest, upgrade it to a new digest.

In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.

We could avoid this by storing hmac256(hmac1(key)) instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.

Test Plan

Hit a page with an old session, got a session upgrade.
Reviewed sessions in Settings.
Reviewed user logs.
Logged out.
Logged in.
Terminated other sessions individually.
Terminated all other sessions.
Spot checked session table for general sanity.

Diff Detail

Repository

rP Phabricator

Branch

mfa3

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/auth/engine/PhabricatorAuthSessionEngine.php:114	XHP16	TODO Comment
Advice	src/applications/auth/engine/PhabricatorAuthSessionEngine.php:151	XHP16	TODO Comment
Advice	src/applications/auth/engine/PhabricatorAuthSessionEngine.php:217	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 21330
Build 29026: Run Core Tests
Build 29025: arc lint + arc unit

Event Timeline

epriestley created this revision.Dec 13 2018, 7:31 PM

Owners added a subscriber: Restricted Owners Package.Dec 13 2018, 7:31 PM

Harbormaster completed remote builds in B21330: Diff 47479.Dec 13 2018, 7:33 PM

epriestley requested review of this revision.Dec 13 2018, 7:33 PM

Accepted assuming my inlines aren't actual issues.

src/applications/auth/engine/PhabricatorAuthSessionEngine.php
143	I don't think this needs a `PhutilOpaqueEnvelope` anymore (especially since we put it inside one before the call to `newSessionDigest` above.
225	See above comment; this is another nesting of `PhutilOpaqueEnvelope`.
src/applications/auth/query/PhabricatorAuthSessionQuery.php
94	Shouldn't this also search for the the original weak-digest as well?

This revision is now accepted and ready to land.Dec 13 2018, 8:56 PM

epriestley added inline comments.Dec 13 2018, 9:03 PM

src/applications/auth/engine/PhabricatorAuthSessionEngine.php
143	We put `session_token` (undigested) in the envelope but not `session_key` (digested) so I think this works as we'd hope. (Possibly `newSessionDigest()` should return an Envelope but maybe I'll clean that up when I remove this code.)
src/applications/auth/query/PhabricatorAuthSessionQuery.php
94	In practice, there's no way to hit this query without upgrading your session first, and we only use this to "look for your current session" today (and only in one place, I think). In the general case, yes, this is sort of a compatibility break, and third-party code doing super weird stuff might have some kind of issue, but it probably shouldn't be doing super weird stuff in the first place (and I'm not aware of any doing any of this kind of thing). I'll call this out in the changelog. You could `bin/auth revoke --type session` to nuke all sessions if you did have some kind of odd third-party thing. (It would be easy to continue searching for the `weakDigest()` too, but since all the other "delete me" code was in one place I wasn't confident I'd remember to get rid of it here.)