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

Correct handling of BIT STRING in DER #138

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Conversation

tomato42
Copy link
Member

@tomato42 tomato42 commented Oct 15, 2019

the first byte of the bit string in DER is the number of unused bits in the bit string, parse it properly and give expected error values if the parsing fails

fixes #120

Todo:

  • fix the loss of coverage for keys.py

@tomato42 tomato42 added the bug unintended behaviour in ecdsa code label Oct 15, 2019
@tomato42 tomato42 added this to the v0.14 milestone Oct 15, 2019
@tomato42 tomato42 self-assigned this Oct 15, 2019
@coveralls
Copy link

coveralls commented Oct 15, 2019

Coverage Status

Coverage increased (+0.4%) to 92.476% when pulling ba3d483 on tomato42:bit_string into 0c19fcb on warner:master.

@tomato42 tomato42 force-pushed the bit_string branch 2 times, most recently from d6fe845 to 650a2a8 Compare October 16, 2019 21:48
@tomato42
Copy link
Member Author

@t8m please review

Copy link

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Approved with the doc nit fixed. The other thing is for your consideration.

src/ecdsa/der.py Outdated
the second being the remaining bytes in the string (if any); if the
`expect_unused` is specified as None, the first element of the returned
tuple will be a tuple itself, with first element being the bit string
as bytes and the second element bing the number of unused bits at the
Copy link

Choose a reason for hiding this comment

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

bing -> being

if unused and not s:
raise ValueError("unused is non-zero but s is empty")
encoded_unused = int2byte(unused)
len_extra = 1
Copy link

Choose a reason for hiding this comment

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

Would it be good idea to either check or override the padding of the last byte here if unused is nonzero?

Copy link
Member Author

Choose a reason for hiding this comment

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

checking is probably better, will do

the functions required the callees to handle the encoded length
of unused bits, handle it inside the functions now

also add test coverage for that

deprecate the old calling convention
@t8m
Copy link

t8m commented Oct 17, 2019

OK, still approved after the rebase and fixes.

@tomato42 tomato42 merged commit 78f7cf4 into tlsfuzzer:master Oct 17, 2019
@tomato42 tomato42 deleted the bit_string branch October 17, 2019 14:13
@tomato42
Copy link
Member Author

@t8m Thank you!

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.

bug in der.encode_bitstring()
3 participants