Add ConstantTimeEq support for Hash

[AaronFeickert]

This PR improves the handling of constant-time equality for hashes.

It adds an optional subtle feature that implements ConstantTimeEq for Hash. This trait uses subtle functionality under the hood for the trait implementation.

This also opens the door for other useful subtle traits elsewhere in the future.

[oconnor663]

This change looks good in code, but it seems to come with a substantial performance penalty. I'm pasting this new benchmark at the end of bench.rs. (I don't know if all the no_inline/black_box shenanigans are really necessary, but it's a habit.)

#[no_mangle]
#[inline(never)]
pub fn foo(hash1: &blake3::Hash, hash2: &blake3::Hash) -> bool {
    hash1 == hash2
}

#[bench]
fn bench_eq(b: &mut Bencher) {
    let hash1 = blake3::hash(b"foo");
    let hash2 = blake3::hash(b"bar");
    b.iter(|| foo(test::black_box(&hash1), test::black_box(&hash2)));
}

On master I see:

$ cargo +nightly bench bench_eq
test bench_eq                          ... bench:           1.37 ns/iter (+/- 0.06)

On this branch I see:

$ cargo +nightly bench bench_eq
test bench_eq                          ... bench:          31.80 ns/iter (+/- 0.71)

31 nanoseconds might not sound like a lot, but for comparison that's almost as long as it takes to hash 64 bytes on my laptop (with AVX-512 support):

$ cargo +nightly bench bench_atonce_0001_block
test bench_atonce_0001_block           ... bench:          45.05 ns/iter (+/- 0.33) = 1422 MB/s

If we look at cargo +nightly asm --bench bench foo and (on your branch) cargo asm --lib ct_eq we can see the difference. constant_time_eq is able to use SIMD (only SSE2, so there's still room for improvement if we want to get fancy), but subtle is doing approximately this for each pair of bytes:

        movzx eax, byte ptr [r14 + 1]
        xor edi, edi
        cmp al, byte ptr [rbx + 1]
        sete dil
        call subtle::black_box
        mov r15d, eax
        and r15b, bpl

Have you looked at these sorts of details before? Do you know if there's anything we can do differently in how we're calling subtle, or would this need to be fixed upstream?

A side benefit is that because subtle plays nicely with Miri, there's no need to keep around the fallback constant-time equality functionality.

This does sound nice. If we don't end up landing this change, I might try to contribute something similar to constant_time_eq.

[AaronFeickert]

The optimization barrier in subtle used to be inline assembly, but it was changed a few years back (and has seen some iteration). I've seen discussion about returning to that behavior, with initial concerns about stability and portability. Unfortunately, the use of subtle would currently require the performance hit. That being said, it is pretty standard at this point in the Rust cryptography ecosystem.

The core library does include its own optimization barrier (the one from your benchmarks) that was previously available in subtle. However, a warning was added not to use it for cryptographic purposes, so it's no longer made available.

One option that I mulled over was whether it's reasonably safe enough to use variable-time equality testing for PartialEq, and reserve constant-time equality testing for ConstantTimeEq. This has the downside of being breaking and easier to misuse, but it does allow implementers to decide if their use case requires it for improved flexibility.

[AaronFeickert]

Anyway, I'm happy to close this if the performance hit isn't worth the library change and Miri cleanup.

[oconnor663]

Let's see what Cesar thinks of that PR I just put up. Also if some callers need trait compatibility with subtle::ConstantTimeEq, another option could be to make subtle an optional feature and implement it as a wrapper around the current implementation.

[AaronFeickert]

Is there a particular reason why the current Miri workaround doesn't just use variable-time equality, if it's not intended for use outside of testing (and not used for benchmarks)? Having a pseudo-constant-time variant in the codebase for that purpose seems like a small footgun that someone might use elsewhere unsafely.

[oconnor663]

No particularly good reason, you're right. It might make sense to delete it, but on the other hand there's some copy-paste risk on both sides of the equation. We don't want folks to use the naive function thinking it's constant time, but we also don't want folks to think it's ok to use non-constant-time comparisons with secret values. Either way we kind of have to lean on scary comments.

[AaronFeickert]

Good point overall; the only proper approach would be to nix it entirely, so hopefully your PR gets accepted!

I do think that the overall risk is still greater (at least marginally) with the current workaround, especially given that the function is named constant_time_eq_miri and there's no reason to think it couldn't or wouldn't ever be variable time. Unless it's too much of a nit and should be closed, I've opened #421 to remove it for now in favor of standard variable-time PartialEq on the underlying byte array; a scary comment will accompany it!

Separately, good idea that subtle could be used as an optional feature, to get its useful traits for callers who need or want them. If that were the case, I think it would be proper to have its constant-time trait implementations use subtle directly and not wrap constant_time_eq, in order to ensure the caller is getting what it says on the box. This would have the extra advantage of allowing the existing PartialEq functionality to retain its speedy behavior. The extra cost would be the extra dependency, but at least it would be optional.

[cesarb]

Let's see what Cesar thinks of that PR I just put up.

Sorry for the delay. Looks fine to me. Merged and published constant_time_eq 0.3.1 with only that change.

[AaronFeickert]

The change that @cesarb references is now accounted for in #421.

[oconnor663]

Thanks @AaronFeickert and @cesarb. Merged!

[AaronFeickert]

@oconnor663: After the ongoing discussion, this has been updated to retain the existing (speedy) functionality from constant_time_eq, but add a ConstantTimeEq implementation for Hash via a new optional subtle feature.

I think this is a useful addition, and also opens the door to adding other useful traits in the future if desired.