[Silabs] SPAKE2+ verifier fixed for Si917.#72365
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the safety of SPAKE2+ operations in CHIPCryptoPALTinyCrypt.cpp by validating input lengths and using them dynamically in uECC_vli_bytesToNative. The review feedback suggests further strengthening this input validation by checking for null pointers and ensuring that the input lengths are multiples of sizeof(uECC_word_t) to prevent silent truncation during conversion.
| VerifyOrReturnError(in_len <= sizeof(tmp), CHIP_ERROR_INVALID_ARGUMENT); | ||
|
|
||
| uECC_vli_bytesToNative(tmp, in, static_cast<int>(in_len)); |
There was a problem hiding this comment.
To prevent potential null pointer dereferences and ensure cryptographic correctness, we should defensively check that in is not null. Additionally, uECC_vli_bytesToNative expects the input length to be a multiple of sizeof(uECC_word_t). If in_len is not a multiple, the function will silently ignore the most significant bytes due to integer division truncation (size / sizeof(uECC_word_t)). Adding a check for this ensures robust input validation.
VerifyOrReturnError(in != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(in_len <= sizeof(tmp), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError((in_len % sizeof(uECC_word_t)) == 0, CHIP_ERROR_INVALID_ARGUMENT);
uECC_vli_bytesToNative(tmp, in, static_cast<int>(in_len));| VerifyOrExit(w1sin_len <= sizeof(tmp), error = CHIP_ERROR_INVALID_ARGUMENT); | ||
|
|
||
| uECC_vli_bytesToNative(tmp, w1sin, NUM_ECC_BYTES); | ||
| uECC_vli_bytesToNative(tmp, w1sin, static_cast<int>(w1sin_len)); |
There was a problem hiding this comment.
To prevent potential null pointer dereferences and ensure cryptographic correctness, we should defensively check that w1sin is not null. Additionally, uECC_vli_bytesToNative expects the input length to be a multiple of sizeof(uECC_word_t). If w1sin_len is not a multiple, the function will silently ignore the most significant bytes due to integer division truncation. Adding a check for this ensures robust input validation.
VerifyOrExit(w1sin != nullptr, error = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(w1sin_len <= sizeof(tmp), error = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit((w1sin_len % sizeof(uECC_word_t)) == 0, error = CHIP_ERROR_INVALID_ARGUMENT);
uECC_vli_bytesToNative(tmp, w1sin, static_cast<int>(w1sin_len));| // Warning: SPAKE2+ Generate uses `kSpake2p_WS_Length = kP256_FE_Length + 8` | ||
| // (40-byte) > NUM_ECC_BYTES (32-byte) |
There was a problem hiding this comment.
I don't feel like this comment is telling me a whole lot. Especially since we are removing NUM_ECC_BYTES usage below.
Either we add a bit of context or we remove it.
| // Warning: SPAKE2+ Generate uses `kSpake2p_WS_Length = kP256_FE_Length + 8` | ||
| // (40-byte) > NUM_ECC_BYTES (32-byte) |
There was a problem hiding this comment.
Same about this comment, feels like it explain the changes in the PR, but for someone reading the code outside of this PR, it doesn't convey much information.
lpbeliveau-silabs
left a comment
lpbeliveau-silabs
left a comment
There was a problem hiding this comment.
Approved, only nit is the comments, I don't think this warning is understandable as is. I wouldn't know how to take action on it. Maybe add some detail in the method's brief or remove the comments altogether.
|
|
||
| VerifyOrReturnError(in_len <= sizeof(tmp), CHIP_ERROR_INVALID_ARGUMENT); | ||
|
|
||
| uECC_vli_bytesToNative(tmp, in, static_cast<int>(in_len)); |
There was a problem hiding this comment.
+1 to the AI comment, nullptr checks appear to be missing in the methods in this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #72365 +/- ##
=======================================
Coverage 55.57% 55.57%
=======================================
Files 1630 1630
Lines 111220 111220
Branches 13408 13408
=======================================
Hits 61812 61812
Misses 49408 49408 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #72365: Size comparison from b578401 to fd4576f Full report (35 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, psoc6, qpg, realtek, stm32, telink)
|
2 participants
Summary
The SPAKE2+ algorithm uses
kSpake2p_WS_Length = kP256_FE_Length + 8(40 bytes), but there are two methods inCHIPCryptoPALTinyCrypt.cppthat assumes 32 byte inputs:Spake2p_P256_SHA256_HKDF_HMAC::ComputeL, andSpake2p_P256_SHA256_HKDF_HMAC::FELoad, truncating the actual value. This causes an incorrect outputCrypto::Spake2pVerifier::Generate.Related issues
MATTER-5716.
Testing
SPAKE2+ Verifier generated in BRD4338A (Si917) using the inputs from
TestOnlyCommissionableDataProvider.Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See:
Pull Request Guidelines