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

SHA-3 update #7714

Merged
merged 6 commits into from
Jun 12, 2023
Merged

SHA-3 update #7714

merged 6 commits into from
Jun 12, 2023

Conversation

daverodgman
Copy link
Contributor

@daverodgman daverodgman commented Jun 8, 2023 •
edited
Loading

Description

Address a few issues in SHA-3 identified in #7708 . Fixes #7713

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog not required - in previous PR
  • backport not required - not in 2.28
  • tests not required

@daverodgman daverodgman requested a review from mpg June 8, 2023 09:15
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests single-reviewer This PR qualifies for having only one reviewer size-xs Estimated task size: extra small (a few hours at most) bug labels Jun 8, 2023
@daverodgman daverodgman changed the title Sha3 update SHA-3 update Jun 8, 2023
mpg
mpg requested changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks, just one more thing about the MD_CAN macro(s).

Btw, ideally, test cases in test_suite_md should be using the appropriate MD_CAN macro as well, rather than SHA3_C, but right now the dependency is correct, and we can update it when we add PSA driver support for SHA-3.

(Note: in test_suite_shax however, the correct dependency is indeed SHA3_C and will remain so even when we add PSA driver support.)

include/mbedtls/md.h
@@ -113,7 +113,10 @@
#define MBEDTLS_MD_SOME_LEGACY
#endif
#if defined(MBEDTLS_SHA3_C)
#define MBEDTLS_MD_CAN_SHA3
#define MBEDTLS_MD_CAN_SHA3_224
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, but I think we also need to update the declared dependencies in tests, namely in tests/suites/test_suite_hmac_drbg.misc.data, otherwise those cases are going to be skipped in all builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: we should have tooling to catch this kind of thing. In fact, we already have 90% of it but it's no good because it doesn't shout when it finds something. See #2691 and #5389, but also #6099.

@daverodgman
Fix test dependencies
33701ac 
Signed-off-by: Dave Rodgman <[email protected]>
tom-cosgrove-arm
tom-cosgrove-arm reviewed Jun 8, 2023
View reviewed changes
include/mbedtls/md.h Outdated
* buffer size using MD_MAX_SIZE in a part of the code that's common to PSA
* and legacy, then assume the buffer's size is PSA_HASH_MAX_SIZE in another
* part of the code based on PSA.
*/
#if defined(MBEDTLS_MD_CAN_SHA512) || defined(MBEDTLS_SHA3_C)
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

MBEDTLS_SHA3_C should be MBEDTLS_MD_CAN_SHA3_512, and there should be separate checks for the other MBEDTLS_MD_CAN_SHA3_xxxs

@daverodgman daverodgman removed the needs-ci Needs to pass CI tests label Jun 9, 2023
tom-cosgrove-arm
tom-cosgrove-arm previously approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 9, 2023
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Jun 11, 2023
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm
Copy link
Contributor

Sorry, but I think there's enough in here that it's not single-reviewer, so removing that label

@tom-cosgrove-arm tom-cosgrove-arm added needs-review Every commit must be reviewed by at least two team members, and removed single-reviewer This PR qualifies for having only one reviewer labels Jun 11, 2023
@tom-cosgrove-arm tom-cosgrove-arm added the needs-reviewer This PR needs someone to pick it up for review label Jun 11, 2023
@tom-cosgrove-arm
Copy link
Contributor

@mpg or @gilles-peskine-arm are either of you two planning to review this?

@mpg mpg removed approved Design and code approved - may be waiting for CI or backports needs-reviewer This PR needs someone to pick it up for review labels Jun 12, 2023
@mpg
Copy link
Contributor

mpg commented Jun 12, 2023

Yes, I've been reviewing, fully intent on re-reviewing.

mpg
mpg approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 12, 2023
@mpg mpg merged commit 14f65a4 into Mbed-TLS:development Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@mpg mpg mpg approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SHA-3: fix MD_CAN macro + misc follow-ups
5 participants
@daverodgman @tom-cosgrove-arm @mpg @gilles-peskine-arm @dave-rodgman