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

Make extension parsing more robust #32

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

g2p
Copy link
Contributor

@g2p g2p commented Jun 26, 2020

Fall back to UnsupportedExtension when an extension parser fails with
Incomplete.
This allows parsing the remaining extensions, instead of silently eating
the rest of the certificate with no indication that the parse was
partial.

Let's encrypt certificates triggered this issue.
I was unable to fix the basicConstraints parsing, but at least with this
I am able to access the whole set of extensions.

Fall back to UnsupportedExtension when an extension parser fails with
Incomplete.
This allows parsing the remaining extensions, instead of silently eating
the rest of the certificate with no indication that the parse was
partial.
@chifflier
Copy link
Member

Hi,

If I understand correctly, the real error is an error in the BasicConstraint parsing? If it's possible, I'd be very interested in examples of certificates that failed, and fixing this also.

That said, this leaves the question of how to handle failures when deep-parsing the extensions. Since the global DER structure was already validated, I'd be in favor of being not too strict, and returning UnsupportedExtension could be a solution. I'll think about the best solution, so I'll just wait one day or two before merging this PR.

Thanks!

@g2p
Copy link
Contributor Author

g2p commented Jun 29, 2020

Sure! I'll open an issue and attach a certificate for the basicConstraints issue.
(I wanted to anonymize the cert a bit, if I couldn't fix the parser on my own)

@chifflier chifflier merged commit dbc05b3 into rusticata:master Jun 29, 2020
@chifflier
Copy link
Member

Merged, thanks!
This gives a way to know that the extension parser fails, while previously it was silently ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants