x509-cert: another parse failure on publicly known cert #826

woodruffw · 2022-12-30T03:39:06Z

Like #821, although probably a different cause: d3eefbcbbcf49867838626e23bb59ca01e305db7 in the Windows CTL fails to parse with:

ASN.1 DER message is too long at DER byte 60

The offending cert, as PEM:

Happy to debug this again, just making the issue for visibility.

The text was updated successfully, but these errors were encountered:

woodruffw · 2022-12-30T03:39:50Z

Here's how OpenSSL parses it:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            e6:09:fe:7a:ea:00:68:8c:e0:24:b4:ed:20:1b:1f:ef:52:b4:44:d1
        Signature Algorithm: sha1WithRSAEncryption
        Issuer: C = PL, O = Krajowa Izba Rozliczeniowa S.A., CN = SZAFIR ROOT CA
        Validity
            Not Before: Dec  6 11:10:57 2011 GMT
            Not After : Dec  6 11:10:57 2031 GMT
        Subject: C = PL, O = Krajowa Izba Rozliczeniowa S.A., CN = SZAFIR ROOT CA
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
                Modulus:
                    00:ac:47:2f:8f:59:31:39:a5:ea:0d:f0:a5:8c:2b:
                    bd:02:a4:bd:cd:0a:73:aa:09:e6:cc:5f:82:6a:75:
                    a9:97:e5:bb:06:ef:f3:c0:5d:a1:c1:d2:89:79:58:
                    f0:dc:eb:55:8c:ee:58:1a:7b:27:ff:4a:50:b3:00:
                    a1:6a:13:23:33:90:e8:2f:53:36:6e:18:6c:0f:40:
                    d6:37:57:d7:06:0d:2f:90:24:ca:57:fc:5b:92:ff:
                    23:80:57:85:3f:4a:9e:52:3a:c7:e6:bb:05:50:26:
                    9e:bf:d3:b6:5f:ae:b5:3f:c0:b2:6b:36:7d:17:c4:
                    b0:5d:af:64:f3:31:1f:85:05:a8:e0:21:16:3c:53:
                    92:62:56:7f:60:ae:e2:7b:d1:40:91:dc:b6:d0:7e:
                    70:40:28:6a:0f:e1:11:1b:7f:fa:dc:e8:e1:44:73:
                    eb:1e:ab:d7:db:68:62:6c:71:e0:58:e3:06:45:21:
                    59:0c:35:5f:6d:6b:7b:ff:c7:a1:fd:be:d1:88:22:
                    c1:84:fc:e3:32:db:7a:a0:ea:08:e3:0e:5f:1b:c7:
                    e3:cd:32:18:39:21:fd:0a:1a:cd:65:f7:53:be:47:
                    40:4c:53:fe:22:65:c2:7b:bb:23:55:7a:1d:7c:ed:
                    28:8c:02:06:3b:31:7d:f6:c7:de:33:ac:84:22:6d:
                    cb:07
                Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Basic Constraints: critical
                CA:TRUE
            X509v3 Key Usage: critical
                Certificate Sign, CRL Sign
            X509v3 Subject Key Identifier:
                53:92:A3:7D:FF:82:76:F0:33:D4:EB:92:67:47:61:33:1B:68:3B:2A
    Signature Algorithm: sha1WithRSAEncryption
    Signature Value:
        39:50:55:9d:e4:42:ff:a4:1b:e2:20:c9:b5:cc:3d:89:2d:40:
        a9:a7:49:89:1b:b2:64:c3:39:c8:3b:36:b0:84:69:85:15:ac:
        46:a3:10:21:40:11:20:85:a3:fe:13:42:91:eb:aa:00:c1:50:
        ba:c5:ed:f6:41:45:52:1d:f5:aa:5a:77:73:6c:e0:f3:2c:1f:
        32:8f:b5:80:47:47:03:33:8e:59:9e:04:81:3c:1b:85:13:75:
        ae:18:b2:57:f6:0d:74:d0:44:df:21:7f:b8:60:14:7f:e0:d4:
        7f:ba:f4:e7:a6:77:7d:7a:d7:bb:5a:19:74:65:f6:3d:e3:2b:
        ae:e3:14:2a:07:94:05:bf:e3:fb:21:75:d5:95:33:a3:92:3b:
        86:5c:47:32:40:08:d0:cd:b1:5a:4f:db:c8:00:14:9b:78:25:
        d6:05:b8:ef:39:7b:a4:27:2e:96:80:08:9c:97:31:8d:49:8a:
        6b:ad:ef:11:cb:27:5d:7e:68:d6:f4:72:06:c2:c2:0e:d3:71:
        25:76:2b:ef:a8:46:0b:d4:7b:f5:c3:f3:ee:af:ee:85:50:43:
        9a:2f:af:0f:10:85:28:bd:f8:30:20:1d:81:03:0f:af:64:3b:
        b0:70:7d:26:5f:9e:3c:3e:88:c6:ca:0e:37:e1:c0:e3:91:25:
        da:de:61:c5

woodruffw · 2022-12-30T03:45:30Z

Haven't confirmed yet, but I'm guessing it's this:

formats/x509-cert/src/serial_number.rs

Lines 32 to 44 in 1c001b3

 /// Maximum length in bytes for a [`SerialNumber`] 

 pub const MAX_LEN: Length = Length::new(20); 

 /// Create a new [`SerialNumber`] from a byte slice. 

 pub fn new(bytes: &[u8]) -> Result<Self> { 

 let inner = Int::new(bytes)?; 

 if inner.len() > SerialNumber::MAX_LEN { 

 return Err(ErrorKind::Overlength.into()); 

 } 

 Ok(Self { inner }) 

 }

(Although that doesn't make total sense, since the SN is 20 bytes here. But maybe I made a mistake in my Int implementation?)

woodruffw · 2022-12-30T03:53:24Z

Ah, yep, this is because of an implementation decision I made: Int preserves the "encoded" length of unsigned integers, which in this case is 0x15 == 21 because of the zero-extension byte. Maybe that was an incorrect decision on my part, since both OpenSSL and https://lapo.it/asn1js/ consider it a 20-byte integer.

It makes the Int API a little less ergonomic, but I could fix this by turning Int into a sum type instead:

enum Int {
    Signed(Sint),
    Unsigned(Uint),
}

...and delegating through that way. Alternatively, I could have Int keep a signed: bool flag internally and specialize on that. Any preferences?

tarcieri · 2022-12-30T04:49:59Z

@woodruffw the encoding is two’s complement, and in that regard, I think that rather than an enum, the representation should be your latter suggestion, decomposing it into a Uint/UintRef with a signed flag.

baloo · 2022-12-31T18:25:21Z

(follow up on #764 (comment))

An enum would work I think.

I kind of have in mind an impl From<u8/u16/u32/u64/usize> for SerialNumber (building the Uint version) for it to easily build "known-safe/known-valid" values of SerialNumber, that would help with an x509 builder API.

I would left out intentionally the From<i8> variants as signed values are contrary to the RFC.

tarcieri · 2022-12-31T18:27:40Z

Yeah, I don't think there should be any explicit APIs for constructing negative serial numbers. It should only be something that can be decoded.

Perhaps there could also be something like fn lint(&self) -> Result<()> which returns an error if a negative serial number is encountered that can be called after a certificate has been parsed.

woodruffw · 2023-01-02T18:35:00Z

Just to summarize: the problem here is an ambiguity in RFC 5280's wording: the length constraint on a serial number can either be interpreted as "no more than 20 bytes, encoded as an unsigned integer" or as "no more than 20 bytes, however they end up being encoded, so long as they end up unsigned."

Other X.509 implementations generally take the latter interpretation, and allow generating serial numbers that encode to 21 bytes, so long as their unsigned representation is only 20 bytes:

Go's crypto/x509: crypto/x509: CreateCertificate creates invalid serial number field golang/go#33310
pyca/cryptography: https://github.com/pyca/cryptography/blob/4e1d5cbe0fc97882844ca6df6d330f2abbcc2e88/src/cryptography/x509/base.py#L1130-L1131

These implementations also don't generally check the length of serial numbers they receive.

baloo · 2023-01-02T19:34:07Z

I guess the point here is:

if it comes from the deserializer, we should support anything, signed, unsigned, more than 20 bytes long
if it's coming from an rust-type using the "easy way"/constructor, we should enforce unsigned 20-bytes long max.

woodruffw mentioned this issue Dec 30, 2022

der, x509-cert: support for signed bigints, negative serial numbers #823

Merged

woodruffw mentioned this issue Jan 2, 2023

der, x509-cert: fix handling of negative integers #831

Merged

baloo mentioned this issue Jan 20, 2023

der, x509-cert: fix handling of negative integers #852

Closed

tarcieri closed this as completed in #831 Jan 20, 2023

tarcieri mentioned this issue Apr 4, 2023

x509-cert/der regression #978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x509-cert: another parse failure on publicly known cert #826

x509-cert: another parse failure on publicly known cert #826

woodruffw commented Dec 30, 2022

woodruffw commented Dec 30, 2022

woodruffw commented Dec 30, 2022

woodruffw commented Dec 30, 2022

tarcieri commented Dec 30, 2022

baloo commented Dec 31, 2022

tarcieri commented Dec 31, 2022

woodruffw commented Jan 2, 2023 •

edited

Loading

baloo commented Jan 2, 2023 •

edited

Loading

x509-cert: another parse failure on publicly known cert #826

x509-cert: another parse failure on publicly known cert #826

Comments

woodruffw commented Dec 30, 2022

woodruffw commented Dec 30, 2022

woodruffw commented Dec 30, 2022

woodruffw commented Dec 30, 2022

tarcieri commented Dec 30, 2022

baloo commented Dec 31, 2022

tarcieri commented Dec 31, 2022

woodruffw commented Jan 2, 2023 • edited Loading

baloo commented Jan 2, 2023 • edited Loading

woodruffw commented Jan 2, 2023 •

edited

Loading

baloo commented Jan 2, 2023 •

edited

Loading