certificate w/o issuer can't be parsed

[victorjulien]

Rule writers have reported that Suricata can't inspect some fields in a cert if the issuer is missing from it. https://redmine.openinfosecfoundation.org/issues/5439

Suricata's call to parse_x509_der errors with: (suricata::x509::rs_x509_decode) -- Error(Der(NomError(Many1)))

Suricata currently uses 0.11 in our git master.

[cpu]

:wave:

I think this issue should be moved from here to x509-parser. It's the one that offers the (deprecated) parse_x509_der fn.

From my perspective I'm not sure this is something that should be fixed. The ASN.1 module defined in RFC 5280 for a TBSCertificate indicates the Issuer field is mandatory. Should all mandatory fields be considered optional to support a best-attempt at parsing malformed certificates? That's likely significant complexity creep.

[victorjulien]

Hi @cpu, I think Suricata is one of the usecases for which these crates have been developed. At suricata, we have to deal with traffic as it is found on the wire, not as it's in RFCs. Sadly, in reality there is a wide discrepancy due to various reasons, like differences in implementations of OS', libs, etc. Malware often uses this discrepancy (accidentally or deliberately) to avoid detection. So this is why parsers for security tools like Suricata need to be way more liberal than is ideal.

I agree this belonged in the x509 repo instead. However it seems the issue is addressed in all versions Suricata cares about (0.8.2, 0.15.1, 0.16), so we can close it.

Btw, if you are interested, here is the pcap of our test: https://github.com/OISF/suricata-verify/tree/master/tests/tls/tls-cert-noissuer

Wireshark also accepts it w/o complaints:

[cpu]

At suricata, we have to deal with traffic as it is found on the wire, not as it's in RFCs.

That's fair and I can understand the desire to have a relaxed parser for that general use-case. I'm just pointing out that there are lots of other mandatory fields that could also be omitted in a malformed certificate. I would advocate that if the goal is to offer a parser that returns as much data as it can, even for malformed certificates, and that goal is shared by x509-parser, then it should be written such that other mandatory fields are treated as optional, not just Issuer.

I agree this belonged in the x509 repo instead. However it seems the issue is addressed in all versions Suricata cares about (0.8.2, 0.15.1, 0.16), so we can close it.

Good to know!

Btw, if you are interested, here is the pcap of our test: https://github.com/OISF/suricata-verify/tree/master/tests/tls/tls-cert-noissuer

Thank you, that's helpful in case someone wants to tackle this.

[chifflier]

Hi, I think there is a possible solution: I previously introduced (in 0.10 I think) X509CertificateParser to allow building parsers with options. The only available option at the moment is whether to parse extensions or not, but adding an option to relax some constraints does not seem hard. I'll have a look