Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose EVP_DigestSqueeze from Hasher #2275

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

initsecret
Copy link
Contributor

This completes the streaming API.

/// Squeezes buf out of the hasher. Can be called multiple times, unlike `finish_xof`.
/// The output will be as long as the buf.
#[cfg(ossl330)]
pub fn squeeze_xof(&mut self, buf: &mut [u8]) -> Result<(), ErrorStack> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to check that state isn't finalized? Is it permissible to update after a squeeze?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re finalized: good point, it indeed fails if already finalized: https://github.com/openssl/openssl/blob/master/crypto/sha/sha3.c#L144-L145

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch, lemme fix the PR.

@initsecret initsecret requested a review from alex August 8, 2024 04:59
@alex
Copy link
Collaborator

alex commented Aug 9, 2024

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
Copy link
Contributor

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
Copy link
Collaborator

alex commented Aug 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

initsecret commented Sep 2, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

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

Comment on lines +281 to +292
if self.state == Squeeze {
// [`EVP_DigestUpdate`], depending on the implementation, may allow Updates after Squeezes.
// But, [FIPS 202], as shown in Figure 7, has a distinguished absorbing phase followed by a squeezing phase.
// Indeed, the [`sha3.c`] implmentation disallows Updates after Squeezes.
// For consistency, we always return an error when Update is called after Squeeze.
//
// [`EVP_DigestUpdate`]: https://github.com/openssl/openssl/blob/b3bb214720f20f3b126ae4b9c330e9a48b835415/crypto/evp/digest.c#L385-L393
// [FIPS 202]: https://dx.doi.org/10.6028/NIST.FIPS.202
// [`sha3.c`]: https://github.com/openssl/openssl/blob/b3bb214720f20f3b126ae4b9c330e9a48b835415/crypto/sha/sha3.c#L52-L63
let errors = ErrorStack::get();
return Err(errors);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this check in rust-openssl is not ideal, but it is the best I could come up with.

The alternative is to update individual implementations in OpenSSL, because they likely will not accept this change at the EVP_DigestUpdate level, since they may want to use it for cryptographic objects that allow updates after squeezes.

Here, I think we can be more opinionated and say that if we have objects that support updates after squeezes, they should define a new API, rather than further overload this API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants