SHA-3 support #5820

polhenarejos · 2022-05-08T23:36:25Z

Description

It adds support for SHA-3 with 224, 256, 384 and 512.

Splitted from #5800.

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

Any additional information that could be of interest

Todos

Tests
Documentation
Changelog updated
Make sure that the vulnerability from the reference implementation is not introduced (see https://eprint.iacr.org/2023/331.pdf)

Signed-off-by: Pol Henarejos <[email protected]>

include/mbedtls/mbedtls_config.h

d3zd3z

Just a minor formatting issue, and potential clarification on the meaning of the 'olen' parameter. This may be documented better once the SHAKE variants come in.

include/mbedtls/error.h

include/mbedtls/sha3.h

Signed-off-by: Pol Henarejos <[email protected]>

mpg · 2022-05-20T09:52:51Z

For reference, there was some earlier work in this area: #1549 (a continuation of #479). We're closing the previous PRs and keeping this one as it's fresh and active while the previous one now has numerous conflicts, but the previous PR can be used as a comparison point. In particular, #1549 did some work on tests, so we should make the the coverage in this PR is at least as good, and if not, import relevant tests from the previous PR to complete coverage.

polhenarejos · 2022-05-20T14:18:17Z

Added SHA3 to MD to support HMAC with SHA3. Also added tests (for test suite) from #1549 but no self-tests yet. Are self-tests necessary despite of test suite? Seems test suites are more complete.

d3zd3z · 2022-05-20T17:08:28Z

@polhenarejos wrote:

Are self-tests necessary despite of test suite? Seems test suites are more complete.

Some types of compliance will require self tests (e.g. FIPS-140 and friends), which require confirmation of the code actually running in the environment (this is supposed to help detect modified code and similar). It can be useful to be able to run a quick test at run time to make sure the operations are as expected.

d3zd3z

In addition, a few of the newer commits have a fairly long first line (git convention is to have a <70 character summary line, a blank line, and then a longer description). This makes commands like 'git shortlog' and 'git log --oneline' more useful. For the "olen parameter shall..." commit, perhaps a description line that is something like "sha3: Fix olen parameter handling", a blank line, and then the existing text refilled to 75 columns or so.

For "olen = 0 is not allowed" just put a blank line after that line

For "Fix when reusing..." probably put the blank line before the value in parens, remove the parens, and make that a complete sentence.

"Fix travis build" could probably use a better description, since the issue being fixed is actually a bit more than just for travis. The enum values shouldn't be conditionalized, which happens to let Travis work.

d3zd3z · 2022-05-20T17:13:10Z

library/sha3.c

+            break;
+    }
+
+    if( p == NULL || p->id == MBEDTLS_SHA3_NONE )


Is there any reason to still have the p == NULL check? It is always assigned above?

Not really. It is a paranoid check I usually do before -> operator.

Sorry, just happened to see this, and wanted to point out some static analysis tools like Coverity will very likely warn about this. Not only cannot p be NULL (because it points to a global variable), but also it is dereferenced before the check. So even as a "just in case", this would do more harm than good.

Sounds logic. I pushed a change.

Signed-off-by: Pol Henarejos <[email protected]>

d3zd3z

Looks good.

library/sha3.c

Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Pol Henarejos <[email protected]>

Signed-off-by: Pol Henarejos <[email protected]>

daverodgman

Mostly LGTM - most of my comments are pretty minor.

Testing for bad arguments, and tidying up the public interface are the main ones IMO.

I had some time to kill on the plane when I reviewed this so also did some performance optimisation. I'll push that in a separate PR as soon as this gets merged so as not to hold this one up.

daverodgman · 2023-05-30T10:17:16Z

ChangeLog.d/sha3.txt

@@ -0,0 +1,3 @@
+Features
+    * Add SHA3 family hash functions.


Minor: please be specific about which variants have been added. Please write SHA3 as SHA-3 (lots of instances of this).

daverodgman · 2023-05-30T10:19:42Z

library/sha3.c

+int mbedtls_sha3_starts(mbedtls_sha3_context *ctx, mbedtls_sha3_id id)
+{
+    mbedtls_sha3_family_functions *p = NULL;
+    if (ctx == NULL) {


I agree with @d3zd3z - we have quite a lot of NULL checks here that aren't needed. I suggest being consistent with the level of null checking in the other hash implementations.

daverodgman · 2023-05-30T10:20:28Z

include/mbedtls/sha3.h

+/**
+ * \brief          Checkup routine for the algorithms implemented
+ *                 by this module: SHA3-224, SHA3-256, SHA3-384, SHA3-512,
+ *                 SHAKE128, SHAKE256, cSHAKE128 and cSHAKE256.


Since this doesn't implement SHAKE please update this.

daverodgman · 2023-05-30T10:21:45Z

library/sha3.c

+        return MBEDTLS_ERR_SHA3_BAD_INPUT_DATA;
+    }
+
+    if (ilen == 0 || input == NULL) {


The check for ilen == 0 can be removed, it's covered by the while condition

daverodgman · 2023-05-30T10:22:34Z

library/sha3.c

+{
+    uint64_t lane[5];
+    uint64_t *s = ctx->state;
+    int i;


Nit: normally I'd prefer to just initialise this in the for loop where it's used.

daverodgman · 2023-05-30T10:27:41Z

include/mbedtls/sha3.h

+ *
+ *                 The structure is used SHA-3 checksum calculations.
+ */
+typedef struct mbedtls_sha3_context {


Suggested change

typedef struct mbedtls_sha3_context {

typedef struct {

Can simplify this a little I think

daverodgman · 2023-05-30T10:28:29Z

tests/suites/test_suite_shax.function

@@ -148,3 +149,142 @@ void sha512_selftest()
    TEST_EQUAL(mbedtls_sha512_self_test(1), 0);
 }
 /* END_CASE */
+


Testing mostly looks good, but we don't have any tests for bad arguments - e.g., passing an invalid hash id, or olen too short.

daverodgman · 2023-05-30T10:32:11Z

include/mbedtls/sha3.h

+
+    uint16_t r;
+    uint16_t olen;
+    uint8_t xor_byte;


I suggest removing xor_byte and simply using a #define in sha3.c, since this is the same for all implemented algorithms.

daverodgman · 2023-05-30T10:33:35Z

include/mbedtls/sha3.h

+
+    uint16_t r;
+    uint16_t olen;
+    uint8_t xor_byte;


I suggest removing xor_byte (replace with a #define in sha3.c), id (it's not used after init), and r (not used after init).

I put xor_byte as a variable thinking on SHAKE, CSHAKE and KMAC, which all use different xor_byte. Another approach can be a lookup table based on id.

daverodgman · 2023-05-30T10:35:02Z

include/mbedtls/sha3.h

+ *                 The structure is used SHA-3 checksum calculations.
+ */
+typedef struct mbedtls_sha3_context {
+    uint64_t state[25];


Everything in this struct should use MBEDTLS_PRIVATE

daverodgman

Mostly LGTM - most of my comments are pretty minor.

Testing for bad arguments, and tidying up the public interface are the main ones IMO.

I had some time to kill on the plane when I reviewed this so also did some performance optimisation. I'll push that in a separate PR as soon as this gets merged so as not to hold this one up.

daverodgman · 2023-06-07T21:10:16Z

Merged via #7708

polhenarejos added 2 commits May 9, 2022 01:04

Add SHA-3 module.

0cd1f1c

Signed-off-by: Pol Henarejos <[email protected]>

Add test vectors (from NIST) for SHA-3.

f645705

Signed-off-by: Pol Henarejos <[email protected]>

This was referenced May 8, 2022

SHA-3 SHAKE128 and SHAKE256 support #5821

Open

EdDSA support (Ed25519 and Ed448 curves) with Pure, Context and Prehash types + SHA3 (SHA3, SHAKE, CSHAKE and KMAC) #5800

Closed

polhenarejos changed the title ~~SHA-3 support.~~ SHA-3 support May 9, 2022

polhenarejos mentioned this pull request May 9, 2022

Add support to Ed448 in EdDSA #5824

Open

3 tasks

tom-cosgrove-arm reviewed May 9, 2022

View reviewed changes

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

tom-cosgrove-arm added priority-scheduled This PR is big - it will require time to be scheduled for review and removed priority-medium Medium priority - this can be reviewed as time permits labels May 9, 2022

daverodgman removed the Community label May 13, 2022

bensze01 added the Community label May 13, 2022

daverodgman removed the Community label May 13, 2022

bensze01 added the Community label May 13, 2022

daverodgman removed the Community label May 13, 2022

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label May 13, 2022

d3zd3z reviewed May 16, 2022

View reviewed changes

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

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

d3zd3z mentioned this pull request May 16, 2022

SHA-3 cSHAKE128 and cSHAKE256 support #5822

Open

3 tasks

Aligning spaces

90f360e

Signed-off-by: Pol Henarejos <[email protected]>

mpg mentioned this pull request May 20, 2022

SHA-3 and SHAKE #1549

Closed

d3zd3z reviewed May 20, 2022

View reviewed changes

Add sha3 streaming and reuse tests.

90f803c

Signed-off-by: Pol Henarejos <[email protected]>

polhenarejos force-pushed the sha3 branch from 74d4610 to 90f803c Compare May 20, 2022 18:50

Fix dependency of HMAC-SHA384 tests.

e884fd7

Signed-off-by: Pol Henarejos <[email protected]>

d3zd3z reviewed May 26, 2022

View reviewed changes

gilles-peskine-arm reviewed Oct 12, 2022

View reviewed changes

library/sha3.c Outdated Show resolved Hide resolved

polhenarejos and others added 2 commits October 13, 2022 08:28

Merge branch 'Mbed-TLS:development' into sha3

c9754c3

Update library/sha3.c

39fb1d5

Co-authored-by: Gilles Peskine <[email protected]> Signed-off-by: Pol Henarejos <[email protected]>

polhenarejos force-pushed the sha3 branch 2 times, most recently from 45003eb to 39fb1d5 Compare February 7, 2023 18:49

polhenarejos added 7 commits February 7, 2023 19:55

Merge branch 'development' into sha3

4e74733

Signed-off-by: Pol Henarejos <[email protected]>

Bad merge. These tests are not used.

a391a9e

Signed-off-by: Pol Henarejos <[email protected]>

Style.

a677928

Signed-off-by: Pol Henarejos <[email protected]>

SHA3 cannot be tested alone, as ENTROPY_C needs also SHA256 enabled.

aa426e0

Signed-off-by: Pol Henarejos <[email protected]>

Correct style.

b3b220c

Signed-off-by: Pol Henarejos <[email protected]>

Merge branch 'development' into sha3

f61d6c0

Fix md test with sha3.

0004a86

Signed-off-by: Pol Henarejos <[email protected]>

polhenarejos added 2 commits May 5, 2023 16:01

Merge branch 'development' into sha3

d06c6fc

Signed-off-by: Pol Henarejos <[email protected]>

Fix coding style.

2d80769

Signed-off-by: Pol Henarejos <[email protected]>

daverodgman reviewed May 30, 2023

View reviewed changes

daverodgman added needs-work historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels May 30, 2023

daverodgman mentioned this pull request Jun 7, 2023

SHA-3 support (+ minor fixes & perf) #7708

Merged

3 tasks

daverodgman merged commit 2d80769 into Mbed-TLS:development Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SHA-3 support #5820

SHA-3 support #5820

polhenarejos commented May 8, 2022 •

edited by yanesca

Loading

d3zd3z left a comment

mpg commented May 20, 2022

polhenarejos commented May 20, 2022

d3zd3z commented May 20, 2022

d3zd3z left a comment

d3zd3z May 20, 2022

polhenarejos May 20, 2022

mhaeuser Feb 4, 2023

polhenarejos Feb 6, 2023

d3zd3z left a comment

daverodgman left a comment

daverodgman May 30, 2023

daverodgman May 30, 2023

daverodgman May 30, 2023

daverodgman May 30, 2023

daverodgman May 30, 2023

daverodgman May 30, 2023

daverodgman May 30, 2023

daverodgman May 30, 2023 •

edited

Loading

daverodgman May 30, 2023

polhenarejos Jun 9, 2023

daverodgman May 30, 2023

daverodgman left a comment

daverodgman commented Jun 7, 2023

SHA-3 support #5820

SHA-3 support #5820

Conversation

polhenarejos commented May 8, 2022 • edited by yanesca Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

d3zd3z left a comment

Choose a reason for hiding this comment

mpg commented May 20, 2022

polhenarejos commented May 20, 2022

d3zd3z commented May 20, 2022

d3zd3z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3zd3z left a comment

Choose a reason for hiding this comment

daverodgman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daverodgman May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daverodgman left a comment

Choose a reason for hiding this comment

daverodgman commented Jun 7, 2023

polhenarejos commented May 8, 2022 •

edited by yanesca

Loading

daverodgman May 30, 2023 •

edited

Loading