Add option to build SHA-512 without SHA-384 #179

Merged
mpg merged 16 commits from sha512-no-sha384 into development 2020-01-30 03:32:20 -06:00
mpg commented 2019-07-17 09:53:14 -05:00 (Migrated from github.com)

This adds a config.h option MBEDTLS_SHA512_NO_SHA384 that 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 running scripts/config.pl baremetal (don't pay attention to the absolute values as this is a full config, only the difference is interesting):

Default Without selftest
libmbedcrypto.a with SHA-384 228769 192320
libmbedcrypto.a without SHA-384 228353 192128
gain in Bytes 416 192

Note: If #327 is merged first, this PR will need to be modified to add a dependency on !MBEDTLS_SHA512_NO_SHA384 to some test cases added by #327.

This adds a config.h option `MBEDTLS_SHA512_NO_SHA384` that 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 running `scripts/config.pl baremetal` (don't pay attention to the absolute values as this is a full config, only the difference is interesting): | | Default | Without selftest | | --- | --- | --- | | `libmbedcrypto.a` with SHA-384 | 228769 | 192320 | | `libmbedcrypto.a` without SHA-384 | 228353 | 192128 | | gain in Bytes | 416 | 192 | **Note:** If #327 is merged first, this PR will need to be modified to add a dependency on `!MBEDTLS_SHA512_NO_SHA384` to some test cases added by #327.
gilles-peskine-arm commented 2019-07-25 12:16:22 -05:00 (Migrated from github.com)

Please extend tests/scripts/depends-hashes.pl to support the new options.

Please extend `tests/scripts/depends-hashes.pl` to support the new options.
mpg commented 2019-09-11 02:20:27 -05:00 (Migrated from github.com)

Note: an extension of depends-hashes.pl for 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.

Note: an extension of `depends-hashes.pl` for 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.
mpg commented 2019-09-11 03:11:02 -05:00 (Migrated from github.com)

@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.pl as you suggested. I think this is now ready for review again.

@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.pl` as you suggested. I think this is now ready for review again.
gilles-peskine-arm (Migrated from github.com) requested changes 2019-09-11 08:27:07 -05:00
gilles-peskine-arm (Migrated from github.com) left a comment

What's there is good other than md_wrap.c and cosmetic issues in test data. But several things are missing.

What's there is good other than `md_wrap.c` and 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.
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 07:44:34 -05:00

Missing: since this option is meaningless without MBEDTLS_SHA512_C, it should declare a requirement on MBEDTLS_SHA512_C and enforce it in check_config.h.

Missing: since this option is meaningless without `MBEDTLS_SHA512_C`, it should declare a requirement on `MBEDTLS_SHA512_C` and enforce it in `check_config.h`.
@ -64,3 +65,4 @@
#endif
}
mbedtls_sha512_context;
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 07:45:34 -05:00

Missing: in function documentation, explain what the is384 parameter means when MBEDTLS_SHA512_NO_SHA384 is defined.

Missing: in function documentation, explain what the `is384` parameter means when `MBEDTLS_SHA512_NO_SHA384` is defined.
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 07:37:36 -05:00

md_wrap.c no longer exists. See https://github.com/ARMmbed/mbed-crypto/pull/185

`md_wrap.c` no longer exists. See https://github.com/ARMmbed/mbed-crypto/pull/185
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 08:14:28 -05:00

Missing: case PSA_ALG_SHA_384 in psa_hash_setup (the other cases have negligible impact)

Missing: `case PSA_ALG_SHA_384` in `psa_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);
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 08:16:31 -05:00

Missing above: SHA512_VALIDATE_RET should not accept is384 == 1 if SHA384 is disabled.

Missing above: `SHA512_VALIDATE_RET` should not accept `is384 == 1` if SHA384 is disabled.
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 08:17:59 -05:00

Minor: I'd prefer to write ARRAY_LENGTH(sha512_test_sum) with a generic ARRAY_LENGTH macro so that it's evident at the point of use that we're looking at the length of the relevant array.

Minor: I'd prefer to write `ARRAY_LENGTH(sha512_test_sum)` with a generic `ARRAY_LENGTH` macro 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_C
pk_parse_keyfile_rsa:"data_files/rsa_pkcs8_pbes2_pbkdf2_4096_des_sha256.der":"":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 08:24:26 -05:00

Minor: I think it would be easier to read if !MBEDTLS_SHA512_NO_SHA384 was consecutive to MBEDTLS_SHA512_C.

Minor: I think it would be easier to read if `!MBEDTLS_SHA512_NO_SHA384` was consecutive to `MBEDTLS_SHA512_C`.
@ -23,11 +23,11 @@ depends_on:MBEDTLS_SHA256_C:MBEDTLS_PKCS1_V15
mbedtls_rsa_pkcs1_verify:"cd810e97dc21095ea7a0238027a7bafd343e01444785ea9184a44a79f80438c41fc0b57aa95693407da38fe5ff0ec1398e03361e51a3dbe134b99cca2df0cef1c444ca54d2b7db2789455b6bb41918c24001fd82fc20ee089de3f34f053699c1c5f7954ce0aaabb9d26fce39d032894152229d98cf64ecafc7089530073c61d9":MBEDTLS_RSA_PKCS_V15:MBEDTLS_MD_SHA256:1024:16:"e28a13548525e5f36dccb24ecb7cc332cc689dfd64012604c9c7816d72a16c3f5fcdc0e86e7c03280b1c69b586ce0cd8aec722cc73a5d3b730310bf7dfebdc77ce5d94bbc369dc18a2f7b07bd505ab0f82224aef09fdc1e5063234255e0b3c40a52e9e8ae60898eb88a766bdd788fe9493d8fd86bcdd2884d5c06216c65469e5":16:"3":"7b5fba70ec5b521638f182bcab39cec30b76e7bc017bdbd1059658a9a1db0969ab482dce32f3e9865952f0a0de0978272c951e3c015328ea3758f47029a379ab4200550fba58f11d51264878406fc717d5f7b72b3582946f16a7e5314a220881fc820f7d29949710273421533d8ac0a449dc6d0fd1a21c22444edd1c0d5b44d3":0
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 08:25:14 -05:00

Minor: once is enough.

Minor: once is enough.
mpg (Migrated from github.com) reviewed 2019-09-12 03:16:46 -05:00
mpg (Migrated from github.com) commented 2019-09-12 03:16:46 -05:00

Oh, nice, I was looking at 185 just last week, and I'm glad it's been merged in the meantime.

Oh, nice, I was looking at 185 just last week, and I'm glad it's been merged in the meantime.
piotr-now (Migrated from github.com) reviewed 2019-12-17 05:16:45 -06:00
@ -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.
piotr-now (Migrated from github.com) commented 2019-12-16 10:00:47 -06:00

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_SHA384 in to for example MBEDTLS_SHA512_WITH_SHA384. MBEDTLS_SHA512_WITH_SHA384 points more natural that there is some connection between SHA512 and SHA384. The SHA512_NO_SHA384 was not clearly at the beginning for me.

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_SHA384` in to for example `MBEDTLS_SHA512_WITH_SHA384`. `MBEDTLS_SHA512_WITH_SHA384` points 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;
#endif
piotr-now (Migrated from github.com) commented 2019-12-17 05:14:09 -06:00

There 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 like MBEDTLS_SHA512_WITH_SHA384

There 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 like `MBEDTLS_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. */
piotr-now (Migrated from github.com) commented 2019-12-17 04:05:47 -06:00

I would change int is384; in to char is384; or even bool to keep good practice with structure padding

I would change `int is384;` in to `char is384;` or even `bool` to keep good practice with structure padding
mpg (Migrated from github.com) reviewed 2019-12-17 05:33:21 -06:00
@ -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.
mpg (Migrated from github.com) commented 2019-12-17 05:33:21 -06:00

Regarding NO vs WITH, I agree in principle, but unfortunately there's a backwards-compatibility issue. Currently people who use a custom config.h can just define MBEDTLS_SHA512_C and they get SHA-384 as well. If make SHA-384 available only if MBEDTLS_SHA512_WITH_SHA384 is defined as well, it means they have to change their code (namely, their config.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 WITH option rather than a NO option. But in the meantime if we want to merge for a 2.x release, I'm afraid a NO option is our only choice.

What we could to however, is to translate it to a WITH option internally, so that we at least avoid the readability issue inside the .c file.

Regarding `NO` vs `WITH`, I agree in principle, but unfortunately there's a backwards-compatibility issue. Currently people who use a custom `config.h` can just define `MBEDTLS_SHA512_C` and they get SHA-384 as well. If make SHA-384 available only if `MBEDTLS_SHA512_WITH_SHA384` is defined as well, it means they have to change their code (namely, their `config.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 `WITH` option rather than a `NO` option. But in the meantime if we want to merge for a 2.x release, I'm afraid a `NO` option is our only choice. What we could to however, is to translate it to a `WITH` option internally, so that we at least avoid the readability issue inside the `.c` file.
piotr-now (Migrated from github.com) reviewed 2019-12-17 07:35:33 -06:00
@ -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.
piotr-now (Migrated from github.com) commented 2019-12-17 07:35:22 -06:00

It makes sense to me. Thanks for explanation

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;
#endif
piotr-now (Migrated from github.com) commented 2019-12-17 06:26:57 -06:00

Whether mbedtls_sha384_info shouldn't be excluded in md.c also?

Whether `mbedtls_sha384_info` shouldn't be excluded in md.c also?
mpg (Migrated from github.com) reviewed 2020-01-06 03:23:34 -06:00
@ -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. */
mpg (Migrated from github.com) commented 2020-01-06 03:23:34 -06:00

That would be an ABI change, so I'd rather not do it here.

That would be an ABI change, so I'd rather not do it here.
mpg commented 2020-01-06 05:22:45 -06:00 (Migrated from github.com)

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.

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.
mpg commented 2020-01-13 18:43:53 -06:00 (Migrated from github.com)

@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.

@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.
piotr-now (Migrated from github.com) reviewed 2020-01-14 10:01:20 -06:00
gilles-peskine-arm (Migrated from github.com) requested changes 2020-01-22 04:00:15 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

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.

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](https://github.com/ARMmbed/mbed-crypto/issues/351) so this PR doesn't need to do anything about it.
@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,
*
gilles-peskine-arm (Migrated from github.com) commented 2020-01-22 03:44:36 -06:00

Minor: \c MBEDTLS_SHA512_NO_SHA384 (and likewise on other functions)

Minor: `\c MBEDTLS_SHA512_NO_SHA384` (and likewise on other functions)
gilles-peskine-arm (Migrated from github.com) commented 2020-01-22 03:45:03 -06:00

Minor: #MBEDTLS_ERR_SHA512_BAD_INPUT_DATA (and likewise on other functions)

Minor: `#MBEDTLS_ERR_SHA512_BAD_INPUT_DATA` (and likewise on other functions)
gilles-peskine-arm (Migrated from github.com) commented 2020-01-22 03:46:42 -06:00

This is correct but inconsistent. I'd prefer to either add #if around the other cases or keep this one unconditional.

This is correct but inconsistent. I'd prefer to either add `#if` around 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 }
gilles-peskine-arm (Migrated from github.com) commented 2020-01-22 03:48:05 -06:00

It's called ARRAY_LENGTH in library/psa_crypto.c and tests/suites/helpers.function. Please keep the names consistent.

It's called `ARRAY_LENGTH` in `library/psa_crypto.c` and `tests/suites/helpers.function`. Please keep the names consistent.
mpg (Migrated from github.com) reviewed 2020-01-22 04:08:26 -06:00
@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,
*
mpg (Migrated from github.com) commented 2020-01-22 04:08:25 -06:00

I assume you mean #MBEDTLS_SHA512_NO_SHA384? Or is there a subtlety I'm missing?

I assume you mean `#MBEDTLS_SHA512_NO_SHA384`? Or is there a subtlety I'm missing?
gilles-peskine-arm (Migrated from github.com) reviewed 2020-01-22 04:14:52 -06:00
@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,
*
gilles-peskine-arm (Migrated from github.com) commented 2020-01-22 04:14:52 -06:00

I meant \c, because when MBEDTLS_SHA512_NO_SHA384 is not defined, the # version would render with the #.

I meant `\c`, because when `MBEDTLS_SHA512_NO_SHA384` is not defined, the `#` version would render with the `#`.
mpg (Migrated from github.com) reviewed 2020-01-22 05:05:22 -06:00
mpg (Migrated from github.com) commented 2020-01-22 05:05:21 -06:00

I'm not sure I understand what the inconsistency is. Could you clarify?

I'm not sure I understand what the inconsistency is. Could you clarify?
gilles-peskine-arm (Migrated from github.com) reviewed 2020-01-22 05:07:16 -06:00
gilles-peskine-arm (Migrated from github.com) commented 2020-01-22 05:07:16 -06:00

To be consistent, case PSA_ALG_SHA_512 should be under #if defined(MBEDTLS_SHA512_C), etc.

To be consistent, `case PSA_ALG_SHA_512` should be under `#if defined(MBEDTLS_SHA512_C)`, etc.
mpg (Migrated from github.com) reviewed 2020-01-24 03:28:58 -06:00
@ -101,7 +103,11 @@ void mbedtls_sha512_clone( mbedtls_sha512_context *dst,
*
mpg (Migrated from github.com) commented 2020-01-24 03:28:58 -06:00

Oh, good point. Never noticed that.

Oh, good point. Never noticed that.
mpg commented 2020-01-24 04:03:30 -06:00 (Migrated from github.com)

@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!

@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!
mpg commented 2020-01-27 03:32:21 -06:00 (Migrated from github.com)

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.

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.
gilles-peskine-arm (Migrated from github.com) requested changes 2020-01-28 11:25:27 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

One minor issue I missed earlier, other than that LGTM

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 (Migrated from github.com) commented 2020-01-28 11:20:57 -06:00
#define ARRAY_LENGTH( a )    ( sizeof( a ) / sizeof( ( a )[0] ) )
```suggestion #define ARRAY_LENGTH( a ) ( sizeof( a ) / sizeof( ( a )[0] ) ) ```
mpg commented 2020-01-29 02:49:03 -06:00 (Migrated from github.com)

@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!

@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!
gilles-peskine-arm (Migrated from github.com) approved these changes 2020-01-29 02:51:45 -06:00
piotr-now commented 2020-01-30 03:18:28 -06:00 (Migrated from github.com)

LGTM

LGTM
piotr-now (Migrated from github.com) approved these changes 2020-01-30 03:18:48 -06:00
mpg commented 2020-01-30 03:31:50 -06:00 (Migrated from github.com)

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.

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.
Sign in to join this conversation.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!179
No description provided.