Check tbsCertificate signature algorithm matches certificate

[rolandshoemaker]

Per RFC 5280 section 4.1.1.2, couldn't find an existing lint.

Resolves #419

[rolandshoemaker]

Just to add a little context after some discussions elsewhere, this lint is probably of value to a CA that is using zlint, since it may catch creating a certificate that is somewhat... dubious post signature, but is likely of significantly less value as a lint for larger corpora, like CT, since the certificate signatureAlgorithm field is not covered by the signature and is as such malleable. This means anyone can submit a bunch of doppelgänger certificates with an altered algorithmIdentifier sequence which would trigger this lint (an example being the 10 certs caught using the corpus integration tests, which all appear to be altered versions of their original certificates.

I'd probably be find abandoning this change given this...

[cpu]

Hey @rolandshoemaker! Thanks for the PR.

I haven't had a chance to review it yet but wanted to leave a comment.

I'd probably be find abandoning this change given this...

I think it's worth keeping if the tradeoffs you mention are documented (in code comments or elsewhere). We actually have an outstanding issue for this lint that @sleevi filed: #419 I think we were favouring an approach that would put more of the ASN.1 work into ZCrypto (zmap/zcrypto#213) WDYT?

[rolandshoemaker]

Ah interesting, I completely missed that issue.

In terms of exposing the signature that seems viable, but I'm not sure it's what you'd entirely want for this particular lint. Using asn1.Unmarshal means throwing away extraneous elements at the end of the SEQUENCE, so for instance an algorithmIdentifier with a trailing VideotextString will look just like one without, but won't actually be byte-for-byte equal. We could probably get access to those bytes, but it'd require adding a handful of asn1.RawContent and alias types all over the place.

[cpu]

Thanks @rolandshoemaker, this is a nice clean implementation ✨

Can you take a pass at fixing the integration test data based on the docs here? It looks like there's also a ~10 error level findings in the corpus. If you could give those a quick look-over when you update the integration/config.json to make sure they're true-positives that would be great.

For my review I flagged a few nits but there's nothing worth holding this PR up on. I'd like to give @sleevi a chance to 🔍 this before I merge but if he's tied up I'm also comfortable pulling the trigger once CI passes and letting it bake in-tree before the next RC.

Thanks again!

[rolandshoemaker]

Thanks for the reviews, think I hit everything.