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.

Sign up for GitHub

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

Jump to bottom

Fixing OpenSSL SHA2 incremental API integration #1454

Merged
merged 6 commits into from
May 15, 2023
Merged

Fixing OpenSSL SHA2 incremental API integration #1454

merged 6 commits into from
May 15, 2023

Conversation

baentsch
Copy link
Member

Possibly fixes open-quantum-safe/oqs-provider#168. Fixes issues in #1420.

  • Corrects SHA2 API documentation
  • Checks all SHA2 OpenSSL API return values
  • Fixes SHA384 and SHA512 incremental API implementations
  • Corrects testing for these APIs
  • Enhances test vectors to avoid re-occurrence of problem
  • [yes] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • [no] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@beldmit
Copy link
Contributor

beldmit commented May 11, 2023

As we prefetch objects, it's probably worth exiting in case on initialization failure?

@baentsch
Copy link
Member Author

As we prefetch objects, it's probably worth exiting in case on initialization failure?

Good point. And while we're at it, initialize only those objects that we'll also need (depending on OQS_USE_XYZ_OPENSSL).

@baentsch baentsch marked this pull request as ready for review May 11, 2023 09:10
@baentsch baentsch requested review from dstebila and xvzcf as code owners May 11, 2023 09:10
@beldmit
Copy link
Contributor

beldmit commented May 11, 2023

In theory, the problem is we need only some openssl-provided implementations for a particular application. So lack of SHA3 hashes shouldn't stop using SHA2.

@baentsch
Copy link
Member Author

@xvzcf This PR should also encompass the things you had looked into as per our conversation yesterday, right? So, if OK, please approve this PR and as discussed instead focus on the AVX2 issue(s?) in #1420 (and/or let me know if I should take a look at that): Thanks in advance.

@baentsch
Copy link
Member Author

In theory, the problem is we need only some openssl-provided implementations for a particular application. So lack of SHA3 hashes shouldn't stop using SHA2.

Agreed. Is anything in this PR precluding this?

@beldmit
Copy link
Contributor

beldmit commented May 11, 2023

I don't know.
In the ideal world there should be, e.g., fallback to a native implementation (as you definitely have it), but then it becomes a mess from a certification point of view. Another option is introduction of bit flags (I want AES and SHA3, SHA2 is not necessary) as a parameter of the init function.

@baentsch
Copy link
Member Author

then it becomes a mess from a certification point of view

Let's cross that bridge as and if we reach it.

xvzcf
xvzcf suggested changes May 11, 2023
View reviewed changes
src/common/common.h Outdated Show resolved Hide resolved
tests/test_hash.c
@@ -117,9 +117,9 @@ static int do_sha384(void) {
OQS_SHA2_sha384_ctx state2;
OQS_SHA2_sha384_inc_ctx_clone(&state2, &state);
// hash with first state
if (msg_len > 64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this file to test_sha2.c? I think that's more clearer than test_hash.c

Copy link
Member Author

Choose a reason for hiding this comment

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

This file name was not my choice. It's also embedded in test scripts. Are you sure you want this? Let's hope nobody else relies on this name...

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it as test_hash.c; in principal one could add other hash algorithms to it. And in some sense, it does behave differently than test_aes.c and test_sha3.c: the AES and SHA-3 ones test against internal specified test vectors, where as test_hash.c just computes the hash of the supplied input which needs to be externally checked against another implementation or test vector.

Copy link
Member Author

Choose a reason for hiding this comment

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

As per Douglas' comment above, left unchanged in 5f8d02c

tests/test_hash.c Show resolved Hide resolved
dstebila
dstebila reviewed May 11, 2023
View reviewed changes
src/common/sha2/sha2_ossl.c
}
EVP_DigestFinal_ex((EVP_MD_CTX *) state->ctx, out, &md_len);
OQS_OPENSSL_GUARD(EVP_DigestFinal_ex((EVP_MD_CTX *) state->ctx, out, &md_len));
EVP_MD_CTX_free((EVP_MD_CTX *) state->ctx);
Copy link
Member

Choose a reason for hiding this comment

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

Would there be any value in setting state->ctx = NULL after this as a precaution against double frees?

Copy link
Member Author

Choose a reason for hiding this comment

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

so changed in 5f8d02c

@baentsch baentsch requested a review from xvzcf May 12, 2023 06:21
@dstebila
Copy link
Member

I'm okay to merge this.

@dstebila dstebila mentioned this pull request May 12, 2023
Merged
1 task
@baentsch
Copy link
Member Author

@xvzcf : Merge is currently blocked by your feedback. Please mark change requests/conversations as resolved or provide explicit approval.

@baentsch baentsch mentioned this pull request May 12, 2023
Open
@baentsch baentsch merged commit 24c7f4f into main May 15, 2023
@baentsch baentsch deleted the mb-osslintegration-fix branch May 15, 2023 15:17
@baentsch baentsch mentioned this pull request May 16, 2023
Closed
@baentsch baentsch mentioned this pull request May 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstebila dstebila dstebila approved these changes

@xvzcf xvzcf xvzcf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug when stopping provider on OSX
4 participants
@baentsch @beldmit @dstebila @xvzcf