Skip to content

Conversation

@tomato42
Copy link
Member

@tomato42 tomato42 commented Oct 21, 2019

Test for and fix the bugs in handling bytes-like objects in VerifyingKey and SigningKey.

fixes #135
fixes #144

@tomato42 tomato42 added bug unintended behaviour in ecdsa code maintenance issues related to making the project usable or testable labels Oct 21, 2019
@tomato42 tomato42 added this to the v0.14 milestone Oct 21, 2019
@tomato42 tomato42 self-assigned this Oct 21, 2019
@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage increased (+0.3%) to 92.903% when pulling cc5d5ae on tomato42:bytes-support into 4c92d31 on warner:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.499% when pulling ece7cab on tomato42:bytes-support into 188f09e on warner:master.

@tomato42 tomato42 force-pushed the bytes-support branch 2 times, most recently from f6b1e7c to 0b39122 Compare October 24, 2019 20:17
also stop copying diff-instrumental.py, it's now in master so
it will be present when we checkout the base commit
Document what inputs don't work in which python versions
make the bytearray work on pythohn2.6
make multi-byte array.array objects work on all versions
more test coverage for VerifyingKey.from_string, to verify that the
alternative inputs can handle bytearray too
document what parameter types don't work for from_der()
and for VerifyingKey.from_public_key_recovery_with_digest()
add support for all bytes-like objects as input for
SigningKey.from_der
this is repeating the s[x] three times for every element extracted,
use a function for that

also decreases number of tests needed for condition coverage
@tomato42 tomato42 force-pushed the bytes-support branch 2 times, most recently from b06b4bd to 2f383de Compare October 24, 2019 23:11
as extra_entropy can be large, and different type than outputs
from either bits2octets or number_to_string, do not concatenate
them but rather push them to the hmac one by one

don't use six.b(), not needed on py2.6

simplify the chained comparison and remove unneeded else after return
in rfc5979.generate_k
@tomato42 tomato42 marked this pull request as ready for review October 25, 2019 00:21
:return: tuple with decoded 'r' and 's' values of signature
:rtype: tuple of ints
"""
signature = normalise_bytes(signature)

Choose a reason for hiding this comment

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

In keys.py you didn't normalize the signature, but here you are. A comment about why this is a different case than when signature is passed to the verify function would probably be in order.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is because the signature object in keys.VerifyingKey can be any object that the sigdecode can handle. Here it's a byte string, but for sigdecode_strings signature will be a tuple or a list

there is also a comment in VerifyingKey.verify() function:

        # signature doesn't have to be a bytes-like-object so don't normalise
        # it, the decoders will do that

sha1.update(data)
data_hash = sha1.digest()
assert isinstance(data_hash, bytes)
sig_raw = sk.sign(data, sigencode=sigencode_string)

Choose a reason for hiding this comment

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

Did you really want to sign data and not data_hash here (and following?).

Choose a reason for hiding this comment

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

OK, never mind. sk.sign does it's own hashing while sk.sign_digest signs the digest. Not clear to me how sk.sign chooses the hash, but I think that's perl API and now a function you created.

Copy link
Member Author

Choose a reason for hiding this comment

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

SigningKey includes the default that's set on object creation (either generation or import), it defaults to sha1, like here:
https://github.com/warner/python-ecdsa/blob/4c92d31dec71c5fe478744d57d332d5ceb255a33/src/ecdsa/keys.py#L221-L222

@tomato42 tomato42 merged commit 3538d44 into tlsfuzzer:master Oct 29, 2019
@tomato42 tomato42 deleted the bytes-support branch October 29, 2019 18: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 maintenance issues related to making the project usable or testable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sign_digest_deterministic() doesn't work without explicit hash Test coverage to verify bytes-like object acceptance for primary classes

3 participants