Page MenuHomePhabricator

Make "Send a login link to your email address" email include why it was sent to avoid confusion
Closed, ResolvedPublic

Description

Version information:
phabricator: 05afa15ce649ee208cf3c022c75aa85ae390eabc (Thu, Jul 4)
arcanist: f43c63ef5aaafaa8bf32ba4784d167a0448efd1a (Dec 20 2018)
phutil: 1ce011bc65687b2571c839bc1a0439cb1b0cdfd6 (Wed, Jul 10)

Steps to reproduce:

  1. Make someone else be not logged into Phabricator
  2. Make someone else go to login page at https://phabricator.example.com/auth/start/
  3. Make someone else click "Send a login link to your email address"
  4. Make someone else enter your email address.
  5. Receive an email:

Subject: [Phabricator] Account Login Link
You can use this login link to regain access to your Phabricator account:
https://phabricator.example.com/login/once/reset/xxx/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

Actual outcome:

  • Confusion

Expected outcome:

  • Include a sentence that you receive this message because someone requested a login link for Phabricator for your email address.
  • Include a sentence that you can ignore the message if you did not request this yourself.
  • Potentially include in that email the IP address of the machine that requested the login link.

Maybe something like:

Someone, probably you, from IP address xxx.xxx.xxx.xxx, has requested a login link for https://phabricator.example.com.
You can use this login link to regain access to your Phabricator account:
https://phabricator.example.com/login/once/reset/xxx/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
If you did not request a login link, ignore this email.

Sugar on top (feel very free to ignore, or move to separate tasks if this makes sense?):

  • Potentially don't allow the "Send a login link to your email address" action at all if the corresponding Phab account is already only linked to external accounts for authentication and the installation does not use passwords? But I might lack technical understanding here.
  • Potentially log this action under https://phabricator.example.com/people/logs/query/all/ to also allow admins to see the IP address used and identify patterns.

Event Timeline

aklapper updated the task description. (Show Details)Jul 17 2019, 10:36 PM

Potentially don't allow the "Send a login link to your email address" action at all if the corresponding Phab account is already only linked to external accounts for authentication and the installation does not use passwords? But I might lack technical understanding here.

If your external provider is, say, Google (or whatever), and it's down or broken, you can use this email flow to access your account as long as your email is working, so this is still legitimate at least some of the time.

If you enable ReCAPTCHA you also get a CAPTCHA on this flow, but there's currently no support for CAPTCHAs that don't harvest all your organs and send them to Google:

It would be nice to provide a first-party CAPTCHA instead, but I believe properly implementing a CAPTCHA is legitimately hard (e.g., accommodating blind users or other users who need accommodations for visual accessibility is particularly difficult).

It looks like we also don't have a hard rate limit on this flow, but can reasonably rate limit resets on a per-remote-address and per-email-address basis to a handful per hour to at least limit the potential for mischief.

This should also be added to the user activity log, as you note.

epriestley triaged this task as Normal priority.

Rough intentions here:

Mail Engine: Move this mail to PhabricatorPeopleMailEngine, mostly for consistency. I don't think there's a huge structural advantage to doing this today, it's just a little cleaner.

One issue is that PeopleMailEngine takes setRecipient(PhabricatorUser $user), and we want to send this mail to the exact (verified) address the user specifies, even if it is not their primary address. One reason for this is consistency with user expectations (they typed x@y.com into the box, and aren't expecting mail at primary@company.com). Another reason is to support the use case where Google/Gmail are down and you're intentionally using this flow to bypass Google OAuth + Gmail primary email. We can likely add setRecipient(PhabricatorUser $user, PhutilEmailAddress $explicit_address = null) to force delivery to a particular address.

Mail Messaging: The mail itself is pretty bare-bones, and providing an explanation and remedy ("ignore this") seem reasonable. We can turn this into an auth message now.

Rate Limiting: This is an older endpoint with no SystemAction rate limiting. We can fairly easily add limits to:

  • the rate at which a given remote address may request email logins; and
  • the rate at which a given email address may have email logins generated.

These can both be set fairly low, e.g. a handful per hour.

Currently, most callers to SystemAction pass PHIDs, but SystemAction explicitly supports non-user/non-PHID actors, and the test in PhabricatorMailEmailEngine uses email addresses. Here, we'd use the user remote address for the first check, and the target email address for the second check. It may be useful to provide helper methods to support normalizing these actor types (e.g., email addresses should be case-insensitive).

We have two older calls to PhabricatorUserLog::loadRecentEventsFromThisIP(...), which is a legacy rate limiter which predates SystemAction. It would be nice to update at least the one in PhabricatorPasswordAuthProvider, partly for consistency and partly because it's not obvious that setting the user.logs GC to a very low setting like "1 second" means both "do not retain logs" and also means "effectively, do not rate limit password login attempts from the same remote address".

User Activity Logs: We should include these login link requests in the user activity logs.

I would generally like to move toward a world where the activity logs serve a security-audit type of role (e.g., login failures, password resets) and administrative actions (make administrator, create account, disable/enable user, etc) are in the timeline on the user profile instead. Particularly, these actions probably should vanish from the activity log eventually:

Create Account
Edit Account
Add/Remove Administrator
Add/Remove System Agent
Add/Remove Mailing List
Enable/Disable
Approve Registration
Delete User
Change Username

Other than "Create Account", these are all cases where an administrator is clicking a button on the user's profile (or some similar approximation). These events are more important to preserve and less likely to be relevant in a security/audit context than things like login failures.

Many of these are already on user profiles and maybe we can take some steps here toward cleaning this up alongside adding the new event.

Including Remote Address Information in Email: I'm somewhat on the fence about this. I'm not sure it's actually useful to users -- what are they realistically going to do with the information? Sometimes this kind of email will include the UA string too, but this is likewise probably not terribly useful, and I can just imagine the HackerOne report where an enterprising "researcher" has "discovered" that they can inject content into this email by crafting a clever UA string. For sophisticated users the remote address and UA string are probably not harmful but I'm not sure they're helpful, and for unsophisticated users they might be confusing.

Maybe a half-step toward this is to tie activity log events into email via PeopleMailEngine, so the mail can say some variation of "Internal Log Reference Number: 12345". Administrators can go look up that activity log entry by ID to pull the exact address (and UA or whatever else we store) information if a user reports the activity as suspicious. Installs which really want remote address information in the email would have a short path to a patch, or we could do this upstream in the future. This would also give us a path to putting a [ This is suspicious, report it! ] button in the actual email, which could take the user to /people/logs/1234/fn13lfn13l4/report-as-suspicious/ and let a user raise a particular event to administrators for review. This broadly feels like a more promising path forward than putting an IP in the email and wishing users the best, especially for less-technical users.

This could eventually open up a flow where the "report" UI lets you explain why the action is suspicious and choose a remedy (e.g., "Block all password resets for my account for 24/72/indefinite hours"), which also feels like a clear improvement if this is an ongoing issue.

To step back slightly, a possible question we might ask is "Is the activity log here for the long term?". It's kind of an older object with a bit of weirdness to it. If we start tying it into auth flows, the answer is probably an unambiguous "yes". I think that's fine and it's reasonable to imagine this as a long-term object, I'd just like to move some of the events out of it.

Another general note is that we also require users go through this flow if they're setting a password for the first time on an account which does not already have a password. For example, this workflow will set up the "set your own password" flow:

  • An installs enables "Google" and "Password" auth providers.
  • A user registers with "Google".
  • Later, they decide they'd also like to have a password, so they go to SettingsPassword.

Since we can't prompt them for an old password (their account doesn't have one yet), this flow requires that the user verify they control their email address by clicking a password reset/set link in an email. Partly this is a little bit simpler on our side (we have exactly one flow for changing the password on an account without having the old password), and partly it makes it somewhat more difficult for an attacker who gains control of the account to add a password of their choosing, and partly it reduces HackerOne reports from "researchers" claiming this is a huge fundamental vulnerability in our authentication model..

Likely, the only real implication this has is that the flow has two entry points which are sufficiently different to motivate separate Auth messages.

It may be useful to provide helper methods to support normalizing these actor types (e.g., email addresses should be case-insensitive).

In the case of email addresses, we can just make the target account the "actor", so a given account can only receive X password reset emails per hour across all attached addresses. This is basically a better limit and avoids any need to normalize email addresses.

Not all of this has landed yet, but once it does:

  • The email now has explanatory text with "just ignore this" as an explicit possible next step:

You (or someone pretending to be you) recently requested an account recovery link be sent to this email address. If you did not make this request, you can ignore this message.

  • You can customize this text in AuthCustomize MessagesMail Body: Email Login.
  • Events which generate an account recovery link appear in the user activity log (PeopleActivity Logs).
  • The mail body now references a specific log ID at the bottom.
  • A specific user account may not have links generated for it (across all linked email addresses) at a rate higher than 3 per hour.
  • A specific remote address may not attempt to generate recovery links (across all accounts) at a rate higher than 20 per hour.

Changes to activity logs:

  • Logs can now be browsed by ID and have standalone detail pages with permalinks.
  • Events like "change username" and "disable user" (which are generally initiated by an administrator rather than the user or a suspicious third-party) are no longer part of the activity log.
    • You can find administrative events on the user profile page under Manage.
    • You can find detailed actions by a particular user under FeedTransaction Logs by applying an "Author" filter.
  • The UI now has a typeahead for selecting log types instead of a list of a thousand checkboxes.
epriestley closed this task as Resolved.Jul 24 2019, 5:04 PM

Everything has made it to master now so I suspect we're in good shape here.