Add option to build SHA-512 without SHA-384 #179
No reviewers
Labels
No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-ARMmbed/mbed-crypto!179
Loading…
Reference in a new issue
No description provided.
Delete branch "sha512-no-sha384"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This adds a config.h option
MBEDTLS_SHA512_NO_SHA384that allows to build with SHA-512 enabled but not SHA-384, for people who don't need SHA-384 and want to save on code size.This is very comparable to https://github.com/ARMmbed/mbedtls-restricted/pull/621 (which should be side-ported).
Contrary to SHA-256 however, it seems desirable to also have a symmetrical option for people who only need SHA-384 but not SHA-512. In order to keep this PR small and allow it to move forward quickly, it was not included but can be done later.
Reviewers, please double-check if I didn't miss anything to do with PSA as I'm less familiar with that part.
Size, building with
CFLAGS='-Os -mcpu=cortex-m0plus -mthumb' CC=arm-none-eabi-gcc(ARM-GCC 7.3) after runningscripts/config.pl baremetal(don't pay attention to the absolute values as this is a full config, only the difference is interesting):libmbedcrypto.awith SHA-384libmbedcrypto.awithout SHA-384Note: If #327 is merged first, this PR will need to be modified to add a dependency on
!MBEDTLS_SHA512_NO_SHA384to some test cases added by #327.Please extend
tests/scripts/depends-hashes.plto support the new options.Note: an extension of
depends-hashes.plfor a similar option has been done in cousin PR https://github.com/ARMmbed/mbedtls-restricted/pull/621 and would be pretty straightforward to adapt here.@gilles-peskine-arm I rebased in order to resolve conflicts (previous state is at https://github.com/mpg/mbed-crypto/tree/sha512-no-sha384-1) and extended
depends-hashes.plas you suggested. I think this is now ready for review again.What's there is good other than
md_wrap.cand cosmetic issues in test data. But several things are missing.@ -1036,0 +1037,4 @@* \def MBEDTLS_SHA512_NO_SHA384** Disable the SHA-384 option of the SHA-512 module. Use this to save some* code size on devices that don't use SHA-384.Missing: since this option is meaningless without
MBEDTLS_SHA512_C, it should declare a requirement onMBEDTLS_SHA512_Cand enforce it incheck_config.h.@ -64,3 +65,4 @@#endif}mbedtls_sha512_context;Missing: in function documentation, explain what the
is384parameter means whenMBEDTLS_SHA512_NO_SHA384is defined.md_wrap.cno longer exists. See https://github.com/ARMmbed/mbed-crypto/pull/185Missing:
case PSA_ALG_SHA_384inpsa_hash_setup(the other cases have negligible impact)@ -154,3 +160,4 @@#else/* SHA-384 */ctx->state[0] = UL64(0xCBBB9D5DC1059ED8);ctx->state[1] = UL64(0x629A292A367CD507);Missing above:
SHA512_VALIDATE_RETshould not acceptis384 == 1if SHA384 is disabled.Minor: I'd prefer to write
ARRAY_LENGTH(sha512_test_sum)with a genericARRAY_LENGTHmacro so that it's evident at the point of use that we're looking at the length of the relevant array.@ -651,147 +651,147 @@ depends_on:MBEDTLS_DES_C:MBEDTLS_SHA256_C:MBEDTLS_PKCS5_Cpk_parse_keyfile_rsa:"data_files/rsa_pkcs8_pbes2_pbkdf2_4096_des_sha256.der":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMATMinor: I think it would be easier to read if
!MBEDTLS_SHA512_NO_SHA384was consecutive toMBEDTLS_SHA512_C.@ -23,11 +23,11 @@ depends_on:MBEDTLS_SHA256_C:MBEDTLS_PKCS1_V15mbedtls_rsa_pkcs1_verify:"cd810e97dc21095ea7a0238027a7bafd343e01444785ea9184a44a79f80438c41fc0b57aa95693407da38fe5ff0ec1398e03361e51a3dbe134b99cca2df0cef1c444ca54d2b7db2789455b6bb41918c24001fd82fc20ee089de3f34f053699c1c5f7954ce0aaabb9d26fce39d032894152229d98cf64ecafc7089530073c61d9":MBEDTLS_RSA_PKCS_V15:MBEDTLS_MD_SHA256:1024:16:"e28a13548525e5f36dccb24ecb7cc332cc689dfd64012604c9c7816d72a16c3f5fcdc0e86e7c03280b1c69b586ce0cd8aec722cc73a5d3b730310bf7dfebdc77ce5d94bbc369dc18a2f7b07bd505ab0f82224aef09fdc1e5063234255e0b3c40a52e9e8ae60898eb88a766bdd788fe9493d8fd86bcdd2884d5c06216c65469e5":16:"3":"7b5fba70ec5b521638f182bcab39cec30b76e7bc017bdbd1059658a9a1db0969ab482dce32f3e9865952f0a0de0978272c951e3c015328ea3758f47029a379ab4200550fba58f11d51264878406fc717d5f7b72b3582946f16a7e5314a220881fc820f7d29949710273421533d8ac0a449dc6d0fd1a21c22444edd1c0d5b44d3":0Minor: once is enough.
Oh, nice, I was looking at 185 just last week, and I'm glad it's been merged in the meantime.
@ -1036,0 +1037,4 @@* \def MBEDTLS_SHA512_NO_SHA384** Disable the SHA-384 option of the SHA-512 module. Use this to save some* code size on devices that don't use SHA-384.I agree with @gilles-peskine-arm in this part.
I am also thinking that enabling some config to disable some function can be misleading. I would suggest to change
MBEDTLS_SHA512_NO_SHA384in to for exampleMBEDTLS_SHA512_WITH_SHA384.MBEDTLS_SHA512_WITH_SHA384points more natural that there is some connection between SHA512 and SHA384. The SHA512_NO_SHA384 was not clearly at the beginning for me.@ -81,2 +81,4 @@#if defined(MBEDTLS_SHA512_C)#if !defined(MBEDTLS_SHA512_NO_SHA384)extern const mbedtls_md_info_t mbedtls_sha384_info;#endifThere is many places when you use this sentence with double negation "
#if !defined(MBEDTLS_SHA512_NO_SHA384)" an it makes riding it is not very friendly. This is a second advantage to use option likeMBEDTLS_SHA512_WITH_SHA384@ -61,3 +61,4 @@unsigned char buffer[128]; /*!< The data block being processed. */#if !defined(MBEDTLS_SHA512_NO_SHA384)int is384; /*!< Determines which function to use:0: Use SHA-512, or 1: Use SHA-384. */I would change
int is384;in tochar is384;or evenboolto keep good practice with structure padding@ -1036,0 +1037,4 @@* \def MBEDTLS_SHA512_NO_SHA384** Disable the SHA-384 option of the SHA-512 module. Use this to save some* code size on devices that don't use SHA-384.Regarding
NOvsWITH, I agree in principle, but unfortunately there's a backwards-compatibility issue. Currently people who use a customconfig.hcan just defineMBEDTLS_SHA512_Cand they get SHA-384 as well. If make SHA-384 available only ifMBEDTLS_SHA512_WITH_SHA384is defined as well, it means they have to change their code (namely, theirconfig.h) when updating Mbed TLS, which one thing we promised they won't have to do except on major version upgrades.So in Mbed TLS 3.0.0 I agree with should have a
WITHoption rather than aNOoption. But in the meantime if we want to merge for a 2.x release, I'm afraid aNOoption is our only choice.What we could to however, is to translate it to a
WITHoption internally, so that we at least avoid the readability issue inside the.cfile.@ -1036,0 +1037,4 @@* \def MBEDTLS_SHA512_NO_SHA384** Disable the SHA-384 option of the SHA-512 module. Use this to save some* code size on devices that don't use SHA-384.It makes sense to me. Thanks for explanation
@ -81,2 +81,4 @@#if defined(MBEDTLS_SHA512_C)#if !defined(MBEDTLS_SHA512_NO_SHA384)extern const mbedtls_md_info_t mbedtls_sha384_info;#endifWhether
mbedtls_sha384_infoshouldn't be excluded in md.c also?@ -61,3 +61,4 @@unsigned char buffer[128]; /*!< The data block being processed. */#if !defined(MBEDTLS_SHA512_NO_SHA384)int is384; /*!< Determines which function to use:0: Use SHA-512, or 1: Use SHA-384. */That would be an ABI change, so I'd rather not do it here.
I just rebased on current development and force-pushed in order to resolve conflicts. The previous state of the branch was saved at https://github.com/mpg/mbed-crypto/tree/sha512-no-sha384-2
I'll push more commits in order to address review comments later.
@gilles-peskine-arm @piotr-now I believe I addressed your comments, either by changing the code as suggested, or commenting why I'd rather not. This is ready for you to review again.
LGTM apart from a few minor issues.
There's no test of trying to use SHA-384 when it's disabled. But this is a preexisting test gap so this PR doesn't need to do anything about it.
@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,*Minor:
\c MBEDTLS_SHA512_NO_SHA384(and likewise on other functions)Minor:
#MBEDTLS_ERR_SHA512_BAD_INPUT_DATA(and likewise on other functions)This is correct but inconsistent. I'd prefer to either add
#ifaround the other cases or keep this one unconditional.@ -569,6 +587,8 @@ static const unsigned char sha512_test_sum[6][64] =0x4E, 0xAD, 0xB2, 0x17, 0xAD, 0x8C, 0xC0, 0x9B }It's called
ARRAY_LENGTHinlibrary/psa_crypto.candtests/suites/helpers.function. Please keep the names consistent.@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,*I assume you mean
#MBEDTLS_SHA512_NO_SHA384? Or is there a subtlety I'm missing?@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,*I meant
\c, because whenMBEDTLS_SHA512_NO_SHA384is not defined, the#version would render with the#.I'm not sure I understand what the inconsistency is. Could you clarify?
To be consistent,
case PSA_ALG_SHA_512should be under#if defined(MBEDTLS_SHA512_C), etc.@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,*Oh, good point. Never noticed that.
@gilles-peskine-arm Thanks for your feedback, and clarifying when I didn't immediately understand it. I believe I've addressed all of it so feel free to review again!
The CI passes all tests except the ones involving Mbed OS, which is a known issue independent from this PR, so it's as good as a pass as far as merging this PR is concerned.
One minor issue I missed earlier, other than that LGTM
@ -569,6 +587,8 @@ static const unsigned char sha512_test_sum[6][64] =0x4E, 0xAD, 0xB2, 0x17, 0xAD, 0x8C, 0xC0, 0x9B }@gilles-peskine-arm Thanks for catching that issue. I fixed it, this should now be ready for re-review.
@piotr-now You reviewed and approved a previous version of this PR, do you think you'd be able to review it again? Thanks!
LGTM
The CI passes all tests except for the ones relying on mbed-os, which is a know issue with mbed-os, unrelated to this PR, so it's a s good as a pass.