sfackler / rust-openssl

OpenSSL bindings for Rust
1.4k stars 752 forks source link

Expose EVP_DigestSqueeze from Hasher #2275

Open initsecret opened 3 months ago

initsecret commented 3 months ago

This completes the streaming API.

alex commented 3 months ago

Hmm, so it looks like if you do a squeeze() followed by a finalize(), the finalize() will do an init(). It seems like that should be an allowable sequence, that's basically equivalent to calling squeeze() 2x. WDYT?

reaperhulk commented 3 months ago

I am having trouble following the state machine transitions here. If I call squeeze_xof, followed by update, followed by squeeze_xof again it will initialize the underlying hasher 3 separate times since that ordering is not allowable.

Existing behavior (prior to this PR) appears to allow calling finish multiple times where each subsequent invocation re-initializes and then immediately finalizes the hasher. Instead we should really just error (as the underlying OpenSSL hasher would!) on disallowed behavior like this.

The particular case Alex mentioned should be fixed, but we should really fix the state machine here in general since it allows nonsense.

alex commented 3 months ago

Unfortunately we can't change the existing behavior, but I guess there's no reason we can't just make it an error to call other methods after squeeze!

initsecret commented 3 months ago

Yes, I tried to preserve prior behavior of silently re-initializing instead of error-ing.

Nope, it's not allowable to call finalize after squeeze. We could allow it, but that would further deviate from the underlying openssl.

reaperhulk commented 2 months ago

@initsecret are you still planning to revise this to make it an error to call other methods after squeeze while retaining the rest of the behavior of this API? I think, despite the deviation from the rest of the API, that's what I'd prefer, but @alex may disagree.

initsecret commented 2 months ago

I updated the PR to not silently re-init after squeeze, but hash::tests::test_squeeze_then_update seems to fail locally on MacOS and I am not sure why. I feel like it is a bug in OpenSSL because this snippet implies that the intended behavior is that it should fail: https://github.com/openssl/openssl/blob/master/crypto/sha/sha3.c#L52-L63

initsecret commented 2 months ago

Interesting, all OSes seem to allow Updates after Squeeze. The documentation seems to only promise that Finalize will fail if called after Squeeze:

Similar to EVP_DigestFinalXOF() but allows multiple calls to be made to squeeze variable length output data. EVP_DigestFinalXOF() should not be called after this. https://docs.openssl.org/master/man3/EVP_DigestInit/

And indeed, the Finalized check is done at the EVP level while the Squeeze check is only done in the above cited implementation code.

When I get a chance, I will dig deeper because afaict this---allowing updates after squeezing---is inconsistent with Section 4 of FIPS 202.

reaperhulk commented 7 hours ago

@initsecret Are you still planning to look at this? I'm triaging issues for our next pyca/cryptography release and this is in our milestone right now 😄

initsecret commented 7 hours ago

@reaperhulk thanks for the ping. Sorry this dropped off my list, I made a note to look deeper tonight and update the PR.