Prevent user enumeration by timing attacks

[composerinteralia]

Closes #636

Before this commit, we were skipping any BCrypt (or other password
strategy) checks when we couldn't find the user by email. This opened up
the possibility for a bad actor to detect whether an account existed for
a given email address based on the timing of the response.

This commit mitigates the problem by creating a throw away user and
setting a dummy password on it, triggering BCrypt to create an encrypted
password.

The added tests were consistently failing before this change because
authentication for accounts that did NOT exists was taking 1-2ms longer
that for accounts that did exist (with the cost set to
::BCrypt::Engine::MIN_COST in tests). The timings are now very close.
The be_within delta could almost have been 0.0001, but I stuck with
1ms to avoid any flakiness in the test suite.

[gnfisher]

TIL!

I have just one question regarding testing but this looks great! 👍

[gnfisher]

Is running just one comparison enough to feel confident here? Or should we run it multiple times and make sure all of them remain within the expected delta?

[composerinteralia]

Good question. How many times do you think would be reasonable?

[gnfisher]

I honestly don't know if it is even necessary. I was just thinking of the fuzz testing I've seen done in Elm apps.

[composerinteralia]

If I run each 100 times I can bump the be_within delta from 0.001 to 0.0001 (* 100). That is nice, but it also adds 0.4 seconds to our test suite. I am torn.

[composerinteralia]

I think I am going to leave it alone for now to keep the test suite as fast as possible. These tests in their current form are enough to catch a regression to the old behavior. We can always revisit this if we find that the 1ms delta is not sufficient.

[eebs]

This looks great! I had a question about the specifics around the timings.

[eebs]

My understanding is that this will call password=, which will result in the password strategy running. For BCrypt that is

clearance/lib/clearance/password_strategies/bcrypt.rb

Lines 22 to 31 in 1dd09b0

	def password=(new_password)
	@password = new_password
	if new_password.present?
	self.encrypted_password = ::BCrypt::Password.create(
	new_password,
	cost: configured_bcrypt_cost,
	)
	end
	end

In the case where a user was found, we'd end up calling authenticated?, which would again call the password strategy

clearance/lib/clearance/password_strategies/bcrypt.rb

Lines 16 to 20 in 1dd09b0

	def authenticated?(password)
	if encrypted_password.present?
	::BCrypt::Password.new(encrypted_password) == password
	end
	end

We want these two code paths to take the same amount of time to avoid leaking via timing attacks. Do BCrypt::Password#== and BCrypt::Password.create use the same process under the hood that causes them to take the same amount of time? Will this be true for all password strategies?

[composerinteralia]

For BCrypt the timings seem to be the same

require 'benchmark'
require 'benchmark/ips'
require "bcrypt"

encrypted_password = BCrypt::Password.create("password")

Benchmark.ips do |bm|
  bm.report("compare") { BCrypt::Password.new(encrypted_password) == "password" }
  bm.report("create") { BCrypt::Password.create("password") }
  bm.compare!
end

Warming up --------------------------------------
             compare     1.000  i/100ms
              create     1.000  i/100ms
Calculating -------------------------------------
             compare      4.889  (± 0.0%) i/s -     25.000  in   5.114814s
              create      4.870  (± 0.0%) i/s -     25.000  in   5.134016s

Comparison:
             compare:        4.9 i/s
              create:        4.9 i/s - 1.00x  (± 0.00) slower

[composerinteralia]

For argon2 as well:

require 'benchmark'
require 'benchmark/ips'
require "argon2"

encrypted_password = Argon2::Password.new.create("password")

Benchmark.ips do |bm|
  bm.report("compare") { Argon2::Password.verify_password("password", encrypted_password) }
  bm.report("create") { Argon2::Password.new.create("password") }
  bm.compare!
end

             compare     1.000  i/100ms
              create     1.000  i/100ms
Calculating -------------------------------------
             compare      8.335  (± 0.0%) i/s -     42.000  in   5.044675s
              create      8.314  (± 0.0%) i/s -     42.000  in   5.057424s

Comparison:
             compare:        8.3 i/s
              create:        8.3 i/s - 1.00x  (± 0.00) slower

[eebs]

Awesome, thanks for the benchmark! I think this is the best we can do until something changes. 👍

[composerinteralia]

I went with this approach because it was the only way I could think of fix this for any password strategy. Another way to fix would be to build a DUMMY_ENCRYPTED_PASSWORD and check against that, but the password strategies don't currently have an API for doing that so that would only work for our officially supported password strategies by modifying the API a bit.

[eebs]

Yeah, I think what you have here is excellent. If we find that some supported strategy in the future has different timings for creating and equality checks we can revisit this.

[composerinteralia]

@eebs @gnfisher I am thinking of sending out a patch release today with just this change (we can release the other bug fixes when we meet again next week). I would create a new branch off of v2.2.0, apply this one commit, and release from that branch.

[eebs]

Sounds good. I'd love to pair on that with you if we can find some overlapping time.

[gnfisher]

I would also join if the time works out!