Skip to content
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.

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

[cryptotest] Add P-384 targets to cryptotest. #25510

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

jadephilipoom
Copy link
Contributor

These were initially excluded because we didn't support P-384 in cryptolib yet, but now that P-384 support is present we should test it. Note that there were some tests with SHA-256 in the original test suite that was commented out, but I removed them: since cryptolib doesn't let you use a hash function with collision resistance weaker than the curve security level these will always fail.

Verified locally that the ECDH and ECDSA targets pass on FPGA.

Follow-up after noticing it was missing while making some changes, see #25391

These were initially excluded because we didn't support P-384 in cryptolib yet,
but now that P-384 support is present we should test it.

Verified locally that the ECDH and ECDSA targets pass on FPGA.

Signed-off-by: Jade Philipoom <[email protected]>
@jadephilipoom jadephilipoom added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Dec 4, 2024
@jadephilipoom jadephilipoom requested a review from a team as a code owner December 4, 2024 13:37
@moidx moidx removed the request for review from a team December 4, 2024 16:06
memcpy(private_key_masked.share1, uj_private_key.d1, kP256ScalarBytes);
private_key_masked_raw = (uint32_t *)&private_key_masked;
private_keyblob_length = sizeof(private_key_masked);
memset(private_key_masked_p256.share0, 0, kP256MaskedScalarShareBytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to write a function that takes the same arguments as memset but that fills the buffer with random data instead of 0s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and in fact we already have one:

void hardened_memshred(uint32_t *dest, size_t word_len);

It's slightly different than memset because it only works with aligned pointers, but it's good practice to handle secret values in words rather than bytes anyway.

Copy link
Contributor

@milesdai milesdai left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for resolving this!

@jadephilipoom jadephilipoom merged commit 9b760eb into lowRISC:master Dec 5, 2024
39 checks passed
@jadephilipoom jadephilipoom deleted the cryptotest-p384 branch December 5, 2024 12:08
Copy link

github-actions bot commented Dec 5, 2024

Backport failed for earlgrey_1.0.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-25510-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-25510-to-earlgrey_1.0.0
git switch --create backport-25510-to-earlgrey_1.0.0
git cherry-pick -x 777277ff68fce7db745d563693a95661dd9f108f

@github-actions github-actions bot added the Manually CherryPick This PR should be manually cherry picked. label Dec 5, 2024
@jadephilipoom
Copy link
Contributor Author

Backport depends on #25391 which depends on #25390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 Manually CherryPick This PR should be manually cherry picked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants