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

Bug Fix: mbedtls_ecdsa_verify_restartable fails with ECDSA_SIGN_ALT #7499

Merged

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.

library/ecdsa.c Outdated Show resolved Hide resolved
library/ecdsa.c Show resolved Hide resolved
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 !

@gilles-peskine-arm gilles-peskine-arm merged commit d2e1dd0 into Mbed-TLS:development Apr 28, 2023
2 checks passed
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