Skip to content

[Silabs] SPAKE2+ verifier fixed for Si917. #72365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rcasallas-silabs wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −7
Open

[Silabs] SPAKE2+ verifier fixed for Si917. #72365

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions src/platform/silabs/SiWx/CHIPCryptoPALTinyCrypt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,8 +878,13 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::FELoad(const uint8_t * in, size_t in_l
{
CHIP_ERROR error = CHIP_NO_ERROR;

// Warning: SPAKE2+ Generate uses `kSpake2p_WS_Length = kP256_FE_Length + 8`
// (40-byte) > NUM_ECC_BYTES (32-byte)
Comment on lines +881 to +882
Copy link
Copy Markdown
Contributor

@lpbeliveau-silabs lpbeliveau-silabs Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uECC_word_t tmp[2 * NUM_ECC_WORDS] = { 0 };
uECC_vli_bytesToNative(tmp, in, NUM_ECC_BYTES);

VerifyOrReturnError(in_len <= sizeof(tmp), CHIP_ERROR_INVALID_ARGUMENT);

uECC_vli_bytesToNative(tmp, in, static_cast<int>(in_len));
Comment on lines +885 to +887
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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));

Copy link
Copy Markdown
Contributor

@lpbeliveau-silabs lpbeliveau-silabs Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the AI comment, nullptr checks appear to be missing in the methods in this PR.


uECC_vli_mmod((uECC_word_t *) fe, tmp, curve_n);

Expand Down Expand Up @@ -1003,14 +1008,17 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::PointCofactorMul(void * R)
CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::ComputeL(uint8_t * Lout, size_t * L_len, const uint8_t * w1sin, size_t w1sin_len)
{
CHIP_ERROR error = CHIP_NO_ERROR;
int result = 0;
int result = UECC_SUCCESS;

// Warning: SPAKE2+ Generate uses `kSpake2p_WS_Length = kP256_FE_Length + 8`
// (40-byte) > NUM_ECC_BYTES (32-byte)
Comment on lines +1013 to +1014
Copy link
Copy Markdown
Contributor

@lpbeliveau-silabs lpbeliveau-silabs Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uECC_word_t tmp[2 * NUM_ECC_WORDS] = { 0 };
uECC_word_t w1_bn[NUM_ECC_WORDS] = { 0 };
uECC_word_t L_tmp[2 * NUM_ECC_WORDS] = { 0 };

result = UECC_SUCCESS;
uECC_word_t tmp[2 * NUM_ECC_WORDS];
uECC_word_t w1_bn[NUM_ECC_WORDS];
uECC_word_t L_tmp[2 * NUM_ECC_WORDS];
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));
Comment on lines +1019 to +1021
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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));


uECC_vli_mmod(w1_bn, tmp, curve_n);

Expand Down
Loading