Lint to enforce no trailing zero byte on KeyUsage bitstrings

[christopher-henderson]

This lint addresses #681 where certificates were discovered whose KeyUsage encodings violated DER's requirement on minimal bitstring representations.

The following are interested parties in this issue:

@robplee
@aarongable
@CBonnell

[zakird]

Looks good to me. Not merging myself given that a few others were flagged in the original PR.

[aarongable]

I don't think this quite checks for exactly the right thing. It seems to be looking for trailing 00 bytes. Such bytes will definitely be in violation of DER, so that's good, but I think it misses some cases.

For example, a KU extension encoded as 03 02 04 80 means "a bit string (03) of length two bytes (02) with four unused bits at the end (04) containing the bits 1000XXXX (80)". But according to DER, that should be encoded as 03 02 07 80 (aka 1XXXXXXX), trimming those three trailing zeros out of the value.

So this needs to operate not just at the byte level, but at the bit level.

[aarongable]

Suggested change

	Description: "RFC 5280 Section 4.2.1.3 describes the value of a KeyUsage to be a DER encoded BitString, which itself must not have unnecessary trailing 00 bytes.",
	Description: "RFC 5280 Section 4.2.1.3 describes the value of a KeyUsage to be a DER encoded BitString, which itself must not have unnecessary trailing 0 bits.",

[christopher-henderson]

@aarongable

The example given is this thread does confuse me a bit, however I think that's because the binary for 80 is actually 01010000 so the four actually works here (I think).

May I construct an arbitrary value to test my understanding of this encoding?

• Say that we have [03 02 01 04]
  • Visualizing the content to binary we have [03 02 01 (0000010X)]
• But it should actually be [03 02 02 04]
  • And thus [03 02 02 (000001XX)]

Is this a correct example?

---

Now, I am thinking that this may actually be two lints, because if my understanding is correct then it is possible to be compliant with one requirement but not the other.

• Say that we have [03 03 09 02 00]
  • Visualizing the content to binary we have [03 03 09 (0000001X) (XXXXXXXX)]

I believe that the above correctly encodes the "skippable" bits but violates the requirement of no trailing zero bytes.

[aarongable]

The example given is this thread does confuse me a bit, however I think that's because the binary for 80 is actually 01010000 so the four actually works here (I think).

Sorry, I was using hexadecimal encoding for all those example bytes, but forgot to say so explicitly. So byte 80 was 10000000.

May I construct an arbitrary value to test my understanding of this encoding?

* Say that we have `[03 02 01 04]`
  
  * Visualizing the content to binary we have `[03 02 01 (0000010X)]`

* But it should actually be `[03 02 02 04]`
  
  * And thus `[03 02 02 (000001XX)]`

Is this a correct example?

Yep, completely agreed.

Now, I am thinking that this may actually be two lints, because if my understanding is correct then it is possible to be compliant with one requirement but not the other.

* Say that we have `[03 03 09 02 00]`
  
  * Visualizing the content to binary we have `[03 03 09 (0000001X) (XXXXXXXX)]`

I believe that the above correctly encodes the "skippable" bits but violates the requirement of no trailing zero bytes.

In this case simply detecting that the third byte (number of skipped bits) is greater than 7 is an immediate indicator that something is wrong: that byte is supposed to encode the number of unused bits up to the next byte boundary, so it should never be 8 or more.

I think you're right that detecting both mis-encodings is good. I don't have a strong opinion on whether it should be one or two lints.

[robplee]

Definitely manages the "no trailing 0 byte" requirement in the title of the PR. Either in the same lint or in a second lint then it'd be nice to have checks for the extra cases you've discussed already with @aarongable.

Happy to keep reviewing if/when you make any further changes to the PR.

[christopher-henderson]

So @aarongable I gave the other side of this coin a whack (counting and correcting unused bytes), however I think I've hit (what I have to assume is) an encoding that I'm hoping you can help me understand.

I'm getting over 6500 failures in the test corpus and they're all from the exact same KU ([3 2 5 128]) so I'm going to assume I'm misunderstanding something here.

Now, all of these certs have only one KU, digitalSignature, with this precise exact ASN1 value

Bytes (decimal): [3 2 5 128]
Binary: [00000011 00000010 00000101 10000000]

My lint, as written, is pointing out that 10000000 has seven trailing zeroes, however their encoding all denote that there are five unused bits. Given 6500 failures of the exact same input I'm going to assume that there is a gap in my understanding somewhere here.

      KeyUsage ::= BIT STRING {
           digitalSignature        (0),
           nonRepudiation          (1), -- recent editions of X.509 have
                                -- renamed this bit to contentCommitment
           keyEncipherment         (2),
           dataEncipherment        (3),
           keyAgreement            (4),
           keyCertSign             (5),
           cRLSign                 (6),
           encipherOnly            (7),
           decipherOnly            (8) }

Indeed, if my understanding is correct and a bitstring reads in a left-to-right evaluation, then 10000000 makes sense as the value for digitalSignature only. I just can't see why unused bits is 5 and not 7.

[CBonnell]

@christopher-henderson I believe that the encoding [3 2 5 128] is not the correct DER encoding of a KU value that asserts solely the digitalSignature bit, and your additional lint is correctly flagging this.

X.690 2002-07, clause 11.2.2 says:

Where ITU-T Rec. X.680 | ISO/IEC 8824-1, 21.7, applies, the bitstring shall have all trailing 0 bits removed before it is encoded.

Thus, it is clear that an encoder that encodes "5" as the initial octet but then asserts "128" as the following octet value has not correctly removed all trailing bits and has failed to DER encode the (named) BIT STRING value.

[aarongable]

(Apologies, I'm on vacation this week but I can take a closer look next week.)

On first glance, I agree with Corey and with your lint: a KU of digitalSignature encoded as 0x03 0x02 0x05 0x80 is encoded in BER but not DER.

[christopher-henderson]

Thank you @aarongable and @CBonnell for the quick glance!

Given that it was over 6k new failures in the test corpus I had to assume that I was the problem 😛.

However, after a bit of digging through the zcrypto/x509 extension construction logic I did indeed find an old bug wherein the unused bits would always be 0. This was fixed in CL 168990043 and, indeed, the fixed Go stdlib agrees with our notions on what the encoding should be.

So I'm gonna reckon that these 6k+ failures are all simply certs generated by the same bug in OpenSSL or other such common provider.

I think I'm going to go ahead and merge this lint and then open up a new PR for encoding issue. Thank you again!