Implement PBKDF2, Scrypt, and HMAC #8989

DaGenix · 2013-09-05T05:35:51Z

With Rust getting more mature, its only a matter of time until someone decides to store passwords with an application written in it (if that hasn't happened already). If someone is going to store a password, its important to protect it against offline brute force attacks. Rust already has some Digest implementations, but they are all exceptionally poor choices to encrypt a password since they are designed to run very quickly and are easily attacked with brute force methods using GPUs. The three major functions to securely store a password are PBKDF2, Scrypt, and Bcrypt. This pull request implements the first two of those. Scrypt is the most secure and is based off of PBKDF2. PBKDF2 is a standard. Bcrypt is less secure than Scrypt and isn't really a standard, so I didn't implement that function at this time. PBKDF2 requires a Pseudo Random Function to work which is generally an HMAC, so I implemented a Mac trait (Message Authentication Code) and the HMAC struct that implements that trait. I also added a new method to the Digest trait, output_bytes(), which does basically the same thing as the existing output_bits() method, but is generally more convenient to use.

I didn't implement any benchmarks for the new functions since Key Derivation Functions are slow by design and Rust's benchmarking functions expect each iteration to be very fast. So, the benchmark would have to test exceptionally weak parameter values to work at all which would mean that the benchmark would be testing configurations that no one should really be running in practice.

huonw · 2013-09-05T08:08:22Z

src/libextra/crypto/cryptoutil.rs

+        x = x | (lhs[i] ^ rhs[i]);
+    }
+
+    return x == Zero::zero();


I believe this line could just be x.is_zero().

alexcrichton · 2013-09-05T08:35:19Z

Just want to say, these are awesome!

huonw · 2013-09-05T08:38:20Z

Seconding @alexcrichton; these are awesome!

bluss · 2013-09-05T12:37:09Z

Things like fixed_time_eq are tricky, you must trust that the optimizer doesn't find a way to short-circuit it, on any platform. Is there some kind of compiler support or llvm intrinsics we can use to make this safe?

sanxiyn · 2013-09-05T13:48:25Z

LLVM added optnone in r189101.

DaGenix · 2013-09-06T00:57:47Z

Thanks for the reviews and feedback! I'll make sure to address all the comments.

pcwalton · 2013-09-10T04:53:53Z

Can we get a crypto expert to review this code (perhaps someone on the Moz security team)? As a point of comparison, Adam Langley, a world-renowned crypto expert, has written all the Go crypto code. I do not consider myself qualified to review this properly.

Until we have thorough review on this, I would like a huge disclaimer saying "EXPERIMENTAL; USE AT YOUR OWN RISK".

huonw · 2013-09-10T05:28:24Z

(Putting #[experimental="correctness not validated"] on the free standing public functions would give a warning with that text for every use of them. It doesn't work for normal methods yet; I'm unsure about static methods.)

DaGenix · 2013-09-11T05:04:10Z

I'm still working on getting all the comments addressed. I'll add in an experimental warning. Just in the extremely unlikely case that a security expert is looking at the pull request and thinking about doing an in-depth review, although I would love such a review, my update to the pull request will change things that would probably invalidate such a review.

I agree I got too clever with the fixed_time_eq method. There is no point in having it be generic - I'll remove that. A FixedTimeEq trait with a macro probably could work, but I'm not aware of any need for such a method except for u8s. Adding #[inline(never)] wouldn't hurt, but it wouldn't solve the most severe problem - the concern that LLVM adds an optimization in the future that is clever enough to compile the method to faster, non-constant time code.

I looked into marking the method with the new no-opt LLVM function attribute. However, there are some issues with that:

The no-opt flag is very new. As of a few days ago, it wasn't in the snapshot of LLVM being used by Rust.
The C interface to LLVM doesn't currently expose the flag since its passed in as a bit-mask and the value of this flag is 1 << 42.
I hacked around the first 2 problems, but that leads to the next problem - all that LLVM currently implements is the flag. As best as I can tell, it doesn't do anything yet.
The flag seems perfect for this use case, but, I'm a little unclear on if it was intended for security purposes. If its not intended for use in this type of function, i'd be a little worried that a future change might subtly break the behavior. Anyway, this would need some more investigation.

So, given all of that, I think the most conservative approach might be to just re-write the method using asm!(). Doing this guarantees that LLVM won't apply an optimization that breaks the constant-time property. Its a pretty simple method so its not all that much work or code. It could always be replaced with a pure-Rust implementation if the no-opt flag for LLVM turns out to fit this use case and if it can't be, as I said, its a tiny amount of asm code. The method as it stands in the original pull request is basically the same as the similar function from Go. I'll try to do a survey other other crypto libraries to see what else is out there.

Thoughts?

klutzy · 2013-09-11T11:01:21Z

src/libextra/crypto/scrypt.rs

+static MAX_MEM: u32 = 1 << 30;
+
+// The salsa20/8 core function.
+fn salsa20_8(input: &[u8], output: &mut [u8]) {


I think it would be better if whole salsa20 family is provided at crypto/salsa20.rs in future.

If Salsa20 were implemented, this function would definitely make more sense there. Since this is a private function, it can easily be moved later. Since I'm not attempting to implement more than the tiny bit of Salsa20 that is necessary for Scrypt, I think it makes sense to leave it here for now since seeing a file name salsa20.rs would be a bit confusing at the current time..

sanxiyn · 2013-09-11T14:48:26Z

I think optnone attribute was introduced to avoid misoptimization bugs and not intended for security purposes, per se.

klutzy · 2013-09-11T14:57:18Z

~~Agreed. We should assume that any crypto implementations are prone to side-channel attack until optnone is used.~~ (I misread sanxiyn's comment)

Also, we may verify correctness (not including side-channels) of crypto blocks (e.g. salsa20_8 cipher) using ECRYPT framework like original salsa20 did.

This function compares two vectors using a fixed number of operations. This method should be used wherever an adversary might time how long a comparison takes and derive useful information from that.

DaGenix · 2013-09-14T04:53:39Z

I emailed the author of the optnone patches for LLVM who said that the purpose of the patches is to disable optimizations for certain functions to help with debugging. The optnone option won't disable all optimizations, even when finished. Specifically, inter-procedural optimizations will still be done. Its a bit unclear to me if that would risk breaking the constant time property of the function. Anyway, its not really feasible to analyze it all that much deeper at the current time since the patches haven't been completed yet.

I don't think that we need to assume that any implementation is vulnerable. However, any implementation that passes through LLVM's optimization passes may be. I'll post a re-based version of these patches shortly that re-implements the fixed_time_eq function using assembly which should guarantee that LLVM's optimizer won't get in the way, even without optnone.

DaGenix · 2013-09-14T05:13:01Z

Rebased the commit to address all comments (except as noted below). If I didn't address anything, it's something I missed so please point it out to me so I can fixup my changes!

I consider this to be basically an RFC state - I want to get more feedback, but even in the unlikely case that everything looks great, please don't r+ since I want to run more tests to make sure that everything is working.

Notes:

The pbkdf2_simple and scrypt_simple() functions both depend on IsaacRng being a cryptographically strong RNG. I believe that this is true and checked the code to make sure that it is being seeded from good quality random data (/dev/urandom on UNIX and the equivalent on Windows).
I didn't modify the format of the hash strings. I'd be happy to implement the same format as another library if someone could point me in the direction of the format that they would like me to implement.
I left the function names unchanged. I'm not sure what to change them to. scrypt::scrypt() is indeed unfortunate, however, I'm not sure what name would be better in the case that someone imports the functions directly. Suggestions are very welcome.
I didn't create a separate module for cryptoutil yet. This makes sense to me, so, I'll get this done the next time I rebase this series.
I removed the #[cfg]s that disabled Scrypt on 16bit platforms. Scrypt isn't really all that useful on such platforms since it needs big chunks of memory to operate on. However, I thought that just disabling it was kinda yucky. So, instead, I implemented checks to make sure that provided parameter values will fit within a uint on all platforms. I couldn't find existing Rust functions to do that type of check, so I implemented the CheckedNumCast trait in std::num; A side question: is (vec.len() / 32 < 0xffffffff) well defined if a uint is only 16 bits? I'm assuming that LLVM will just optimize that into a no-op, but, I'm not sure.
I added the #[experimental] annotation to all the relevant functions. As noted by @huonw, it won't work on non-free standing functions. I'm not sure what to do about that.
I implemented the comments about using chunk_iter(). The only issue is that there didn't appear to be an implementation of mut_chunk_iter(), so I implemented one in vec.rs.

DaGenix · 2013-09-14T05:21:48Z

Oh, I also re-wrote the fixed_time_eq() function in assembly using asm!() for both x86 and ARM which should guarantee that the function won't be turned into a non-fixed time function by an LLVM optimization pass. I tested on x86_64 and ARM. I didn't test on 32-bit x86, but I believe it should work there too. I don't think there are any other architectures that Rust currently compiles for.

klutzy · 2013-09-14T05:27:24Z

Wow, thanks a lot!

On optnone, sanxiyn corrected it above but I just misread his comment. sorry for confusion! I now think assembly is the right way to do it safely.

huonw · 2013-09-14T05:30:16Z

src/libextra/crypto/scrypt.rs

+}
+
+#[cfg(test)]
+#[cfg(test)]


Just to be sure it gets removed? :P

Lol. Better safe than sorry!

luqmana · 2013-09-14T05:32:06Z

@DaGenix Well there's some level of mips support, but I'm not sure if it even works right now since I don't think anyone builds or tests it.

pcwalton · 2013-09-17T16:30:49Z

FWIW, the recommendation from the Mozilla security engineers I've talked to is that Rust not implement any crypto primitives and instead delegate to NSS or OpenSSL.

thestinger · 2013-09-17T17:05:51Z

@pcwalton: we can tag functions as #[experimental] now (throwing lint warnings for users), so I think it makes sense to add this stuff but leave it flagged until we have domain experts to review it

OpenSSL isn't GPL-compatible, so it would rule out static linking for many users, and with NSS they would be responsible for making the NSS source available (as with LGPL).

brson · 2013-09-18T21:08:27Z

While this is an impressive piece of code, people much smarter that me warn against reimplementing crypto, and the current consensus on the team is that we should not do it. This would be great material for an external Rust package, for users who understand the tradeoffs, but I'm not comfortable maintaining hand-rolled crypto, nor exposing users to it in the standard distribution.

Thanks for your patience and effort @DaGenix. Closing.

feat(lint): add default_iter_empty close rust-lang#8915 This PR adds `default_iter_empty` lint. This lint checks `std::iter::Empty::default()` and replace with `std::iter::empty()`. Thank you in advance. --- changelog: add `default_instead_of_iter_empty` lint.

huonw reviewed Sep 5, 2013
View reviewed changes

klutzy reviewed Sep 11, 2013
View reviewed changes

Palmer Cox added 7 commits September 14, 2013 00:29

Implement CheckedNumCast trait for integer types.

5afcdb2

Implement mut_chunk_iter for mutable vectors.

0b3dd20

Digest: Add the block_size() method to the Digest trait.

7eb8658

Digest: Add the output_bytes() method to the Digest trait.

9435061

Crypto: Implement fixed_time_eq function.

72dcb6e

This function compares two vectors using a fixed number of operations. This method should be used wherever an adversary might time how long a comparison takes and derive useful information from that.

Crypto: Implement the read_u32_le and read_u32_be functions.

ea73aa6

Hmac: Implement the Mac trait and the Hmac function.

055adbc

PBKDF2: Implement the PBKDF2 Key Derivation function.

490468e

Scrypt: Implement the Scrypt Key Derivation function.

a70eced

huonw reviewed Sep 14, 2013
View reviewed changes

brson closed this Sep 18, 2013

brson mentioned this pull request Sep 18, 2013

Reconsider distribution of cryptographic hash functions #9300

Closed

j8r mentioned this pull request Apr 16, 2019

Add Argon2 password hashing implementation to Standard Library crystal-lang/crystal#7676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PBKDF2, Scrypt, and HMAC #8989

Implement PBKDF2, Scrypt, and HMAC #8989

DaGenix commented Sep 5, 2013

huonw Sep 5, 2013

alexcrichton commented Sep 5, 2013

huonw commented Sep 5, 2013

bluss commented Sep 5, 2013

sanxiyn commented Sep 5, 2013

DaGenix commented Sep 6, 2013

pcwalton commented Sep 10, 2013

huonw commented Sep 10, 2013

DaGenix commented Sep 11, 2013

klutzy Sep 11, 2013

DaGenix Sep 14, 2013

sanxiyn commented Sep 11, 2013

klutzy commented Sep 11, 2013

DaGenix commented Sep 14, 2013

DaGenix commented Sep 14, 2013

DaGenix commented Sep 14, 2013

klutzy commented Sep 14, 2013

huonw Sep 14, 2013

DaGenix Sep 14, 2013

luqmana commented Sep 14, 2013

pcwalton commented Sep 17, 2013

thestinger commented Sep 17, 2013

brson commented Sep 18, 2013

Implement PBKDF2, Scrypt, and HMAC #8989

Implement PBKDF2, Scrypt, and HMAC #8989

Conversation

DaGenix commented Sep 5, 2013

huonw Sep 5, 2013

Choose a reason for hiding this comment

alexcrichton commented Sep 5, 2013

huonw commented Sep 5, 2013

bluss commented Sep 5, 2013

sanxiyn commented Sep 5, 2013

DaGenix commented Sep 6, 2013

pcwalton commented Sep 10, 2013

huonw commented Sep 10, 2013

DaGenix commented Sep 11, 2013

klutzy Sep 11, 2013

Choose a reason for hiding this comment

DaGenix Sep 14, 2013

Choose a reason for hiding this comment

sanxiyn commented Sep 11, 2013

klutzy commented Sep 11, 2013

DaGenix commented Sep 14, 2013

DaGenix commented Sep 14, 2013

DaGenix commented Sep 14, 2013

klutzy commented Sep 14, 2013

huonw Sep 14, 2013

Choose a reason for hiding this comment

DaGenix Sep 14, 2013

Choose a reason for hiding this comment

luqmana commented Sep 14, 2013

pcwalton commented Sep 17, 2013

thestinger commented Sep 17, 2013

brson commented Sep 18, 2013