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.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13225, T13222
Differential Revision: https://secure.phabricator.com/D19883
Summary:
Depends on D18904. Ref T13043. If an attacker compromises a victim's session and bypasses their MFA, they can try to guess the user's current account password by making repeated requests to change it: if they guess the right "Old Password", they get a different error than if they don't.
I don't think this is really a very serious concern (the attacker already got a session and MFA, if configured, somehow; many installs don't use passwords anyway) but we get occasional reports about it from HackerOne. Technically, it's better policy to rate limit it, and this should reduce the reports we receive.
Test Plan: Tried to change password over and over again, eventually got rated limited. Used `bin/auth unlimit` to clear the limit, changed password normally without issues.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18906
Summary:
Ref T13043. This moves user account passwords to the new shared infrastructure.
There's a lot of code changes here, but essentially all of it is the same as the VCS password logic in D18898.
Test Plan:
- Ran migration.
- Spot checked table for general sanity.
- Logged in with an existing password.
- Hit all error conditions on "change password", "set password", "register new account" flows.
- Verified that changing password logs out other sessions.
- Verified that revoked passwords of a different type can't be selected.
- Changed passwords a bunch.
- Verified that salt regenerates properly after password change.
- Tried to login with the wrong password, which didn't work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18903
Summary:
See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow.
Specifically, the flow goes like this today:
- User clicks the welcome link in email.
- They get redirected to the "set password" settings panel.
- This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact).
- This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a `/auth/multifactor/?id=123` sort of URI to highlight the factor they added. This causes us to lose the `/settings/panel/password/blah/blah?key=xyz` URI.
The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted.
Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall.
We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first.
This works better and is broadly simpler for users.
Test Plan:
- Required MFA + legalpad, invited a user via email, registered.
- Before: password set flow got lost when setting MFA.
- After: prompted to set password, then sign documents, then set up MFA.
- Reset password (with MFA confgiured, was required to MFA first).
- Tried to reset password without a valid reset key, wasn't successful.
- Changed password using existing flow.
- Hit various (all?) error cases (short password, common password, mismatch, missing password, etc).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18840
Summary: Updates settings panel UI for new white box, cleans up other various UI nitpicks.
Test Plan: Click through each setting that had a local setting page. Edit Engine pages will follow up on another diff.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18526
Summary: Moves Settings to use a normal side navigation vs. a two column side navigation. It also updates Edit Engine to do the same, but I don't think there are other callsites. Added a consistent header for better clarification if you were editng your settings, global settings, or a bot's settings.
Test Plan: Test each page on a personal account, create global settings, test each page there, create a bot account, and test each page on the bot account. Anything else?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18342
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T4103. Settings panels are grouped into categories of similar panels (like "Email" or "Sessions and Logs").
Currently, this is done informally, by just grouping and ordering by strings. This won't work well with translations, since it means the ordering is entirely dependent on the language order, so the first settings panel you see might be something irrelvant or confusing. We'd also potentially break third-party stuff by changing strings, but do so in a silent hard-to-detect way.
Provide formal objects and modularize the panel groups completely.
Test Plan: Verified all panels still appear properly and in the same groups and order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16020
Summary:
Ref T10603. This makes minor updates to temporary tokens:
- Rename `objectPHID` (which is sometimes used to store some other kind of identifier instead of a PHID) to `tokenResource` (i.e., which resource does this token permit access to?).
- Add a `userPHID` column. For LFS tokens and some other types of tokens, I want to bind the token to both a resource (like a repository) and a user.
- Add a `properties` column. This makes tokens more flexible and supports custom behavior (like scoping LFS tokens even more tightly).
Test Plan:
- Ran `bin/storage upgrade -f`, got a clean upgrade.
- Viewed one-time tokens.
- Revoked one token.
- Revoked all tokens.
- Performed a one-time login.
- Performed a password reset.
- Added an MFA token.
- Removed an MFA token.
- Used a file token to view a file.
- Verified file token was removed after viewing file.
- Linked my account to an OAuth1 account (Twitter).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15478
Summary:
Ref T10603. This converts existing hard-codes to modular constants.
Also removes one small piece of code duplication.
Test Plan:
- Performed one-time logins.
- Performed a password reset.
- Verified temporary tokens were revoked properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10603
Differential Revision: https://secure.phabricator.com/D15476
