`TbsCertificate::find_extension()` should return an error if there are duplicate extensions

[lilyball]

Not a Contribution

TbsCertificate::extensions_map() returns an error if any extension is present twice. This is good behavior in general, as trying to handle duplicated extensions is a potential security issue. Unfortunately TbsCertificate::find_extension() only returns Option<&X509Extension<'a>>, with no potential for error. It should be updated to return Result<Option<&X509Extension<'a>>, X509Error>, returning Err(X509Error::DuplicateExtensions) if the desired extension is present twice in the extensions list.

We should probably also at least update the documentation for X509Certificate::extensions() to point at TbsCertificate::find_extension() and TbsCertificate::extension_map(), so that way callers can more easily find those and don't try to reimplement extension searching themselves.

Security

The reason why this is a security issue is when you combine two or more pieces of software that process the same certificate. If a certificate is created that carries duplicate extensions, and if all software involved in handling the certificate ignores duplicate extensions, then you can run into the scenario where software A processes the first duplicate and software B processes the second. This leads to attacks where you pass a "safe" extension value for software A to validate, and pass a duplicate malicious extension value for software B to act upon.

The correct way to defend against this is to treat the duplicate extension as an error, regardless of what the extension is. If your software treats it as an error, then it doesn't matter what other software is used to process the same certificate, because your part in the certificate processing will error out and stop the whole attack.

Even with this change, a client that wishes to ignore duplicates can call cert.extensions().iter().find(|ext| ext.oid == oid) themselves. But we should make the "obvious" way to do this (calling find_extension()) have the correct behavior.

This is also perfectly conforming behavior. RFC 5280 §4.2 says

A certificate MUST NOT include more than one instance of a particular extension.

Unfortunately it does not elaborate on the security implications here in the Security Considerations section.

Additional notes

Instead of updating the doc comment on X509Certificate::extensions(), you might also consider just removing all of the methods from X509Certificate that duplicate those on TbsCertificate (which I believe is all of them except for verify_signature()) and instead add a Deref<Target = TbsCertificate> impl. This way all of the TbsCertificate methods can be accessed from X509Certificate easily, which should simplify basic usage.

[chifflier]

Hi,

Sorry for the long delay, the refactoring of der-parser and introduction of the asn1-rs was harder than expected :/

Extension duplication

I fully agree, the find_extension() was just a helper, but as it is currently, it encourages unsafe behavior. I will deprecate/remove it in next release.

Please note that my experience parsing certificates (for ex. the Certificate Transparency stream) shows that duplicate extensions are quite common, some PKIs seem to add extensions to certificate requests without removing already existing extensions from the CSR if they are present. Despite the MUST NOT in the RFC, common libraries (openssl and others) do not treat this as an error. This is why x509-parser changed, previously it was rejecting such certificates, now it just parses them and leaves the decision to the crate user.

Note that the check is implemented in the X509StructureValidator object, and will raise an error.

About Deref

Thanks a lot for the suggestion! Adding a Deref is indeed a very elegant way to avoid code duplication. The only limitation I can see is that we can have only one Deref, but since on this object all methods will be delegated to TbsCertificate, this shouldn't be a problem.

[lilyball]

Huh, I wonder why so many certificates are issued with duplicate extensions in violation of the rather strict language in RFC 5280?

In any case, I appreciate you fixing this.