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.

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

Jump to bottom

[pentest] Add ECC256 Keygen SCA test #22190

Closed
wants to merge 1 commit into from
Closed

[pentest] Add ECC256 Keygen SCA test #22190

wants to merge 1 commit into from

Conversation

nasahlpa
Copy link
Member

@nasahlpa nasahlpa commented Mar 22, 2024 •
edited
Loading

This commit adds the ECC256 key generation side-channel penetration test to the codebase.

The host code is located in lowRISC/ot-sca#347

nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Mar 22, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
575d190 
This commit enables communication with the OTBN SCA code over uJSON.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa nasahlpa added the CherryPick:master This PR should be cherry-picked to master label Mar 22, 2024
@nasahlpa nasahlpa mentioned this pull request Mar 22, 2024
Draft
@nasahlpa nasahlpa force-pushed the ujson_otbn_port branch 2 times, most recently from a84e637 to 43310a0 Compare March 22, 2024 12:52
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Mar 22, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
d8b4905 
This commit enables communication with the OTBN SCA code over uJSON.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Mar 22, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
5f41003 
This commit enables communication with the OTBN SCA code over uJSON.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Mar 22, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
aa953b9 
This commit enables communication with the OTBN SCA code over uJSON.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Mar 26, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
5c497d1 
This commit enables communication with the OTBN SCA code over uJSON.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa nasahlpa changed the title [pentest] Port OTBN SCA code to uJSON [pentest] Add ECC256 Keygen SCA test Apr 9, 2024
@nasahlpa nasahlpa marked this pull request as ready for review April 13, 2024 09:28
@nasahlpa nasahlpa requested a review from a team as a code owner April 13, 2024 09:28
@nasahlpa nasahlpa requested review from HU90m, m-temp, vogelpi, wettermo and johannheyszl and removed request for a team and HU90m April 13, 2024 09:28
m-temp
m-temp reviewed Apr 15, 2024
View reviewed changes
Copy link
Contributor

@m-temp m-temp left a comment

Choose a reason for hiding this comment

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

I had a look at the code.
LGTM, but I'm nor very familiar with writing device code.

sw/device/tests/penetrationtests/firmware/ecc256_keygen_sca.c Outdated
*
* Adds the integer stored in source array to the integer stored in
* destination aray.
* The user needs to ensure that dest_len isenough to store the result
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: typo: "isenough"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

sw/device/tests/penetrationtests/firmware/ecc256_keygen_sca.c Outdated
*
* @param[in] dest Location of the first input array and the result.
* @param[in] source Location of the second input array.
* @param[in] dest_len Length od the dest array in bytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: typo: "od"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@nasahlpa nasahlpa force-pushed the ujson_otbn_port branch 4 times, most recently from 282a821 to b16e852 Compare April 16, 2024 15:10
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Apr 16, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
3a498da 
This commit enables communication with the OTBN SCA code over uJSON.
Currently, the ECC256 Keygen SCA test is supported.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Apr 17, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
7d0dc05 
This commit enables communication with the OTBN SCA code over uJSON.
Currently, the ECC256 Keygen SCA test is supported.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Apr 17, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
5616fce 
This commit enables communication with the OTBN SCA code over uJSON.
Currently, the ECC256 Keygen SCA test is supported.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
wettermo
wettermo reviewed Apr 17, 2024
View reviewed changes
sw/device/tests/penetrationtests/firmware/otbn_sca.c Outdated
kScaPeripheralEdn | kScaPeripheralHmac);

// Load p256 keygen from seed app into OTBN.
if (otbn_load_app(kOtbnAppP256KeyFromSeed).value != OTCRYPTO_OK.value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that this was already there in the previous version of the code, but do we need app loading here?
I mean it's done in handle_otbn_sca_ecc256_app_select anyway, and also in p256_run_keygen it's re-loaded for every iteration.
Of course it doesn't do any harm to load it anyway as default, but probably could be omitted here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, you are right. I've removed it :)

wettermo
wettermo approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@wettermo wettermo left a comment

Choose a reason for hiding this comment

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

LGTM!

@nasahlpa nasahlpa force-pushed the ujson_otbn_port branch 3 times, most recently from 6d41106 to 78c57c5 Compare April 18, 2024 08:54
nasahlpa added a commit to nasahlpa/ot-sca that referenced this pull request Apr 18, 2024
@nasahlpa
[capture] Enable uJSON for OTBN
e74186d 
This commit enables communication with the OTBN SCA code over uJSON.
Currently, the ECC256 Keygen SCA test is supported.

The device code is located in lowRISC/opentitan#22190.

Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa
Copy link
Member Author

Currently not building as after a rebase the image size exceeds the available IMEM. This issue is resolved by #22717

@nasahlpa nasahlpa force-pushed the ujson_otbn_port branch from 2857e11 to c5acc96 Compare May 8, 2024 06:11
@nasahlpa
[pentest] Add ECC256 Keygen SCA test
9a26c5d 
This commit adds the ECC256 key generation side-channel
penetration test to the codebase.

The host code is located in lowRISC/ot-sca#347

Signed-off-by: Pascal Nasahl <[email protected]>
@nasahlpa nasahlpa closed this Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-temp m-temp m-temp left review comments

@wettermo wettermo wettermo approved these changes

@vogelpi vogelpi Awaiting requested review from vogelpi

@johannheyszl johannheyszl Awaiting requested review from johannheyszl

Assignees
No one assigned
Labels
CherryPick:master This PR should be cherry-picked to master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nasahlpa @m-temp @wettermo