search

zmap / zlint

X.509 Certificate Linter focused on Web PKI standards and requirements.
https://zmap.io
Apache License 2.0
353 stars 107 forks source link

Add lint for checking that server certificates do not contain any OUs in the Subject #689

Closed defacto64 closed 2 years ago

defacto64 commented 2 years ago

Add lint for checking that server certificates do not contain any OUs in the Subject, according to BRs 7.1.4.2.2, letter i) as modified by ballot SC47 (since BR 1.7.9).

robplee commented 2 years ago

Sorry to self-cite but can you explain what this PR is adding that wasn't covered by #675 ?

defacto64 commented 2 years ago

@robplee You are right, I just was not aware of #675, so I am going to close my pull request.

However, the prohibition of OUs only affects Subscriber (i.e. TLS server) certificates, so it should not be applied to CA certificates or other types of certificates. Therefore the CheckApplies() function should return false if the certificate is not a TLS server certificate.

defacto64 commented 2 years ago

Closing this pull request as it's a duplicate of #675 that I was not aware of.

robplee commented 2 years ago

@defacto64, I've done some digging(/reminding myself about how zlint works) and I think the lint I merged in is fine and the CheckApplies() is fine how it is despite just returning true. The reason is because the lint source is the CA/B Forum BRs. If you check here:

https://github.com/zmap/zlint/blob/44e12c12ca43a4af86f0dc2da4a71493ac9f8345/v3/lint/base.go#L101

Then you'll see that CABF BR lints are only applied against certs with EKU Any or EKU ServerAuth so I think we are ok here. However, the above link does have a bit of a code smell about it to me as I'd prefer to see the CABF BR lints have adequate CheckApplies functions rather than having this extra check elsewhere that is easily forgotten about and probably causes frustration for some of the people who import zlint in to their projects.

One day I'll open an issue and PR to try and address this. But it may not be that soon :)

defacto64 commented 2 years ago

@robplee I do not think that is sufficient. The util.IsServerAuthCert() util just checks that the certificate has either Any or ServerAuth in the EKU, but keep in mind that intermediate CAs for issuing TLS Server certs MUST (per the BRs) also have ServerAuth in their EKU, so this util function is not enough. At least, IMO, you should also add a check that the certificate is not a CA cert.

robplee commented 2 years ago

Ach! So close, I'll open an issue/PR today. Good catch!