1520923 - X.509 certificate serial number display wrongly shows leading 0x00 if highest bit set

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P5)

Description

[David von Oheimb]

Attachment: serialnumber-DER-leading-00.diff

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

When displaying X.509 certificates, for instance in the certificate viewer, there is a bug in the hexadecimal output of cert serial numbers (and possibly also of other DER encoded unsigned integers or binary strings).

Actual results:

If in their raw binary representation the first byte is >= 0x80 the DER representation demands a leading 0x00 byte - see, e.g., https://docs.microsoft.com/en-us/windows/desktop/seccertenroll/about-integer

Take for example one of the first root certs in the current default trust store ('Authorities'), of "AC Camerfirma S.A." with subject "Chambers of Commerce Root - 2008". Its serial number is 'A3:DA:42:7E:A4:B1:AE:DA' while Thunderbird shows it as '00:A3:DA:42:7E:A4:B1:AE:DA'. In other words, it fails to skip the leading zero byte.

Expected results:

Other cert viewers, .e.g,

openssl x509 -in ChambersofCommerceRoot-2008.crt -noout -text | head -n 5

correctly display
``
Certificate:
Data:
Version: 3 (0x2)
Serial Number:
a3:da:42:7e:a4:b1:ae:da

I've attached a patch correcting this.

Comment 1

[David von Oheimb]

This bug is related to Bug 1518449 (and may be related to Bug 1300150).

Comment 2

[J.C. Jones [:jcj] (he/they)]

Thanks, David!

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9037381 [details] [diff] [review]
serialnumber-DER-leading-00.diff

Review of attachment 9037381 [details] [diff] [review]:
-----------------------------------------------------------------

This will have some follow-on effects to Firefox Telemetry [1] and how we calculate cert hashes [2], which may have a bunch of cascading effects.

We may need a new method instead, a CERT_Hexify_Unsigned perhaps that we can use elsewhere.

Tagging Dana for feedback


[1] https://searchfox.org/mozilla-central/source/security/manager/ssl/ContentSignatureVerifier.cpp#184

[2] https://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSCertificate.cpp#625

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9037381 [details] [diff] [review]
serialnumber-DER-leading-00.diff

Review of attachment 9037381 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like in most cases this function is used to hex-convert arbitrary binary data, so it would be wrong for those uses to skip a leading zero byte. As for displaying serial numbers, I think it's actually better this way. Otherwise, we wouldn't be able to distinguish between properly and improperly encoded serial numbers. We should probably update the documentation for this function, though. Additionally, looks like we return "00" for zero-length data, which seems wrong.

Comment 5

[David von Oheimb]

This will have some follow-on effects to Firefox Telemetry [1] and how we calculate cert hashes [2], which may have a bunch of cascading effects.

If cert hashes are really calculated over the hex output of their binary representation this would be pretty weird. Note that fingerprints of X.509 certs use a hash function on the DER encoding, which does not involve hex output.

We may need a new method instead, a CERT_Hexify_Unsigned perhaps that we can use elsewhere.

This sounds reasonable.

Comment 6

[David von Oheimb]

It looks like in most cases this function is used to hex-convert arbitrary binary data, so it would be wrong for those uses to skip a leading zero byte.

The leading 0x00 must be skipped only when decoding the DER representation of unsigned integers (such as serial numbers).

As for displaying serial numbers, I think it's actually better this way.

No, displaying that leading 0x00 is wrong.
See the definition of ASN.1 DER and the examples/references I gave above.

Otherwise, we wouldn't be able to distinguish between properly and improperly encoded serial numbers. We should probably update the documentation for this function, though.

There are many ways of producing illegal DER encodings. In any such cases, a proper error should be reported, rather than outputting misleading artifacts to the user.

Additionally, looks like we return "00" for zero-length data, which seems wrong.

Yes, this is wrong. On empty input, the hex output should be empty as well.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

To clarify, I'm saying that this function is not actually predominantly used on DER-encoded data (and thus, it would be wrong to skip leading zero bytes). So, it would be better to update the documentation to reflect this. With regard to illegal DER encodings, it is not uncommon for certificates to have negative serial numbers, and rejecting them outright is not something we want to do for compatibility reasons. Consequently, it is useful to display the bytes of the serial number to be able to distinguish these certificates.

Comment 8

[David von Oheimb]

Hardly no X.509 certs actually have negative serial numbers. They would not be RFC 5280 compliant. As written there:

CertificateSerialNumber ::= INTEGER

The serial number MUST be a positive integer assigned by the CA to each certificate. It MUST be unique for each certificate issued by a given CA (i.e., the issuer name and serial number identify a unique certificate). CAs MUST force the serialNumber to be a non-negative integer.

If the length of the raw binary representation of an unsigned serial number is a multiple of eight bits (which usually is the case) and the highest bit is set (which should be the case for 50% of all random serial numbers) then the leading byte in the raw data is >= 0x80. In such cases a leading zero byte is needed to prevent misinterpreting the integer as negative.

This is the case for instance in the example I gave initially above, where the raw representation of the 8-byte random serial number is a3:da:42:7e:a4:b1:ae:da, which needs to be encoded as {0x00, 0xa3, 0xda, 0x42, 0x7e, 0xa4, 0xb1, 0xae, 0xda}.

From this one can conclude that for nearly 50% of all certs the hex display of their serial number by Mozilla's cert viewer is wrong because the leading 0x00 is not stripped.

Certificate serial numbers that are really meant to be negative (and thus not RFC 5280 compliant) or that are meant to be positive but are wrongly encoded (i.e., are not prefixed by 0x00 in the case specified above) could be recognized by having a DER representation in which the first data byte is >= 0x80. For these cases, RFC 5280 recommends:

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.

These non-compliant/error cases are very rare.

So in contrast to what Dana vote I do not think it is acceptable not to strip the 0x00 prefix, which just would have the minor advantage to allow manual detection of illegal serial numbers, while having the major disadvantage that for nearly half of all legal certs, which constitute very likely more than 99.9% of all certs, a misleading artifact is displayed.

Comment 9

[David von Oheimb]

Attachment: serialnumber-DER-leading-0x00.diff - version 2

Meanwhile I've had a look at the other uses of CERT_Hexify() and now better understand the comments by J.C. and Dana that in many cases the function is used to display raw (octet) byte strings, which are for instance cert fingerprints.
I fully agree with J.C. that a further function for hex-converting DER-encoded unsigned integers is needed for al clean solution because CERT_Hexify() cannot serve both purposes.

So I'm providing an updated patch, which introduces char *CERT_HexifyUnsigned(SECItem *i, int do_colon) and a static auxiliary function char *hexify(unsigned char *data, unsigned int len, int do_colon that does the actual hex encoding also for CERT_Hexify(). The new function CERT_HexifyUnsigned() gets called (only) where cert serial numbers are to be displayed.

The definition of the new function CERT_HexifyUnsigned() also includes comments indicating where encoding errors are gracefully dealt with. The new version of CERT_Hexify() also fixes the further bug mentioned by Dana, namely that empty byte strings should not be output as '00' but as an empty hex string.

Comment 10

[J.C. Jones [:jcj] (he/they)]

Thanks, David. I think this approach makes sense to me, too, and I like adding that new function. I need to check-in on compat issues with the functions that call it before I review it properly though.

adds to his todo list

Comment 11

[David von Oheimb]

:jcj, any progress on this over the last (almost) 3 years?

Comment 12

[John Schanck [:jschanck]]

There are no normative guidelines for the presentation of serial numbers, and there is significant disagreement in practice. However, whatever presentation is chosen should preserve all of the information in the DER encoding.

So, I would consider the following to be reasonable presentations:

1. The current choice.
2. Decimal with a + or - sign.
3. The hex encoded contents of the DER encoding of the absolute value of the serial with a +/- sign.

I don't think that the proposal here (to use the absolute value directly with no indication of the encoded sign) is a good choice. We have seen bugs, like Bug 1717100, where the loss of the sign bit caused serious problems.

Comment 13

[David von Oheimb]

@jschnack wrote:

There are no normative guidelines for the presentation of serial numbers, and there is significant disagreement in practice.

Likely true, but there is the normative spec how to encode and interpret X.509 cert serial numbers, mentioned above: IETF RFC 5280 section 4.1.2.2 https://www.rfc-editor.org/rfc/rfc5280.html#section-4.1.2.2
There is also the ASN.1 standard with its DER (Distinguished Encoding Rules).
And there is common sense.

However, whatever presentation is chosen should preserve all of the information in the DER encoding.

Agreed.

In your above list of 'reasonable representations' you are missing what other implementations such as OpenSSL do:

4. For positive serial numbers, strip any leading 0x00 because it is a pure DER artifact, which is needed just in case an unsigned serial number happens to have the highest bit set in its byte representation.
   See again the example ChambersofCommerceRoot-2008.crt I gave above, which may be found also here:
   https://ssl-tools.net/certificates/786a74ac76ab147f9c6a3050ba9ea87efe9ace3c.txt
   For negative serial numbers, which are possible but non-conforming, print a minus sign and the absolute (hex or decimal) value.

Your choice number 1. (The current choice) is wrong.
When the cert to be printed has a negative value, for instance -2, Thunderbird prints

Serial Number FE

which looks like the hex representation of the (positive) serial number 254.

I don't think that the proposal here (to use the absolute value directly with no indication of the encoded sign) is a good choice.

I meanwhile agree on this.

We have seen bugs, like Bug 1717100, where the loss of the sign bit caused serious problems.

Yeah, but that is likely just a consequence of some other inconsistencies within Mozilla on this matter.

Comment 14

[David von Oheimb]

BTW, for other sorts of hex numbers, such as fingerprints and subject key identifiers, e.g.

Key ID  A3:F6:DB:9C:23:F5:AD:E8:92:4E:58:D8:6B:CD:1E:F2:6A:1F:CB:00

you also don't print an artificial leading 00: in case the first byte is >= 0x80.

So your treatment of cert serial numbers is inconsistent also in this way.

Comment 15

[John Schanck [:jschanck]]

I understand you perspective, and I agree that there is room for disagreement. However, a decision was made here. Please don't re-open the bug.