Repository navigation
Conversation
Member
|
I'm not sure this change is worth the churn. |
Contributor
Author
|
fair on the churn. the only thing that nudged me to send it is that this is the one malformed-extension path that escapes as a KeyError rather than a ValueError, so a cert with an unknown TLSFeature value slips past anyone guarding parsing with except ValueError. it's a narrow edge though, so if you'd rather not carry it i'm happy to close it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The TLSFeature extension is decoded as a sequence of integers, each of which is looked up in the mapping to the TLSFeatureType enum. That lookup goes through a dictionary access that raises a bare KeyError when the value isn't one of the two defined features, so a certificate whose TLSFeature extension carries an unrecognised value makes accessing extensions raise KeyError instead of the ValueError that every other malformed-extension path raises. That is awkward for anyone wrapping certificate parsing in except ValueError, since the KeyError escapes unhandled. I came across it while feeding crafted extension values through the parser. Mapping the missed lookup to a parse error makes it surface as a ValueError with the usual message, which also matches how an unrecognised name attribute tag is already handled. I kept the change inside parse_cert_ext so nothing else shifts, and added a regression test that builds a certificate carrying an unknown feature value.