Skip to content

Conversation

JonathanWitthoeft
Copy link
Contributor

@JonathanWitthoeft JonathanWitthoeft commented Apr 26, 2023

When ECDSA_SIGN_ALT but not ECDSA_VERIFY_ALT, mbedtls_ecdsa_can_do was not being defined causing mbedtls_ecdsa_verify_restartable to always fail

closes #7498

Description

This will support ATECC608 integration for signing but not verification.

Gatekeeper checklist

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

@gilles-peskine-arm
Copy link
Contributor

Please add a signoff line to your commit message(s), otherwise we can't accept them. The DCO check needs to pass.

@gilles-peskine-arm
Copy link
Contributor

Out of curiosity, which ATECC608 integration are you referring to? I don't recall seeing one that uses ECDSA_SIGN_ALT.

@JonathanWitthoeft
Copy link
Contributor Author

JonathanWitthoeft commented Apr 26, 2023

DCO finished!

I discovered this issue when attempting to use cryptoauthlib with mbedTLS 3 (upgrading from 2). ALT verify never would work with my ATECC608, so I always had it disabled. Non ALT verify worked with mbedTLS 2 because mbedtls_ecdsa_can_do was never used in mbedtls_ecdsa_verify_restartable

@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) priority-medium Medium priority - this can be reviewed as time permits labels Apr 26, 2023
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

I don't think the change is quite right. Also it needs a changelog entry.

When ECDSA_SIGN_ALT but not ECDSA_VERIFY_ALT, mbedtls_ecdsa_can_do was not being defined causing mbedtls_ecdsa_verify_restartable to always fail

Signed-off-by: JonathanWitthoeft <jonw@gridconnect.com>
Signed-off-by: JonathanWitthoeft <jonw@gridconnect.com>
Signed-off-by: JonathanWitthoeft <jonw@gridconnect.com>
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm 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 updating, looks good to me!

@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Apr 27, 2023
@tom-daubney-arm tom-daubney-arm self-requested a review April 28, 2023 09:05
@tom-daubney-arm tom-daubney-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-reviewer This PR needs someone to pick it up for review needs-review Every commit must be reviewed by at least two team members, labels Apr 28, 2023
Copy link
Contributor

@tom-daubney-arm tom-daubney-arm 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 @JonathanWitthoeft !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mbedtls_ecdsa_verify_restartable fails with ECDSA_SIGN_ALT
3 participants