1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-13 18:32:41 +01:00
Commit graph

312 commits

Author SHA1 Message Date
epriestley
99c72a32d0 Allow installs to require multi-factor authentication for all users
Summary: Ref T5089. Adds a `security.require-multi-factor-auth` which forces all users to enroll in MFA before they can use their accounts.

Test Plan:
Config:

{F159750}

Roadblock:

{F159748}

After configuration:

{F159749}

  - Required MFA, got roadblocked, added MFA, got unblocked.
  - Removed MFA, got blocked again.
  - Used `bin/auth strip` to strip MFA, got blocked.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5089

Differential Revision: https://secure.phabricator.com/D9285
2014-06-03 16:50:27 -07:00
epriestley
83112cc2e8 Move email verification into PhabricatorUserEditor
Summary: Both email verify and welcome links now verify email, centralize them and record them in the user activity log.

Test Plan:
  - Followed a "verify email" link and got verified.
  - Followed a "welcome" (verifying) link.
  - Followed a "reset" (non-verifying) link.
  - Looked in the activity log for the verifications.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9284
2014-06-03 16:45:18 -07:00
epriestley
6df1a02413 (Redesign) Clean up older "Tile" code
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:

  - Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
  - Old tile size methods are replaced with `isPinnnedByDefault()`.
  - Shortened some short descriptions.
  - `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
  - Added a marker for third-party / extension applications.

Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9358
2014-06-03 15:47:27 -07:00
epriestley
81d95cf682 Make default view of "Applications" app a full-page launcher
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.

Open to feedback.

Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)

{F160052}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5176

Differential Revision: https://secure.phabricator.com/D9297
2014-05-29 12:17:54 -07:00
epriestley
f1534e6feb Make password reset emails use one-time tokens
Summary:
Ref T4398. This code hadn't been touched in a while and had a few crufty bits.

**One Time Resets**: Currently, password reset (and similar links) are valid for about 48 hours, but we always use one token to generate them (it's bound to the account). This isn't horrible, but it could be better, and it produces a lot of false positives on HackerOne.

Instead, use TemporaryTokens to make each link one-time only and good for no more than 24 hours.

**Coupling of Email Verification and One-Time Login**: Currently, one-time login links ("password reset links") are tightly bound to an email address, and using a link verifies that email address.

This is convenient for "Welcome" emails, so the user doesn't need to go through two rounds of checking email in order to login, then very their email, then actually get access to Phabricator.

However, for other types of these links (like those generated by `bin/auth recover`) there's no need to do any email verification.

Instead, make the email verification part optional, and use it on welcome links but not other types of links.

**Message Customization**: These links can come out of several workflows: welcome, password reset, username change, or `bin/auth recover`. Add a hint to the URI so the text on the page can be customized a bit to help users through the workflow.

**Reset Emails Going to Main Account Email**: Previously, we would send password reset email to the user's primary account email. However, since we verify email coming from reset links this isn't correct and could allow a user to verify an email without actually controlling it.

Since the user needs a real account in the first place this does not seem useful on its own, but might be a component in some other attack. The user might also no longer have access to their primary account, in which case this wouldn't be wrong, but would not be very useful.

Mitigate this in two ways:

  - First, send to the actual email address the user entered, not the primary account email address.
  - Second, don't let these links verify emails: they're just login links. This primarily makes it more difficult for an attacker to add someone else's email to their account, send them a reset link, get them to login and implicitly verify the email by not reading very carefully, and then figure out something interesting to do (there's currently no followup attack here, but allowing this does seem undesirable).

**Password Reset Without Old Password**: After a user logs in via email, we send them to the password settings panel (if passwords are enabled) with a code that lets them set a new password without knowing the old one.

Previously, this code was static and based on the email address. Instead, issue a one-time code.

**Jump Into Hisec**: Normally, when a user who has multi-factor auth on their account logs in, we prompt them for factors but don't put them in high security. You usually don't want to go do high-security stuff immediately after login, and it would be confusing and annoying if normal logins gave you a "YOU ARE IN HIGH SECURITY" alert bubble.

However, if we're taking you to the password reset screen, we //do// want to put the user in high security, since that screen requires high security. If we don't do this, the user gets two factor prompts in a row.

To accomplish this, we set a cookie when we know we're sending the user into a high security workflow. This cookie makes login finalization upgrade all the way from "partial" to "high security", instead of stopping halfway at "normal". This is safe because the user has just passed a factor check; the only reason we don't normally do this is to reduce annoyance.

**Some UI Cleanup**: Some of this was using really old UI. Modernize it a bit.

Test Plan:
  - **One Time Resets**
    - Used a reset link.
    - Tried to reuse a reset link, got denied.
    - Verified each link is different.
  - **Coupling of Email Verification and One-Time Login**
    - Verified that `bin/auth`, password reset, and username change links do not have an email verifying URI component.
    - Tried to tack one on, got denied.
    - Used the welcome email link to login + verify.
    - Tried to mutate the URI to not verify, or verify something else: got denied.
  - **Message Customization**
    - Viewed messages on the different workflows. They seemed OK.
  - **Reset Emails Going to Main Account Email**
    - Sent password reset email to non-primary email.
    - Received email at specified address.
    - Verified it does not verify the address.
  - **Password Reset Without Old Password**
    - Reset password without knowledge of old one after email reset.
    - Tried to do that without a key, got denied.
    - Tried to reuse a key, got denied.
  - **Jump Into Hisec**
    - Logged in with MFA user, got factor'd, jumped directly into hisec.
    - Logged in with non-MFA user, no factors, normal password reset.
  - **Some UI Cleanup**
    - Viewed new UI.
  - **Misc**
    - Created accounts, logged in with welcome link, got verified.
    - Changed a username, used link to log back in.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D9252
2014-05-22 10:41:00 -07:00
epriestley
cac61980f9 Add "temporary tokens" to auth, for SMS codes, TOTP codes, reset codes, etc
Summary:
Ref T4398. We have several auth-related systems which require (or are improved by) the ability to hand out one-time codes which expire after a short period of time.

In particular, these are:

  - SMS multi-factor: we need to be able to hand out one-time codes for this in order to prove the user has the phone.
  - Password reset emails: we use a time-based rotating token right now, but we could improve this with a one-time token, so once you reset your password the link is dead.
  - TOTP auth: we don't need to verify/invalidate keys, but can improve security by doing so.

This adds a generic one-time code storage table, and strengthens the TOTP enrollment process by using it. Specifically, you can no longer edit the enrollment form (the one with a QR code) to force your own key as the TOTP key: only keys Phabricator generated are accepted. This has no practical security impact, but generally helps raise the barrier potential attackers face.

Followup changes will use this for reset emails, then implement SMS multi-factor.

Test Plan:
  - Enrolled in TOTP multi-factor auth.
  - Submitted an error in the form, saw the same key presented.
  - Edited the form with web tools to provide a different key, saw it reject and the server generate an alternate.
  - Change the expiration to 5 seconds instead of 1 hour, submitted the form over and over again, saw it cycle the key after 5 seconds.
  - Looked at the database and saw the tokens I expected.
  - Ran the GC and saw all the 5-second expiry tokens get cleaned up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D9217
2014-05-20 11:43:45 -07:00
Tal Shiri
43d45c4956 can now tell phabricator you trust an auth provider's emails (useful for Google OAuth), which will mark emails as "verified" and will skip email verification.
Summary: This is useful when you're trying to onboard an entire office and you end up using the Google OAuth anyway.

Test Plan: tested locally. Maybe I should write some tests?

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9150
2014-05-16 14:14:06 -07:00
Chad Little
0120388a75 Found some missing icons
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.

Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9088
2014-05-13 07:45:39 -07:00
Chad Little
b2f3001ec4 Replace Sprite-Icons with FontAwesome
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.

Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9052
2014-05-12 10:08:32 -07:00
lkassianik
dfcccd4cb8 Add config to require real name, respect config when creating new users, drop real name from full name if not provided.
Summary: Fixes T4728, first pass, Make real name optional on user accounts

Test Plan: Default real name config should be false (not required). Create new user, real name should not be required. Toggle config, real name should be required. Users with no real name should be always listed by their usernames.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4728

Differential Revision: https://secure.phabricator.com/D9027
2014-05-12 09:51:41 -07:00
Ashish SHUKLA
0da22abdb5 WordPress.com OAuth2 plugin
Summary:
This plugin provides an OAuth authentication provider to authenticate users using WordPress.com Connect.

This diff corresponds to github pull request https://github.com/facebook/phabricator/pull/593/ and had its libphutil counterpart reviewed in D9004.

Test Plan: Configured WordPress.com as an authentication provider, saw it show up on the login screen, registered a new account, got expected defaults for my username/name/email/profile picture.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9019
2014-05-08 14:23:19 -07:00
epriestley
a04e138ae2 Minor cleanup of some session code
Summary: Ref T4398. Add some documentation and use `phutil_units()`.

Test Plan:
  - Established a web session.
  - Established a conduit session.
  - Entered and exited hisec.
  - Used "Sessions" panel to examine results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8924
2014-05-01 10:23:19 -07:00
epriestley
e146958217 Generate QR codes for TOTP tokens
Summary: Ref T4398. I found a reasonable-ish LGPLv3 library for doing this, which isn't too huge or unwieldy.

Test Plan:
  - Scanned QR code with Authy.
  - Scanned QR code with Google Authenticator.

{F149317}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8923
2014-05-01 10:23:11 -07:00
epriestley
50376aad04 Require multiple auth factors to establish web sessions
Summary:
Ref T4398. This prompts users for multi-factor auth on login.

Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.

Test Plan:
  - Used Conduit.
  - Logged in as multi-factor user.
  - Logged in as no-factor user.
  - Tried to do non-login-things with a partial session.
  - Reviewed account activity logs.

{F149295}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8922
2014-05-01 10:23:02 -07:00
epriestley
e8cebb7da5 Add support for aural-only and visual-only elements
Summary:
Ref T4843. This adds support to `javelin_tag()` for an `aural` attribute. When specified, `true` values mean "this content is aural-only", while `false` values mean "this content is not aural".

  - I've attempted to find the best modern approaches for marking this content, but the `aural` attribute should let us change the mechanism later.
  - Make the "beta" markers on application navigation visual only (see T4843). This information is of very low importance, the application navigation is accessed frequently, and the information is available on the application list.
  - Partially convert the main navigation. This is mostly to test things, since I want to get more concrete feedback about approaches here.
  - Add a `?__aural__=1` attribute, which renders the page with aural-only elements visible and visual-only elements colored.

Test Plan: {F146476}

Reviewers: btrahan, scp, chad

Reviewed By: chad

Subscribers: aklapper, qgil, epriestley

Maniphest Tasks: T4843

Differential Revision: https://secure.phabricator.com/D8830
2014-05-01 07:18:18 -07:00
epriestley
3fde020049 Make many actions require high security
Summary:
Ref T4398. Protects these actions behind a security barrier:

  - Link external account.
  - Retrieve Conduit token.
  - Reveal Passphrase credential.
  - Create user.
  - Admin/de-admin user.
  - Rename user.
  - Show conduit certificate.
  - Make primary email.
  - Change password.
  - Change VCS password.
  - Add SSH key.
  - Generate SSH key.

Test Plan: Tried to take each action and was prompted for two-factor.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8921
2014-04-30 17:44:59 -07:00
epriestley
23e654ec2b Rate limit multi-factor actions
Summary: Ref T4398. Prevent users from brute forcing multi-factor auth by rate limiting attempts. This slightly refines the rate limiting to allow callers to check for a rate limit without adding points, and gives users credit for successfully completing an auth workflow.

Test Plan: Tried to enter hisec with bad credentials 11 times in a row, got rate limited.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8911
2014-04-30 14:30:31 -07:00
epriestley
535cfa3ebe Add bin/auth list-factors and bin/auth strip to remove multi-factor auth
Summary:
Ref T4398. The major goals here is to let administrators strip auth factors in two cases:

  - A user lost their phone and needs access restored to their account; or
  - an install previously used an API-based factor like SMS, but want to stop supporting it (this isn't possible today).

Test Plan:
  - Used `bin/auth list-factors` to show installed factors.
  - Used `bin/auth strip` with various mixtures of flags to selectively choose and strip factors from accounts.
  - Also ran `bin/auth refresh` to verify refreshing OAuth tokens works (small `OAuth` vs `OAuth2` tweak).

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8909
2014-04-30 14:30:00 -07:00
epriestley
a017a8e02b Make two-factor auth actually work
Summary:
Ref T4398. Allows auth factors to render and validate when prompted to take a hi-sec action.

This has a whole lot of rough edges still (see D8875) but does fundamentally work correctly.

Test Plan:
  - Added two different TOTP factors to my account for EXTRA SECURITY.
  - Took hisec actions with no auth factors, and with attached auth factors.
  - Hit all the error/failure states of the hisec entry process.
  - Verified hisec failures appear in activity logs.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8886
2014-04-28 10:20:54 -07:00
epriestley
17709bc167 Add multi-factor auth and TOTP support
Summary:
Ref T4398. This is still pretty rough and isn't exposed in the UI yet, but basically works. Some missing features / areas for improvement:

  - Rate limiting attempts (see TODO).
  - Marking tokens used after they're used once (see TODO), maybe. I can't think of ways an attacker could capture a token without also capturing a session, offhand.
  - Actually turning this on (see TODO).
  - This workflow is pretty wordy. It would be nice to calm it down a bit.
  - But also add more help/context to help users figure out what's going on here, I think it's not very obvious if you don't already know what "TOTP" is.
  - Add admin tool to strip auth factors off an account ("Help, I lost my phone and can't log in!").
  - Add admin tool to show users who don't have multi-factor auth? (so you can pester them)
  - Generate QR codes to make the transfer process easier (they're fairly complicated).
  - Make the "entering hi-sec" workflow actually check for auth factors and use them correctly.
  - Turn this on so users can use it.
  - Adding SMS as an option would be nice eventually.
  - Adding "password" as an option, maybe? TOTP feels fairly good to me.

I'll post a couple of screens...

Test Plan:
  - Added TOTP token with Google Authenticator.
  - Added TOTP token with Authy.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8875
2014-04-28 09:27:11 -07:00
epriestley
3f5a55fa6e Let users review their own account activity logs
Summary:
Ref T4398. This adds a settings panel for account activity so users can review activity on their own account. Some goals are:

  - Make it easier for us to develop and support auth and credential information, see T4398. This is the primary driver.
  - Make it easier for users to understand and review auth and credential information (see T4842 for an example -- this isn't there yet, but builds toward it).
  - Improve user confidence in security by making logging more apparent and accessible.

Minor corresponding changes:

  - Entering and exiting hisec mode is now logged.
  - This, sessions, and OAuth authorizations have moved to a new "Sessions and Logs" area, since "Authentication" was getting huge.

Test Plan:
  - Viewed new panel.
  - Viewed old UI.
  - Entered/exited hisec and got prompted.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8871
2014-04-27 17:32:09 -07:00
epriestley
f42ec84d0c Add "High Security" mode to support multi-factor auth
Summary:
Ref T4398. This is roughly a "sudo" mode, like GitHub has for accessing SSH keys, or Facebook has for managing credit cards. GitHub actually calls theirs "sudo" mode, but I think that's too technical for big parts of our audience. I've gone with "high security mode".

This doesn't actually get exposed in the UI yet (and we don't have any meaningful auth factors to prompt the user for) but the workflow works overall. I'll go through it in a comment, since I need to arrange some screenshots.

Test Plan: See guided walkthrough.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8851
2014-04-27 17:31:11 -07:00
Chad Little
11fd6afeb1 Move Timeline icons to Fonts
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.

Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8827
2014-04-22 08:25:54 -07:00
epriestley
499f66963d Minor, fix the scoping of a static variable
Auditors: chad, btrahan
2014-04-11 04:24:58 -07:00
Bob Trahan
2d43cf1296 OAuth - add a little notes section for admins to remember details about external accounts
Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.

Test Plan: looked at providers and read pretty instructions.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4755

Differential Revision: https://secure.phabricator.com/D8726
2014-04-09 11:09:50 -07:00
epriestley
582ec54465 Add a checkbox to the LDAP auth configuration UI to "Always Search"
Summary: Fixes T3208. This forces us to bind+search even if there are no anonymous credentials.

Test Plan: Checked the box, saved the form. Unchecked the box, saved the form. LDAP??

Reviewers: Firehed

Reviewed By: Firehed

Subscribers: epriestley

Maniphest Tasks: T3208

Differential Revision: https://secure.phabricator.com/D8723
2014-04-08 11:36:23 -07:00
epriestley
750c872839 Update Google auth documentation to discuss "Google+ API" and new console URI
Summary: Fixes T4451. See also D8612.

Test Plan: Viewed panel and read text, saw it matched up with the new console.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4451

Differential Revision: https://secure.phabricator.com/D8613
2014-03-25 13:36:47 -07:00
epriestley
d8713f6f0b Make dialogs a little easier to use
Summary:
  - Dialog pages currently have no titles or crumbs, and look shoddy. Add titles and crumbs.
  - Dialog titles aren't always great for crumbs, add an optional "short title" for crumbs.
  - `AphrontDialogResponse` is pure boilerplate. Allow controllers to just return a `DialogView` instead and get the same effect.
  - Building dialogs requires a bit of boilerplate, and we generally construct them with no explicit `"action"`, which has some issues with T4593. Provide a convenience method to set the viewer and get a reasonable, explict submit URI.

Test Plan:
  - Viewed dialog on its own.
  - Viewed dialog as a dialog.

{F132353}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8577
2014-03-21 14:40:05 -07:00
epriestley
ba8925a531 Support multiple LDAP filters in the Phabricator UI
Summary: Ref T3208. Not ready for prime time yet.

Test Plan: ldap T.T

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, frgtn, aran

Maniphest Tasks: T3208

Differential Revision: https://secure.phabricator.com/D8160
2014-03-17 15:08:30 -07:00
epriestley
aea624118b Allow users to terminate login sessions
Summary:
This is partly a good feature, and partly should reduce false positives on HackerOne reporting things vaguely related to this.

Allow a user to terminate login sessions from the settings panel.

Test Plan:
  - Terminated a session.
  - Terminated all sessions.
  - Tried to terminate all sessions again.
  - Logged in with two browsers, terminated the other browser's session, reloaded, got kicked out.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8556
2014-03-17 15:02:01 -07:00
epriestley
559c0fe886 Tune cookie behaviors for 'phcid', 'phreg', etc
Summary:
Fixes T3471. Specific issues:

  - Add the ability to set a temporary cookie (expires when the browser closes).
  - We overwrote 'phcid' on every page load. This creates some issues with browser extensions. Instead, only write it if isn't set. To counterbalance this, make it temporary.
  - Make the 'next_uri' cookie temporary.
  - Make the 'phreg' cookie temporary.
  - Fix an issue where deleted cookies would persist after 302 (?) in some cases (this is/was 100% for me locally).

Test Plan:
  - Closed my browser, reopned it, verified temporary cookies were gone.
  - Logged in, authed, linked, logged out.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3471

Differential Revision: https://secure.phabricator.com/D8537
2014-03-14 14:33:31 -07:00
epriestley
f7b1ed7221 Fix two registration errors for unusual provider emails
Summary:
See <https://github.com/facebook/phabricator/issues/541>.

  - If a provider returns the email `""` or `"0"`, we currently don't let the user edit it and thus don't let them register.
  - If a provider returns an invalid email like `"!!!"` (permitted by GitHub, e.g.), we show them a nonsense error message.

Instead:

  - Pretend we didn't get an address if we get an invalid address.
  - Test the address strictly against `null`.

Test Plan: Registered on Phabricator with my GitHub email set to `""` (empty string) and `"!!!"` (bang bang bang).

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8528
2014-03-13 19:03:12 -07:00
epriestley
a9f38e55e5 Modernize Facebook OAuth instructions
Summary: Ref T4593. We have somewhat outdated instructions, and safer setup instructions are now available.

Test Plan: Read instructions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8518
2014-03-13 13:00:47 -07:00
epriestley
969d0c3e8d Use "\z" instead of "$" to anchor validating regular expressions
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.

When we care about this, use `\z` instead, which matches "end of input" only.

This allowed registration of `"username\n"` and similar.

Test Plan:
  - Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
  - Fixed the ones where this seemed like it could have an impact.
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8516
2014-03-13 12:42:41 -07:00
epriestley
7176240717 Whitelist controllers which can receive a 'code' parameter
Summary:
Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this.

We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away.

To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it.

This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time.

Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable.

Test Plan:
  - Logged in with OAuth.
  - Hit any other page with `?code=...` in the URL, got an exception.
  - Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8499
2014-03-12 11:30:04 -07:00
epriestley
e62c7321c2 Automatically verify the setup account's email address
Summary: Although the defaults don't require a verified email address, it's easy to lock yourself out by accident by configuring `auth.require-email-verification` or `auth.email-domains` before setting up email. Just force-verify the initial/setup account's address.

Test Plan: Went through setup on a fresh install, saw address verify.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8365
2014-02-27 15:16:04 -08:00
Joshua Spence
6270114767 Various linter fixes.
Summary:
- Removed trailing newlines.
- Added newline at EOF.
- Removed leading newlines.
- Trimmed trailing whitespace.
- Spelling fix.
- Added newline at EOF

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8344
2014-02-26 12:44:58 -08:00
epriestley
bcf255e9c9 Require CSRF submission to verify email addresses
Summary: If an attacker somehow intercepts a verification URL for an email address, they can hypothetically CSRF the account owner into verifying it. What you'd do before (how do you get the link?) and after (why do you care that you tricked them into verifying) performing this attack is unclear, but in theory we should require a CSRF submission here; add one.

Test Plan: {F118691}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8351
2014-02-26 11:17:46 -08:00
epriestley
14627ad65b Fix an incorrectly spelled call on the registration error pathway
Summary: If you copy the registration URL, then register, then load the URL again while logged out (i.e., attempt to reuse the registration URL), we try to show you a tailored error message. However, this call is not correct so we show you a not-so tailored exception instead.

Test Plan:
  - Get to the registration screen.
  - Save URL.
  - Complete registration.
  - Log out.
  - Return to saved URL.

Previously, exception. Now, readable error.

{F117585}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8322
2014-02-24 11:45:28 -08:00
epriestley
a566ae3730 Require a CSRF code for Twitter and JIRA (OAuth 1) logins
Summary:
OAuth1 doesn't have anything like the `state` parameter, and I overlooked that we need to shove one in there somewhere. Append it to the callback URI. This functions like `state` in OAuth2.

Without this, an attacker can trick a user into logging into Phabricator with an account the attacker controls.

Test Plan:
  - Logged in with JIRA.
  - Logged in with Twitter.
  - Logged in with Facebook (an OAuth2 provider).
  - Linked a Twitter account.
  - Linked a Facebook account.
  - Jiggered codes in URIs and verified that I got the exceptions I expected.

Reviewers: btrahan, arice

Reviewed By: arice

CC: arice, chad, aran

Differential Revision: https://secure.phabricator.com/D8318
2014-02-23 16:39:24 -08:00
epriestley
7cf0358dda Disallow email addresses which will overflow MySQL storage
Summary:
Via HackerOne. An attacker can bypass `auth.email-domains` by registering with an email like:

  aaaaa...aaaaa@evil.com@company.com

We'll validate the full string, then insert it into the database where it will be truncated, removing the `@company.com` part. Then we'll send an email to `@evil.com`.

Instead, reject email addresses which won't fit in the table.

`STRICT_ALL_TABLES` stops this attack, I'm going to add a setup warning encouraging it.

Test Plan:
  - Set `auth.email-domains` to `@company.com`.
  - Registered with `aaa...aaa@evil.com@company.com`. Previously this worked, now it is rejected.
  - Did a valid registration.
  - Tried to add `aaa...aaaa@evil.com@company.com` as an email address. Previously this worked, now it is rejected.
  - Did a valid email add.
  - Added and executed unit tests.

Reviewers: btrahan, arice

Reviewed By: arice

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D8308
2014-02-23 10:19:35 -08:00
epriestley
580bcd0d2b Implement bcrypt hasher, transparent login upgrade, and explicit upgrade for passwords
Summary:
Ref T4443.

  - Add a `password_hash()`-based bcrypt hasher if `password_hash()` is available.
  - When a user logs in using a password, upgrade their password to the strongest available hash format.
  - On the password settings page:
    - Warn the user if their password uses any algorithm other than the strongest one.
    - Show the algorithm the password uses.
    - Show the best available algorithm.

Test Plan: As an md5 user, viewed password settings page and saw a warning. Logged out. Logged in, got upgraded, no more warning. Changed password, verified database rehash. Logged out, logged in.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8270
2014-02-18 14:09:36 -08:00
epriestley
3c9153079f Make password hashing modular
Summary:
Ref T4443. Make hashing algorithms pluggable and extensible so we can deal with the attendant complexities more easily.

This moves "Iterated MD5" to a modular implementation, and adds a tiny bit of hack-glue so we don't need to migrate the DB in this patch. I'll migrate in the next patch, then add bcrypt.

Test Plan:
  - Verified that the same stuff gets stored in the DB (i.e., no functional changes):
    - Logged into an old password account.
    - Changed password.
    - Registered a new account.
    - Changed password.
    - Switched back to master.
    - Logged in / out, changed password.
    - Switched back, logged in.
  - Ran unit tests (they aren't super extensive, but cover some of the basics).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, kofalt

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8268
2014-02-18 14:09:36 -08:00
Chad Little
4743ad9649 Miniturize the nav buttons
Summary: This uses the slightly smaller icons. Not sure about the logout icon, will play with it more in the morning.

Test Plan: tested new nav on desktop and mobile.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8119
2014-01-31 09:10:32 -08:00
epriestley
049fb2018b Add very basic "quick create" menu
Summary:
Ref T3623. This is like a pre-v0, in that it doesn't have a dropdown yet.

Clicking the button takes you to a page which can serve as a right click / mobile / edit target in the long run, but is obviously not great for desktop use. I'll add the dropdown in the next iteration.

Test Plan: {F105631}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3623

Differential Revision: https://secure.phabricator.com/D8088
2014-01-28 20:18:01 -08:00
epriestley
152f05aebe Fix some security issues with email password resets
Summary:
Via HackerOne, there are two related low-severity issues with this workflow:

  - We don't check if you're already logged in, so an attacker can trick a victim (whether they're logged in or not) into clicking a reset link for an account the attacker controls (maybe via an invisible iframe) and log the user in under a different account.
  - We don't check CSRF tokens either, so after fixing the first thing, an attacker can still trick a //logged-out// victim in the same way.

It's not really clear that doing this opens up any significant attacks afterward, but both of these behaviors aren't good.

I'll probably land this for audit in a few hours if @btrahan doesn't have a chance to take a look at it since he's probably on a plane for most of the day, I'm pretty confident it doesn't break anything.

Test Plan:
  - As a logged-in user, clicked another user's password reset link and was not logged in.
  - As a logged-out user, clicked a password reset link and needed to submit a form to complete the workflow.

Reviewers: btrahan

CC: chad, btrahan, aran

Differential Revision: https://secure.phabricator.com/D8079
2014-01-27 16:53:04 -08:00
epriestley
febc494737 Actually check CSRF on Password and LDAP forms
Summary: Ref T4339. We didn't previously check `isFormPost()` on these, but now should.

Test Plan: Changed csrf token on login, got kicked out.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8051
2014-01-23 14:18:26 -08:00
epriestley
5b1d9c935a After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:

  - User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
  - User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).

To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.

The comment in the body text describes this in more detail.

This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.

Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3793

Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 14:16:08 -08:00
epriestley
f9ac534f25 Support CSRF for logged-out users
Summary: Fixes T4339. If you're anonymous, we use a digest of your session key to generate a CSRF token. Otherwise, everything works normally.

Test Plan: Logged out, logged in, tweaked CSRF in forms -- I'll add some inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8046
2014-01-23 14:03:54 -08:00
epriestley
24544b1a2f Straighten out absolute/relative URIs in login providers
Summary:
Ref T4339. Login providers use absolute URIs, but the ones that rely on local form submits should not, because we want to include CSRF tokens where applicable.

Instead, make the default be relative URIs and turn them into absolute ones for the callback proivders.

Test Plan: Clicked, like, every login button.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8045
2014-01-23 14:03:44 -08:00