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

Backport signature decode #124

Merged
merged 10 commits into from
Oct 7, 2019
Merged

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Oct 4, 2019

Backport the recent fixes to signature decoding.

See #114

also harden the public and private key decoding (partial backport of #118)

@tomato42 tomato42 added the bug unintended behaviour in ecdsa code label Oct 4, 2019
@tomato42 tomato42 self-assigned this Oct 4, 2019
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 88.324% when pulling 75c0669 on tomato42:backport-sig-decode into bb359d3 on warner:0.13.

@coveralls
Copy link

coveralls commented Oct 4, 2019

Coverage Status

Coverage increased (+1.5%) to 90.78% when pulling 1eb2c04 on tomato42:backport-sig-decode into bb359d3 on warner:0.13.

@tomato42 tomato42 force-pushed the backport-sig-decode branch 4 times, most recently from 1439b29 to 9c7ebc3 Compare October 4, 2019 22:20
@tomato42 tomato42 force-pushed the backport-sig-decode branch 5 times, most recently from e204c2d to 8adaa3b Compare October 5, 2019 12:52
@tomato42 tomato42 requested a review from warner October 5, 2019 13:22
@tomato42 tomato42 force-pushed the backport-sig-decode branch from 8adaa3b to 2dc0d7a Compare October 5, 2019 16:59
@tomato42 tomato42 mentioned this pull request Oct 6, 2019
@tomato42
Copy link
Member Author

tomato42 commented Oct 7, 2019

@Jakuje please review

Copy link

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

There are some nits along the lines, but considering it is backport of something already in master, we should not bother that much with the messy history of the commits.

Only thing that might make sense could be mentioning the master commits IDs in the cherry-picked so it is obvious that it is backport.

ecdsa/test_malformed_sigs.py Outdated Show resolved Hide resolved
ecdsa/test_malformed_sigs.py Outdated Show resolved Hide resolved
ecdsa/test_malformed_sigs.py Show resolved Hide resolved
ecdsa/test_der.py Show resolved Hide resolved
tomato42 and others added 10 commits October 7, 2019 14:42
the users of VerifyingKey.verify() and VerifyingKey.verify_digest()
should not need to use multiple exceptions to correctly catch invalid
signatures

backport of 487f6d3
Verify that strings of unexpected lengths are rejected with the
same BadSignatureError exception

backport of 8533e51
the same issues as with decoding integers happen with the NIST521p curve
as it's large enough that the length encoding of some fields needs
to use multi-byte encoding

backport of a655d6f
as assert statements will be removed in optimised build, do use a custom
exception that inherits from AssertionError so that the failures are
caught

this is reworking of d47a238 to implement the same checks but
without implementing support for SEC1/X9.62 formatted keys
not a backport, necessary to make the tests runnable on 0.13 branch
@tomato42 tomato42 force-pushed the backport-sig-decode branch from 2dc0d7a to 1eb2c04 Compare October 7, 2019 13:07
Copy link

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

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

looks good now

@tomato42 tomato42 merged commit 5c4c74a into tlsfuzzer:0.13 Oct 7, 2019
@tomato42 tomato42 deleted the backport-sig-decode branch October 7, 2019 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behaviour in ecdsa code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants