runvnc / bcryptnim

15 stars 4 forks source link

Verify that bcryptnim does not have sign-extension bug #1

Closed flaviut closed 10 years ago

flaviut commented 10 years ago

According to the name of the method and PHP docs for the method of the same name, it sounds like the password is being encrypted with the salt as a key. However, the implementation of the hash looks like it is actually bcrypt, but I don't know enough to be able to tell how good it is or if it has bugs.

runvnc commented 10 years ago

Lol. Salted password encryption is a standard security practice so I have no idea what you are trying to say. If you think I used the salt incorrectly please say specifically what I should have done differently. If you have a compilation error please report the actual error so we can work on it or at least other people with the same error know it happened to someone else. On May 22, 2014 1:34 PM, "Flaviu Tamas" notifications@github.com wrote:

I'm not sure this compiles, but encrypting the password with a salt is just a much more convoluted way of storing passwords in plaintext.

— Reply to this email directly or view it on GitHubhttps://github.com/runvnc/bcryptnim/issues/1 .

runvnc commented 10 years ago

Note that this is a Babel module so I don't think you would compile it directly rather you install it with Babel then import it and compile the project that has it as a dependency.

flaviut commented 10 years ago

@runvnc Sorry, my initial bug report wasn't clear and rude; I was hasty to jump to conclusions without reading the function body, its been edited to be more clear, but it seems that github sent you an email with the original version. From reading the name, crypt_blowfish() function sounds as if it encrypts the password with the seed as key, which would be no better than storing it plaintext. On further investigation, I found that the function appears to hash using bcrypt, but it is unclear where the implementation of bcrypt comes from, which is especially relevant if it suffers from the sign-extension bug that many implementations do.

runvnc commented 10 years ago

I took it from FreeBSD. That's why I specified that the license is BSD. Anyway that particular bug affected one implementation of bcrypt that was NOT BSD (See http://security.stackexchange.com/questions/12132/could-the-bcrypt-ruby-binding-be-vulnerable ) Compare bcryptnim to https://github.com/freebsd/freebsd-head/tree/master/secure/lib/libcrypt

Also if you had read the source you can see it says:

/* This code is derived from section 14.3 and the given source
 * in section V of Applied Cryptography, second edition.
 * Blowfish is an unpatented fast block cipher designed by
 * Bruce Schneier.
 */

/*
 * FreeBSD implementation by Paul Herman <pherman@frenchfries.net>
*/

and

/* This password hashing algorithm was designed by David Mazieres
 * <dm@lcs.mit.edu> and works as follows:
 *
 * 1. state := InitState ()
 * 2. state := ExpandKey (state, salt, password) 3.
 * REPEAT rounds:
 *  state := ExpandKey (state, 0, salt)
 *      state := ExpandKey(state, 0, password)
 * 4. ctext := "OrpheanBeholderScryDoubt"
 * 5. REPEAT 64:
 *  ctext := Encrypt_ECB (state, ctext);
 * 6. RETURN Concatenate (salt, ctext);
 *
 */

/*
 * FreeBSD implementation by Paul Herman <pherman@frenchfries.net>
 */
flaviut commented 10 years ago

I did skim through the headers, but I missed that part. I'm very sorry to have wasted your time when there was nothing wrong, I appreciate your patience.