LG-260 Separate otp request tracking and throttling for verified phones

[stevegsa]

Why: So throttling a user with an unverified phone does not throttle a user with a verified phone and we can track these otp requests separately.

How: Add a new phone_confirmed true/false column to the otp_requests_trackers. Add a new unique index for phone_fingerprint + phone_confirmed. Then drop the unique index for phone_fingerprint. Force any interaction with the table to use the new composite key.

Hi! Before submitting your PR for review, and/or before merging it, please
go through the checklists below. These represent the more critical elements
of our code quality guidelines. The rest of the list can be found in
CONTRIBUTING.md

Controllers

• When adding a new controller that requires the user to be fully
  authenticated, make sure to add before_action :confirm_two_factor_authenticated
  as the first callback.

Database

• Unsafe migrations are implemented over several PRs and over several
  deploys to avoid production errors. The strong_migrations gem
  will warn you about unsafe migrations and has great step-by-step instructions
  for various scenarios.

• Indexes were added if necessary. This article provides a good overview
  of indexes in Rails.

• Verified that the changes don't affect other apps (such as the dashboard)

• When relevant, a rake task is created to populate the necessary DB columns
  in the various environments right before deploying, taking into account the users
  who might not have interacted with this column yet (such as users who have not
  set a password yet)

• Migrations against existing tables have been tested against a copy of the
  production database. See LG-228 Make migrations safer and more resilient #2127 for an example when a migration caused deployment
  issues. In that case, all the migration did was add a new column and an index to
  the Users table, which might seem innocuous.

Encryption

• The changes are compatible with data that was encrypted with the old code.

Routes

• GET requests are not vulnerable to CSRF attacks (i.e. they don't change
  state or result in destructive behavior).

Session

• When adding user data to the session, use the user_session helper
  instead of the session helper so the data does not persist beyond the user's
  session.

Testing

• Tests added for this feature/bug
• Prefer feature/integration specs over controller specs
• When adding code that reads data, write tests for nil values, empty strings,
  and invalid inputs.

[stevegsa]

Table update:

[jgsmith-usds]

This works, but why not just put this on the previous line with default: false?

[stevegsa]

Strong migrations prevented it.

[jmhooper]

A bit of a nit, but thoughts on changing this up to take additional args as options, e.g.

def self.find_or_create_with_phone(phone, phone_confirmed:)
  # ...
end

[monfresh]

For visibility and transparency, Steve and I discussed this last week on Slack, and I proposed a different solution that would simplify things and allow us to eliminate the OtpRequestsTracker table. This issue was placed in the backlog a while back (because we were still trying to find the best solution), and as far as I know, hasn't been officially prioritized again by any end user product manager.

This PR addresses one particular scenario, where user B who knows user A's phone number could attempt to sign up with user A's number, and keep requesting OTPs until they get locked out, which would also prevent user A from signing in because they would be locked out as well. However, it doesn't address the scenario where user B knows that user A wants to change their phone number to number X, and user B could request OTPs to phone number X until they get locked out, at which point user A would not be able to change their number to phone number X, or use this phone to verify their identity.

The fundamental problem with the original rate limiting solution we came up with is that we are locking out users, thereby adversely affecting legitimate users. I propose that we stop locking out users. Instead, what does the team think about implementing a general rate limit of X number of OTP requests over Y period of time per user, not per phone number? Twilio recommends 1 request per minute, which is essentially what our current Figaro settings average out to. Instead of locking out users, we display an error message similar to the one GitHub shows:

Also, we want to make sure to track as much information as possible about these events, such as the context. Right now, we can't easily tell if they're happening mainly during authentication or confirmation.

[stevegsa]

@monfresh The solution you mention opens us up to an unlimited number of sms requests by any number of users to arbitrary phone numbers and worse a coordinated unthrottled multi-user attack to a single number (ie 1000 simultaneous sms requests to phone number X). The solution as presented prioritizes users that already have a registered phone which is the case a majority of the time in the lifecycle of the user. That is also the exact attack vector that was submitted to h1 (preventing login of a registered user/phone). Moreover I was assigned to all the issues in h1 in May by the director and fixed all of them except this one because it was assigned to another developer by the PM. When that developer removed himself from the issue in August I resolved it since we had 30 days to resolve any h1 issue.

[monfresh]

That's a good point (about the multiple accounts sending an OTP at the same time to the same number). It's worth noting that Twilio's Verify service has rate limiting built in, which I believe will prevent this. I couldn't find much documentation, but I've seen some users in our logs hitting this rate limit when they've tried to request a bunch of OTPs over a short period of time. Twilio blocked this before our own rate limiting code kicked in.

I think it's worth spending time brainstorming some more solutions that can balance usability and security. For example, this small UI tweak can slow down how many requests a user can make: remove the "Get another code" link from the OTP verification screen. This is most likely how the user I saw in the logs was able to make so many requests within the same second: they just clicked the link a bunch of times really fast, which I can easily reproduce.

[stevegsa]

Yep. I won't merge unless we're all onboard.

[konklone]

FWIW, the 30 day response time for H1 is not a binding thing, but was our goal as an office to measure our own response time, and to encourage a positive and responsive relationship with the security community. And our contract with H1 is over anyhow. So if the team thinks this merits further discussion, by all means.

[monfresh]

Should we close this for now, until we discuss this further? See the JIRA issue for a link to the updated Google Doc where I wrote my thoughts.

[stevegsa]

No. Let's plan on a group meeting on this one first.

[stevegsa]

I'll look at your doc and add comments.

[jmhooper]

@stevegsa: This one has been sitting here for a while. Any objections to closing it?

[brodygov]

I added some discussion to the Jira ticket, but it would be good to at least make a decision on what to do with the outstanding HackerOne report.

Are there still current objections to merging / adopting the current proposal as-is? I do see that there are now merge conflicts.

IMO it's good to continue rate limiting on phone number, and we shouldn't feel obligated to solve all rate limiting problems in this space upfront. The sign-in lockout issue is particularly bad, even though we've never seen evidence of exploitation. So I like this current proposal of separating rate limits based on whether the phone number has been confirmed.