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

Conversion function between raw and DER ECDSA signatures #8681

Closed
wants to merge 15 commits into from
Closed

Conversion function between raw and DER ECDSA signatures #8681

wants to merge 15 commits into from

Conversation

valeriosetti
Copy link
Contributor

Description

Add ECDSA's conversion functions between raw and DER (ASN.1) format.

Resolves #7765

PR checklist

  • changelog TODO
  • backport not required
  • tests provided

@valeriosetti
psa_util: add raw<->DER ECDSA conversion functions
1d884c9 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
pk_wrap: use PSA util functions for ECDSA conversion instead of PK ones
0c0197a 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti added needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Jan 8, 2024
@valeriosetti valeriosetti requested a review from mpg January 8, 2024 15:53
@valeriosetti valeriosetti self-assigned this Jan 8, 2024
@valeriosetti valeriosetti added this to To Do in Roadmap Board for Mbed TLS via automation Jan 8, 2024
@valeriosetti
test_suite_psa_crypto_util: add test function and data for ECDSA conv…
9816ae5 
…ersion functions

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
This was referenced Jan 9, 2024
Merged
Merged
mpg
mpg requested changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

This is mostly a complete review, but there are a few more things I want to do:

  • compare with previous functions
  • check test coverage.

Overall it's looking good even though I had quite a lot to say :)

include/mbedtls/psa_util.h Outdated Show resolved Hide resolved
include/mbedtls/psa_util.h Show resolved Hide resolved
include/mbedtls/psa_util.h Show resolved Hide resolved
library/psa_util.c Outdated Show resolved Hide resolved
library/psa_util.c Show resolved Hide resolved
tests/suites/test_suite_psa_crypto_util.data Outdated Show resolved Hide resolved
tests/suites/test_suite_psa_crypto_util.function Outdated Show resolved Hide resolved
tests/suites/test_suite_psa_crypto_util.function Outdated Show resolved Hide resolved
tests/suites/test_suite_psa_crypto_util.function Outdated Show resolved Hide resolved
tests/suites/test_suite_psa_crypto_util.function Outdated Show resolved Hide resolved
Roadmap Board for Mbed TLS automation moved this from To Do to In Development Jan 9, 2024
@mpg
Copy link
Contributor

mpg commented Jan 9, 2024

Following this discussion with Gilles, actually could you try prototyping (in another PR) an alternative implementation of these functions that doesn't rely on existing mbedtls_asn1 functions (hence does not depend on MBEDTLS_ASN1_xxx_C) but instead re-implements just what it needs from ASN.1 for this particular case (can make assumption on lengths fitting in two bytes, etc)? Just to see if it would result in a lot of code duplication or not that much.

(IMO this doesn't have to be blocking this PR. We can perfectly start by merging a version that depends on MBEDTLS_ASN1_xxx_C and then later change it to a version that doesn't. Incremental improvement and so on.)

@valeriosetti
psa_util: improve description for ECDSA conversion functions
e68f886 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
psa_util: minor code improvements
55ce240 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test_suite_psa_crypto_util: increase the size of tested integers
fc7b7ac 
- Replace 192 case with 256
- Replace 256 case with 512
- Add 521 case

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
psa_util: add include asn1write.h in public header
50d75f2 
This is mandatory to have support for the error codes defined
in the asn1write.h header file.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
Copy link
Contributor Author

[...] could you try prototyping (in another PR) an alternative implementation of these functions that doesn't rely on existing mbedtls_asn1 functions [...]?

Sure! I will start the new PR based on this one

@valeriosetti
psa_util: remove CRYPTO_C guard from ECDSA conversion functions
7dec057 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test_suite_psa_crypto_util: split ECDSA test function in two
76f5c37 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
all.sh: add exception for ASN1_PARSE_C in check_test_dependencies
87484b1 
There is no PSA equivalent to ASN1 legacy symbols.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
psa_util: skip leading zeros in der format with "if" instead of "while"
4e53147 
This is possible because we know that DER format can have at most
1 leading zero.

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti mentioned this pull request Jan 10, 2024
Closed
3 tasks
@valeriosetti
psa_util: add variable casting in convert_raw_to_der_single_int()
d01e3e3 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti
test_suite_psa_crypto_util: change curve type for 256bits tests
589a48b 
Tests with 256 bits curve simply depends on any curve of that size,
but they don't really care about which family is enabled.

Here I replaced PSA_WANT_ECC_SECP_R1_256 with PSA_WANT_ECC_SECP_K1_256
because otherwise there were test disparities in the
"analyze_driver_vs_reference_tfm_config" component of
"analyze_outcomes.py". It looked simpler to change the curve type
in the test suite's data rather than adding proper exceptions
in "analyze_outcomes.py"

Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
mpg
mpg reviewed Jan 11, 2024
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback.

(Still not a complete review, same points left as last time.)

tests/suites/test_suite_psa_crypto_util.data Outdated Show resolved Hide resolved
tests/suites/test_suite_psa_crypto_util.data Show resolved Hide resolved
@valeriosetti
test_suite_psa_crypto_util: add comments to 512/521 bit size test cases
dd185e6 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti requested a review from mpg January 11, 2024 09:43
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) and removed needs-ci Needs to pass CI tests labels Jan 11, 2024
@valeriosetti valeriosetti mentioned this pull request Jan 16, 2024
Merged
3 tasks
@valeriosetti
psa_util: guard ECDSA conversion functions with proper (internal) symbol
df4e456 
Signed-off-by: Valerio Setti <valerio.setti@nordicsemi.no>
@valeriosetti valeriosetti mentioned this pull request Jan 16, 2024
Closed
@valeriosetti
Copy link
Contributor Author

Closing this one as agreed on #8696 (comment). This feature will be completed in #8703

Roadmap Board for Mbed TLS automation moved this from In Development to Done Jan 24, 2024
@valeriosetti valeriosetti mentioned this pull request Feb 7, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@mpg mpg Awaiting requested review from mpg

Assignees

@valeriosetti valeriosetti

Labels
needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
Roadmap Board for Mbed TLS
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conversion function between raw and DER ECDSA signatures
3 participants
@valeriosetti @mpg @gilles-peskine-arm