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

Improve ECDSA verify validation #6190

Merged
merged 14 commits into from
Oct 31, 2022
Merged

Improve ECDSA verify validation #6190

merged 14 commits into from
Oct 31, 2022

Conversation

daverodgman
Copy link
Contributor

@daverodgman daverodgman commented Aug 10, 2022 •
edited by gilles-peskine-arm
Loading

Fixes #4420, Fixes https://github.com/Mbed-TLS/mbedtls-restricted/issues/962

Also corrects the documentation for the return code from mbedtls_ecdsa_verify() for an invalid signature to align with actual behaviour.

Backport in #6191

@daverodgman daverodgman force-pushed the invalid-ecdsa-pubkey branch from 2452be9 to d01e730 Compare August 10, 2022 10:39
@daverodgman daverodgman changed the title Invalid ecdsa pubkey Improve ECDSA verify validation Aug 10, 2022
@daverodgman daverodgman added bug needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. 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-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Aug 10, 2022
@daverodgman daverodgman mentioned this pull request Aug 10, 2022
Merged
@daverodgman daverodgman removed the needs-ci Needs to pass CI tests label Aug 10, 2022
tom-cosgrove-arm
tom-cosgrove-arm previously approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@gilles-peskine-arm gilles-peskine-arm self-requested a review August 18, 2022 09:08
@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Aug 18, 2022
gilles-peskine-arm
gilles-peskine-arm requested changes Aug 18, 2022
View reviewed changes
library/ecp.c Show resolved Hide resolved
library/ecdsa.c Outdated Show resolved Hide resolved
library/ecdsa.c Outdated Show resolved Hide resolved
tests/suites/test_suite_ecdsa.function Outdated Show resolved Hide resolved
tests/suites/test_suite_ecdsa.data Outdated Show resolved Hide resolved
tests/suites/test_suite_ecdsa.function Outdated Show resolved Hide resolved
tests/suites/test_suite_ecdsa.data Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 18, 2022
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 18, 2022
gilles-peskine-arm
gilles-peskine-arm requested changes Aug 18, 2022
View reviewed changes
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.

The change of error code for a signature with r or s out of range has not been justified. And as I'd suspected, it's breaking the PSA interface.

tests/suites/test_suite_psa_crypto.data Outdated Show resolved Hide resolved
tom-cosgrove-arm
tom-cosgrove-arm previously approved these changes Aug 22, 2022
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Oct 26, 2022
Closed
gilles-peskine-arm
gilles-peskine-arm requested changes Oct 26, 2022
View reviewed changes
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.

The code and tests look good to me, but the changelog entry needs to mention #4420.

tests/suites/test_suite_ecdsa.function Outdated Show resolved Hide resolved
ChangeLog.d/ecdsa-verify-fixes.txt Outdated Show resolved Hide resolved
ChangeLog.d/ecdsa-verify-fixes.txt Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 26, 2022
@daverodgman @gilles-peskine-arm
Update tests/suites/test_suite_ecdsa.function
683850b 
Co-authored-by: Gilles Peskine <[email protected]>
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 27, 2022
@daverodgman daverodgman added the needs-ci Needs to pass CI tests label Oct 27, 2022
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-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

gilles-peskine-arm
gilles-peskine-arm approved these changes Oct 28, 2022
View reviewed changes
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.

LGTM

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 28, 2022
@daverodgman
Copy link
Contributor Author

Ignoring Win2013 build failure - this seems like a general CI issue not specific to this PR

@daverodgman daverodgman removed the needs-ci Needs to pass CI tests label Oct 31, 2022
@daverodgman daverodgman merged commit 1a22bef into Mbed-TLS:development Oct 31, 2022
@daverodgman daverodgman deleted the invalid-ecdsa-pubkey branch October 31, 2022 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ECDSA signature verification succeeds when it should not
3 participants
@daverodgman @gilles-peskine-arm @tom-cosgrove-arm