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

Robust signature decoding #115

Merged
merged 8 commits into from
Sep 27, 2019
Merged

Conversation

tomato42
Copy link
Member

Ensure that the signature verification raises the same exception irrespective if the error is caused by mismatch of the hash value or because the encoding of the signature is invalid

fixes #114

so that they have the same formatting as the ones for integer and
sequence types
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 87.009% when pulling 932b7e3 on tomato42:robust-sig-decode into 72621b1 on warner:master.

@coveralls
Copy link

coveralls commented Sep 26, 2019

Coverage Status

Coverage increased (+1.005%) to 87.489% when pulling b96e02e on tomato42:robust-sig-decode into 72621b1 on warner:master.

the users of VerifyingKey.verify() and VerifyingKey.verify_digest()
should not need to use multiple exceptions to correctly catch invalid
signatures
Verify that strings of unexpected lengths are rejected with the
same BadSignatureError exception
@tomato42 tomato42 self-assigned this Sep 26, 2019
@rjrelyea
Copy link

it basically looks good with one exception:
line 138 in function remove_integer. You check for nbytes < 0x80 and say 'Negative integers not supported'. I think this is checking that the length of the integer is less than 128 bytes (1024 bits), not that the first bit of the integer is on (indicating that the integer is negative). This seems wrong. Either the error message or the code is wrong. I presume the latter since in RSA signatures, we often have integer lengths longer than 128 (like 2048 bit keys).

@tomato42
Copy link
Member Author

tomato42 commented Sep 27, 2019

You check for nbytes < 0x80 and say 'Negative integers not supported'. I think this is checking that the length of the integer is less than 128 bytes (1024 bits),

no, nbytes is the first byte of the part of the buffer that represents the integer – it's the most significant byte – I'll rename the variable to make it more readable

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
@tomato42
Copy link
Member Author

@rjrelyea: updated, I've also added more checks to verify if the values are correct DER encodings (not just BER), so please take a second look

@rjrelyea
Copy link

OK, that looks good. nbytes was probably from the copy of dealing with the length field,
Your checks for minimal integer and minimal length encoding are correct.

@opoplawski
Copy link

Is there an ETA for a 0.14 release? I just got CVE bugs filed against this in Fedora. Thanks.

@tomato42
Copy link
Member Author

tomato42 commented Oct 5, 2019

@opoplawski actually, I'm working on 0.13.3 in #124 – there has been too little testing in master to consider it 0.14-worthy

@tomato42 tomato42 added the bug unintended behaviour in ecdsa code label Oct 13, 2019
@tomato42 tomato42 added this to the v0.14 milestone Oct 13, 2019
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.

Inconsistent handling of malformed DER signatures
4 participants