`x509::Extension` unable to reproduce DER

[npmccallum]

I noticed a possible issue with x509::Extension, but I haven't investigated it completely. Specifically, Extension::critical seems to not emit any DER for that field when serialized if the value of the field is false. This means that if you deserialize and the re-serialize an Extension that contains the field in the DER, you will not be able to re-produce the original DER. This might cause problems with signature validations. Perhaps Option<bool> might be the better choice here?

[carl-wallace]

That shouldn't be the case. An effort was made to push awareness of things like this into the library (vs having the caller know to leave the field absent for DER). Here's a snip from cargo expand that shows how the encoder should have the effect of omitting the default value for the criticality field. The test cases feature some decode/reencode (but round trip issues may still exist).

    impl<'a> ::der::Sequence<'a> for Extension<'a> {
        fn fields<F, T>(&self, f: F) -> ::der::Result<T>
        where
            F: FnOnce(&[&dyn der::Encodable]) -> ::der::Result<T>,
        {
            f(&[
                &self.extn_id,
                &::der::asn1::OptionalRef(if &self.critical == &default_false() {
                    None
                } else {
                    Some(&self.critical)
                }),
                &::der::asn1::OctetString::new(&self.extn_value)?,
            ])
        }
    }

[carl-wallace]

I just re-read this question while looking at PRs 323 and 348. I thought initially that you were asking why the field was not optional so that the caller could enforce DER manually, but you are asking why is false omitted. It is omitted because false is the default value and in DER default values are omitted. See section 11.5 in X.690:

The encoding of a set value or sequence value shall not include an encoding for any component value which is equal to
its default value.

You are correct that BER parsed then encoded with these DER decoders and encoders will not regenerate the original BER. That some implementations generate invalid DER is why the certval library uses undecoded bits for signature verification.

[npmccallum]

@carl-wallace I agree that the behavior of this crate matches the standards. However, there are major projects that have, at least historically, issued non-compliant certificates. Failure to handle this case will generally mean an inability to validate signatures when doing re-encoding.

[carl-wallace]

I think it'd be great if the encoders could switch hit between DER and BER (which is essentially what you are getting at). I also think streaming support would be nice. However, even with BER, re-encoding at signature verification time is not a great idea in my experience (how would the encoder know which fields were misencoded). Misencodings come in flavors other than BER/DER too. Better to verify with un-decoded TBSCertificate bits. That is why the tlv_bytes feature was added and that's what is done in the certval library.

[npmccallum]

@carl-wallace I agree that it is better to verify with undecoded bits. Note that the Certificate and CertReq structs are identical except for the signed contents. We could genericize a shared structure here...

[npmccallum]

(However, note that CertReq MUST decode the CertReqInfo in order to get the public key for validation.)

[carl-wallace]

Sure. While it's a bit sad to do a second partial decode, that's less heavy than a full re-encode of TBS bits and less problematic in terms of verification.

[npmccallum]

@carl-wallace Agreed.

[npmccallum]

I'm closing this issue since this encoding is non-compliant and we should be able to support decode.