Small security issue with :lockable

[cfeckardt]

Environment

• Ruby any
• Rails any running a multithreaded server (like puma)
• Devise 4.5.0

Current behavior

Pentesters found an issue where our users did not lock (using :lockable), despite running many many attempts to brute force the password.
To reproduce, try to login many times at exactly the same time (where your model is :lockable).
100 attempts within 1 milliseconds of each other will not increment the failed_attempts attribute on your user 100 times on a busy or slow database. They will most likely be incremented by approximately 100 % num_threads where num_threads is the number of threads your server is configured to.

This is a test that describes the behaviour that I would reasonably expect:

 describe '#increment_failed_attempts' do
   let(:user) { create :user }
   let(:same_user) { User.find(user.id) }
   it 'increments the count independently across instances' do
     expect {
       same_user.increment_failed_attempts
       same_user.save
       user.increment_failed_attempts
     }.to change { user.reload.failed_attempts }.by(2)
   end
 end

This test will fail in devise 4.5.0

Expected behavior

The issue arises because we read and set failed_attempts in two steps, instead of in one database transaction.

Below is an excerpt of the method from lockable.rb, my comments added:

def increment_failed_attempts
  self.failed_attempts ||= 0 # This line triggers a read in ActiveRecord if not already cached
  # some other thread now increments the counter to 1
  self.failed_attempts += 1 # We have already read the value 0, and will increment that erroneously to 1 again
end

Our workaround we use is something along the lines of:

def increment_failed_attempts
  self.failed_attempts = self.class.connection.execute("UPDATE SET (failed_attempts = failed_attempts + 1) WHERE id = #{self.id} RETURNING failed_attempts").getvalue(0,0)
end

which passes the test above. This solution is postgres specific (and also assumes integer ids) so may not be suitable for devise, but the idea stands. Reading and writing from this attribute needs to happen on at least a row level transaction in the database.

[tegon]

Hey @cfeckardt, thanks for reporting this.

Yeah, it definitely makes sense to do both operations in a row level transaction. We'll see how we can do this.

Thanks!

[reedloden]

Has a CVE been assigned to this TOCTOU issue?

[tegon]

@reedloden Not yet. I want to apologize for taking so long to reply on this but I knew nothing about CVEs and had to do some research on this topic before I was able to do it.
Anyway, I requested one via DWF's form and will let you know as soon as I get a response.

[tegon]

@cfeckardt I'd also like to add that for security issues it's better to report to us directly via the opensource@plataformatec.com.br email (like we mention in our contribution guide). This way we don't leak the vulnerability before we can release a fix.

At first, I thought this issue wasn't a security breach since the attacker still has to succeed on a brute force attack in order to exploit this. After talking to other people and analyzing deeply, it really is a vulnerability since people are using the lockable module thinking that their applications are going to block an account after a defined number of failed attempts, but in this case, this might not be true.

Now the issue is public for quite some time, so I guess it's ok to leave it here. Just to keep everyone on the same page, this was fixed on v4.6.0.

Thanks, everyone!

[reedloden]

I prepped rubysec/ruby-advisory-db#375 for this. Just need the CVE ID.

@cfeckardt let me know if you run into issues getting a CVE ID. I can make something happen if needed.

[tegon]

@reedloden Thanks, I've accepted the terms last Sunday and I'm waiting for a response. I'll let you know if I don't get any.

[reedloden]

@tegon been 19 days... any updates?

[tegon]

I asked Kurt Seifried yesterday about it and I'm waiting for a response. This was addressed in this pull request that later got closed, so I'm not sure if it's going to be included in a new PR or not.

[tegon]

@reedloden Kurt said that the DWF has been shut down 😞
Can you help us to get the CVE?

[reedloden]

@tegon Huh, hadn't heard that. Sad to hear... In any case, this has been assigned CVE-2019-5421. I'll get this updated at MITRE shortly.

[tegon]

@reedloden Thank you!

[dougo]

Any plans to make a patch release for 4.4.3?

[tegon]

@dougo There are no plans yet but can we can discuss it. Are you having complications updating to 4.6 - e.g. something is broken for you?

[dougo]

Yep, the "Set encrypted_password to nil when password is set to nil" bugfix made a bunch of our tests fail when I upgraded (I guess we were relying on it being empty string, and we have a non-null constraint in the db). Probably easy to fix, but not the sort of yak-shaving I wanted to do while addressing a CVE...

[lukebooth]

I think I agree with @dougo, there should be a patch release for your supported versions instead of asking everyone to bump their devise gems, sometimes multiple, minor versions for this CVE. Unless 4.6.x is your only supported version?

[tegon]

the "Set encrypted_password to nil when the password is set to nil" bugfix made a bunch of our tests fail when I upgraded

@dougo This change caused a lot of issues so we decided to roll it back, you can update to version 4.6.2 now in order to avoid this.

Since we follow SemVer here, updating two minor versions should not break anything. Of course, mistakes happen so we introduced a bug that broke some applications, but it's fixed now in version 4.6.2.
Please let us know if there are any other issues when upgrading so that we can discuss them case-by-case. But overall we should be releasing minor versions that are safe to upgrade.

It was definitely a mistake to release a security fix in a minor version along with other changes, and next time I'll do it in a minor version separately.