x509-cert/der regression

[lumag]

I found a regression after upgrading to x509-cert 0.2.1 / der 0.7.1. The attached certificate
test.txt can be parsed prefectly with x509-cert 0.1 / der 0.6.1, but fails after upgrade with the following error:

Error: Error { kind: Overlength, position: Some(Length(62)) }

I could not spot any obvious issue and the position seems lame (parsing TbsCertificate starting from byte 4 returns an error at position 29.

Reproducer (convert cert to DER for it to work):

use x509_cert::certificate::Certificate;
use x509_cert::der::Decode;

fn main() {
    let name = std::env::args().skip(1).next().unwrap();
    let bytes = std::fs::read(name).unwrap();
    let parsed_cert = &Certificate::from_der(&bytes).unwrap();
    println!("{parsed_cert:?}");
}

[tarcieri]

git bisect is probably the easiest way to narrow down what change caused this.

Offhand I'm not sure what it would've been. The only major changes to decoding I can think of were related to integer/serial number handling.

[lumag]

git-bisect points to a8a0b04 (#795)

[tarcieri]

Seems I guessed it then! cc @baloo

[lumag]

And going patch-by-patch (ugh), 62bbe0a is the problem.

[lumag]

And, I think I know the cause:

+const SERIAL_NUMBER_MAX_LEN: Length = Length::new(20);

[tarcieri]

Yeah, looks like a 21-byte serial number:

$ openssl x509 -in /Users/tarcieri/Downloads/test.txt -inform pem -text                                           130 ↵
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            84:ce:0b:f6:a0:fe:82:4e:e5:e5:06:ca:a8:9d:78:c6:15:01:62:57:71

I don't think it meets the criteria for 21-byte serial numbers to be allowed, which would be adding a leading zero so the resulting number is positive.

[lumag]

Well, quoting RFC 5280:

Note: Non-conforming CAs may issue certificates with serial numbers
that are negative or zero. Certificate users SHOULD be prepared to
gracefully handle such certificates.

I think we should drop the SERIAL_NUMBER_MAX_LEN check from SertificateNumber. Instead this check should be a part of certificate builder.

[lumag]

In the end, rfc2459 did not contain such requirement. So we should be liberal in what we accept.

[baloo]

I thought we relaxed the check when parsing from DER (to allow any length) and enforced the 20bytes limitation on new?
Did I imagine that discussion?

[tarcieri]

See lots of previous discussion on #823, #826, and #831

[tarcieri]

@baloo you even quoted the exact same excerpt of RFC 5280: #823 (comment)

[baloo]

Looks like it fixed parsing of negative serials, but this one is just overlength?

[tarcieri]

Yeah I think you're right. And RFC5280 doesn't seem to provide specific guidance for this case.

[baloo]

That was the comment I was thinking about: #826 (comment)

[tarcieri]

also regardless, this seems like the wrong error, despite the name:

Error: Error { kind: Overlength, position: Some(Length(62)) }

That error is intended for TLV framing bugs which made this somewhat confusing.

A better one might be ErrorKind::Value with tag: Tag::Integer, which would at least make it clearer where the error was occurring.

[tarcieri]

I improved the error in #988.

Gonna close this in favor of #984 and #987