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

mpg · 2019-07-17T14:53:14Z

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 · 2019-07-25T17:16:22Z

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

mpg · 2019-09-11T07:20:27Z

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 · 2019-09-11T08:11:02Z

@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

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

library/md_wrap.c

include/mbedtls/config.h

include/mbedtls/sha512.h

library/psa_crypto.c

library/sha512.c

tests/suites/test_suite_pkparse.data

tests/suites/test_suite_rsa.data

include/mbedtls/config.h

piotr-now · 2019-12-17T10:05:47Z

include/mbedtls/sha512.h

    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 to char is384; or even bool to keep good practice with structure padding

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

piotr-now · 2019-12-17T11:14:09Z

include/mbedtls/md_internal.h

 #if defined(MBEDTLS_SHA512_C)
+#if !defined(MBEDTLS_SHA512_NO_SHA384)
 extern const mbedtls_md_info_t mbedtls_sha384_info;
+#endif


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

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

Saves 140 bytes on sha512.o, measured with: arm-none-eabi-gcc -Wall -Wextra -Iinclude -Os -mcpu=cortex-m0plus -mthumb -c library/sha512.c && arm-none-eabi-size sha512.o arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q2-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907] Todo: - fix selftest - fix dependencies in test suites - implement in MD layer

mpg · 2020-01-06T11:22:45Z

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.

- Always put MBEDTLS_SHA512_NO_SHA384 immediately after MBEDTLS_SHA512_C - Remove duplicate occurrences of MBEDTLS_SHA512_NO_SHA384 on the same line

mpg · 2020-01-14T00:43:53Z

@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

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.

include/mbedtls/sha512.h

library/psa_crypto.c

library/sha512.c

Other cases in this switch statement aren't guarded either.

Other modules have similar internal macros using _LENGTH in the name.

mpg · 2020-01-24T10:03:30Z

@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 · 2020-01-27T09:32:21Z

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

One minor issue I missed earlier, other than that LGTM

gilles-peskine-arm · 2020-01-28T17:20:57Z

library/sha512.c

 };

-#define ARRAY_LEN(a)    ( sizeof( a ) / sizeof( a[0] ) )
+#define ARRAY_LENGTH(a)    ( sizeof( a ) / sizeof( a[0] ) )


Suggested change

#define ARRAY_LENGTH(a) ( sizeof( a ) / sizeof( a[0] ) )

#define ARRAY_LENGTH( a ) ( sizeof( a ) / sizeof( ( a )[0] ) )

mpg · 2020-01-29T08:49:03Z

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

piotr-now · 2020-01-30T09:18:28Z

LGTM

mpg · 2020-01-30T09:31:50Z

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.

Previously in d875285: * ARMmbed#333: Streamline PSA key type encodings: prepare * ARMmbed#323: Initialise return values to an error Previously in dbcb442: * ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY * ARMmbed#334: Fix some pylint warnings Previously in ceceedb: * ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4 * ARMmbed#354: Fix incrementing pointer instead of value In this commit: * ARMmbed#349: Fix minor defects found by Coverity * ARMmbed#179: Add option to build SHA-512 without SHA-384 * ARMmbed#327: Implement psa_hash_compute and psa_hash_compare * ARMmbed#330: Streamline PSA key type and curve encodings

mpg added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. labels Jul 17, 2019

Patater added the needs: work The pull request needs rework before it can be merged. label Sep 10, 2019

mpg removed the needs: work The pull request needs rework before it can be merged. label Sep 11, 2019

mpg requested a review from gilles-peskine-arm September 11, 2019 08:10

gilles-peskine-arm requested changes Sep 11, 2019

View reviewed changes

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 11, 2019

piotr-now self-requested a review December 16, 2019 15:00

piotr-now reviewed Dec 17, 2019

View reviewed changes

mpg added 6 commits January 6, 2020 11:40

Declare new config.h option MBEDTLS_SHA512_NO_SHA384

ad6cb11

Adapt sha512 selftest to NO_SHA384 option

39ea19a

Declare test dependencies on !SHA512_NO_SHA384

6ba5a3f

Implement NO_SHA384 in MD layer and PSA

d602084

Adjust depends-hashes.pl to test NO_SHA384 as well

20f236d

mpg added 6 commits January 7, 2020 10:11

Clarify documentation on is384.

663ee20

Make more code paths conditional in psa_crypto.c

792b16d

Make optional parameter validation more precise

0b9db44

Improve readability of macro in selftest

2d88549

Improve readability of test dependencies

86a39bd

- Always put MBEDTLS_SHA512_NO_SHA384 immediately after MBEDTLS_SHA512_C - Remove duplicate occurrences of MBEDTLS_SHA512_NO_SHA384 on the same line

Make SHA512_NO_SHA384 depend on SHA512_C

1e6fb01

piotr-now previously approved these changes Jan 14, 2020

View reviewed changes

gilles-peskine-arm requested changes Jan 22, 2020

View reviewed changes

include/mbedtls/sha512.h Outdated Show resolved Hide resolved

include/mbedtls/sha512.h Outdated Show resolved Hide resolved

library/psa_crypto.c Outdated Show resolved Hide resolved

library/sha512.c Outdated Show resolved Hide resolved

Improve doxygen formatting

3a3b5c7

mpg added 2 commits January 24, 2020 10:59

Remove preprocessor directive for consistency

b7f7092

Other cases in this switch statement aren't guarded either.

Rename internal macro for consistency

2b9b780

Other modules have similar internal macros using _LENGTH in the name.

mpg dismissed piotr-now’s stale review via 2b9b780 January 24, 2020 10:02

mpg added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Jan 24, 2020

mpg requested review from gilles-peskine-arm and piotr-now January 24, 2020 10:03

mpg mentioned this pull request Jan 28, 2020

Implement psa_hash_compute and psa_hash_compare #327

Merged

gilles-peskine-arm requested changes Jan 28, 2020

View reviewed changes

Fix some whitespace issues

74ca84a

mpg requested a review from gilles-peskine-arm January 29, 2020 08:49

gilles-peskine-arm approved these changes Jan 29, 2020

View reviewed changes

piotr-now approved these changes Jan 30, 2020

View reviewed changes

mpg added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Jan 30, 2020

mpg merged commit f712e16 into ARMmbed:development Jan 30, 2020

	#define ARRAY_LENGTH(a) ( sizeof( a ) / sizeof( a[0] ) )
	#define ARRAY_LENGTH( a ) ( sizeof( a ) / sizeof( ( a )[0] ) )

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

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

Uh oh!

Conversation

mpg commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Jul 25, 2019

Uh oh!

mpg commented Sep 11, 2019

Uh oh!

mpg commented Sep 11, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

piotr-now Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

mpg Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piotr-now Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

piotr-now Dec 17, 2019

Choose a reason for hiding this comment

Uh oh!

mpg commented Jan 6, 2020

Uh oh!

mpg commented Jan 14, 2020

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpg commented Jan 24, 2020

Uh oh!

mpg commented Jan 27, 2020

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

mpg commented Jan 29, 2020

Uh oh!

piotr-now commented Jan 30, 2020

Uh oh!

mpg commented Jan 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mpg commented Jul 17, 2019 •

edited

Loading

mpg Jan 6, 2020 •

edited

Loading