search

inejge / pwhash

A collection of password hashing routines in pure Rust
MIT License
61 stars 11 forks source link

A few changes to support Base64 strings properly. The code was also formatted. #6

Closed gbip closed 7 years ago

gbip commented 7 years ago

Hello, I ran the crate through cargo fmt. I also fixed a small problem with Base64 strings, where '=' and '+' where creating an EncodeError. If you don't want your crate being formatted, just take the commit 53916b6. This should solve the issue #5.

inejge commented 7 years ago

Thanks for the PR! I agree that the code should be eventually reformatted, but I'd prefer to wait for the formatting RFC process to run its course (it's not too far off).

As for the Base64 encoding/decoding, the change you've proposed would accept invalid encodings, since bcrypt explicitly doesn't include + and = in its Base64 set (which is different from the set used by MIME). See the encoding string: in addition to a-zA-Z0-9, . and / are used to round it off.

So, I can't accept the PR as is -- but you can open an issue requesting rustfmt, which can be accompanied by a new PR once there's a fully defined set of formatting rules.

gbip commented 7 years ago

The problem is that the crate base64 generates Base64 strings based on the MIME one. The data encoding crate bases its implementation on RFC4648 which also includes + and =.

I don't see why bcrypt uses an other set that the one used by MIME, since the original paper talks about

OpenBSD generates the 128-bit bcrypt salt from an arcfour(arc4random(3)) key stream, seeded with random data the kernel collects from device timings.

where it is never mentionned a potential Base64 valid encodings, as long that it is random bytes. Source. Maybe you can point me to the source of this information, please ?

How do you generate your salts on your side ? Do you have a recommended crate ?

Anyway, thanks for taking the time to maintain this !

inejge commented 7 years ago

I don't see why bcrypt uses an other set that the one used by MIME

The + has special meaning in the Unix password file when the historic NIS/YP resolution is active. While it's unlikely that a salt or hash value would end there, it was felt that any chance of confusion would be best eliminated by using something else. Likewise, = separates variables and values in the shell.

I haven't tried to find the authoritative source for the encoding scheme; my code is basically a port of one of the two canonical C implementations, the Openwall one.

How do you generate your salts on your side ?

Internally, with random::gen_salt_bytes(). If you call bcrypt::hash() with just the cleartext password, a random salt will be generated for you.

gbip commented 7 years ago

Okay thanks, I'll make a pull request with a bit more documentation on this point !