Add return checks on SHA3 functions in ML-KEM #1859
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1859 +/- ##
==========================================
- Coverage 78.46% 78.45% -0.01%
==========================================
Files 585 585
Lines 99458 99458
Branches 14238 14236 -2
==========================================
- Hits 78036 78033 -3
- Misses 20786 20788 +2
- Partials 636 637 +1 ☔ View full report in Codecov by Sentry. |
| if (SHAKE_Init(ctx, SHAKE128_BLOCKSIZE) == 0) { | ||
| return; | ||
| } | ||
| if (SHA3_Update(ctx, extseed, sizeof(extseed)) == 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
While this is checking the return code now, is this safe to do? If SHAKE_Init fails can ML-KEM continue? Options I see:
- Make SHAKE_Init void (if it can be void and never fails)
- Make kyber_shake128_absorb non-void and bubble up the failure
- If SHAKE_init fails abort
My concern is if kyber_shake128_absorb silently fails and continues AWS-LC will be in an inconsistent state and generate an invalid result but callers will think AWS-LC succeeded.
There was a problem hiding this comment.
From my understanding, SHA3_Update is the one creating the issue.
- We could make it void, however, it's success/failure is based on the execution of
SHA3_Absorb, which returns the number of bytes still to be processed. Called fromSHA3_Update,SHA3_Absorbis called to process the leftover data in the buffer (if any from previous calls toSHA3_Update). If the data is enough to fill at least an entire block,SHA3_Absorbwill be called first on a single block. In case the block is not completely processed, the return value ofSHA3_Absorbis indicating an error. This case, indeed, should never happen (even ifKeccakis not executed correctly, it is a void function, so the incorrect data will be stored independently, indicating the success ofSHA3_Absorb, thus, ofSHA3_Update). So, we could makeSHA3_Updatea void function, however, we should not check the returns that are called from it either, and we cannot makeSHA3_Absorbvoid. - In the ML-KEM there are no checks for the xof success.
- We could abort, however, would that not impact the flow when two parties are executing the kem?
From my understanding, the tests on SHA3, SHAKE are checking the correctness of SHA3/SHAKE functionality. Since we only use the SHA3/SHAKE tested functions, we could assume they are executing correctly if SHA3/SHAKE tests pass.
Issues:
Resolves #P155314914
Description of changes:
Add checks on the return values of
{SHA3,SHAKE}_{Init,Update,Final}andSHAKE256when consumed by ML-KEM.Call-outs:
Coverity Defect is generated when
SHA3_Updatereturn code is not checked. Checks on the return values are added to all FIPS202 function calls.Testing:
./crypto/crypto_testBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.