search

cdepillabout / password

datatypes and functions for easily working with passwords in Haskell
http://hackage.haskell.org/package/password
55 stars 16 forks source link

potential timing attack in checkPassword #15

Closed cdepillabout closed 4 years ago

cdepillabout commented 4 years ago

@maralorn sent me an email with a concern about a potential timing attack in checkPassword.

Basically, in https://hackage.haskell.org/package/password-2.0.0.0/docs/src/Data.Password.Argon2.html#checkPassword we use the Eq instance of ByteString to compare password hashes.

@maralorn explains that in theory, this could give the attacker information about the position of the first not-matching letter in the hash, via a timing attack.

@maralorn gives a reference to https://crackstation.net/hashing-security.htm (search for "length-constant"?) for some additional info about this, and says that there are packages on hackage which provied string comparision in constant time (e.g. constTimeEq from crypte-api).

cdepillabout commented 4 years ago

@Vlix maybe we want to use constTimeEq from cypte-api just to be safe here?

(Also, I'd be interested in seeing how packages like scrypt deal with this.)

I feel like we should probably take care of this before announcing password-2.0.

Vlix commented 4 years ago

Well, I've read up on the timing attacks, and that seems a valid concern. I think it's a good idea to look into it. Though I'd rather define the Eq instance of PasswordHash like that, so that anyone equaling hashes will do it this way. scrypt seems to also just Eq ByteString. I checked the bcrypt library and they use Data.ByteArray.constEq, which seems to be exactly what we want, and we already depend on memory, so that's nice. The argon2 package seems to do the verifying in a FFI call, so no idea if they do it in constant time.

maralorn commented 4 years ago
  1. The Eq instance sounds great.
  2. Data.ByteArray.constEq sounds also great.
  3. Someone should probably tell the scrypt people. Since you apparently haven‘t yet I’ll take the liberty to do it. Probably best to shoot them a mail, too. Maybe they take it unkindly to post a security issue to the easily findable public tracker. @Vlix if you haven‘t done it, I will do it.
Vlix commented 4 years ago

@maralorn uhmmm, the scrypt package hasn't been worked on since 2014, though... 🤔 The README says to be free to post issues in the issue tracker, so I guess it'd be fine to just throw this in, and if anyone's still maintaining the package, they'll probably notice at some point.

informatikr commented 4 years ago

I am the scrypt Haskell package maintainer (or rather "maintainer"), thanks for informing me.

As far as I understand it, an attacker who knows the salt and parameters, but not the hash (far fetched, in the case of storing passwords), could use the timing information from Bytestring.Eq to perform an offline attack instead of an online attack. The attacker would gain a constant-time advantage. Additionally, a dictionary attack might be possible for weak passwords.

So I don't think this is a problem, but using a constant-time comparison would be prudent "just to be sure".

As you noticed I didn't touch the scrypt package since 2014. Do you have any opinion on deprecating it in favor of password and cryptonite?

cdepillabout commented 4 years ago

As you noticed I didn't touch the scrypt package since 2014. Do you have any opinion on deprecating it in favor of password and cryptonite?

That seems reasonable.

@Vlix What do you think about this?

maralorn commented 4 years ago

As a humble hackage user who just a while back was looking for a password hashing library I must say that having one library point to another via deprecation warning helps discovery and decision paralysis a lot.

Vlix commented 4 years ago

Just like the PBKDF2 package, pointing to another package/other packages might be a good idea. Though there's something to say about just having one package for one algorithm, so that someone who JUST wants scrypt doesn't need the entire cryptonite package. (Though, thankfully, cryptonite does not have many dependencies itself)

@informatikr If you're not expecting to update the scrypt package anymore, saying so might be a good idea to also include in the deprecation message. That way it's clear why it's deprecated and any users of the package can estimate the risks in using it. The deprecation update will probably be a new release, so you could include the constant time ByteString comparison if you'd want to. Maybe use Byteable since it's a small easy package, though not really maintained (but then again, scrypt also isn't)

Vlix commented 4 years ago

I made a PR (#16). @maralorn can you check if it looks right?