rusticata / x509-parser

X.509 parser written in pure Rust. Fast, zero-copy, safe.
Other
207 stars 67 forks source link

Error parsing standard extension causes whole certificate to fail to decode #83

Closed lilyball closed 3 years ago

lilyball commented 3 years ago

Reading through the extensions code right now, it appears that if any of the standard supported extensions fail to parse out their parsed_extension form, it bubbles the error all the way up and causes the whole certificate to fail to decode. This is extremely unfortunate, both because it's completely unnecessary, and because this means explicitly unsupported GeneralNames (x400Address and ediPartyName) will cause this even if the data is otherwise well-formed.

Instead of bubbling the error up, it should catch it and store it in place of the ParsedExtension, such that X509Extension::parsed_extension() can return a Result. This way not only will parse errors (including unsupported items) not cause the whole certificate to fail to decode, but also I can tell where the error is, and it won't even affect me if I don't need that extension.

chifflier commented 3 years ago

What you describe is in fact what was intended (there is a ParsedExtension::ParseError variant). However, due to a mistake in parse_extension0 (https://github.com/rusticata/x509-parser/blob/x509-parser-0.9.2/src/extensions.rs#L453):

let (_, ext) = parser(i)?;

The above raises the error to the caller, instead of catching it and using the error type.

Thank you for the issue, I'll fix this.