Add lint to check for incorrect 'unused' bit encoding in KeyUsages #684
Conversation
| Name: "e_incorrect_ku_encoding", | ||
| Description: "RFC 5280 Section 4.2.1.3 describes the value of a KeyUsage to be a DER encoded BitString, which itself defines that all trailing 0 bits be counted as being \"unused\".", | ||
| Citation: "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.", | ||
| Source: lint.RFC5280, |
There was a problem hiding this comment.
Nitpicky, but I'd expect the source to match where the citation text is located. Otherwise, someone might be searching 5280 for the text that's actually in X.690.
There was a problem hiding this comment.
Thank you for reading carefully!
I agree that it is a bit awkward, for encoding issues what have been doing to date is to say...
RFC 5280 says that it has to be a proper <X> encoding, where those encoding rules may be found in <X REQS>
That is, we list it under an RFC requirement, but then cite the encoding requirement's docs because that's the real meat of the lint that someone would likely have an interest in.
| // Bytes 3..n: KeyUsage | ||
| declaredUnused := uint(ku[2]) | ||
| actualUnused := big.NewInt(0).SetBytes(ku[3:]).TrailingZeroBits() | ||
| if declaredUnused == actualUnused { |
There was a problem hiding this comment.
Not directly related to this particular lint, but in a similar area: another area that may deserve linting is whether a certificate contains only KU values defined in RFC 5280/X.509 08/2005. I was doing some testing of this code by hacking up the DER of the testdata certificate in a hex editor and found that this certificate (with an overly-long KU value) lints clean:
-----BEGIN CERTIFICATE-----
MIIFWzCCBEOgAwIBAgIRAJoU+BokmxTr5Dn+9UpWX5IwDQYJKoZIhvcNAQELBQAw
XzELMAkGA1UEBhMCRlIxDjAMBgNVBAgTBVBhcmlzMQ4wDAYDVQQHEwVQYXJpczEO
MAwGA1UEChMFR2FuZGkxIDAeBgNVBAMTF0dhbmRpIFN0YW5kYXJkIFNTTCBDQSAy
MB4XDTE5MDYwNzAwMDAwMFoXDTIwMDYwNzIzNTk1OVowczEhMB8GA1UECxMYRG9t
YWluIENvbnRyb2wgVmFsaWRhdGVkMRswGQYDVQQLExJHYW5kaSBTdGFuZGFyZCBT
U0wxMTAvBgNVBAMTKDhiMzFkZjAwMDg3MTQ4OWY4NGJhZDY3MGJmZWNjNTBiLnlh
dHUud3MwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAS5We6u1F5b4UyH/5kJtlB5
xuDc1HMf8wKlfR9lkJUJjZGljbt9UuZv3eshGTeKxZfgxaby5IGNznzEnrRsbe7o
o4ICxzCCAsMwHwYDVR0jBBgwFoAUs5Cn2MmvTs1hPJ98rV1/Qf1pMOowHQYDVR0O
BBYEFDeLodmWLKchJAz8Z3X7ae7gqptHMBEGA1UdDwEB/wQHAwUHgAABgDAMBgNV
HRMBAf8EAjAAMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjBLBgNVHSAE
RDBCMDYGCysGAQQBsjEBAgIaMCcwJQYIKwYBBQUHAgEWGWh0dHBzOi8vY3BzLnVz
ZXJ0cnVzdC5jb20wCAYGZ4EMAQIBMEEGA1UdHwQ6MDgwNqA0oDKGMGh0dHA6Ly9j
cmwudXNlcnRydXN0LmNvbS9HYW5kaVN0YW5kYXJkU1NMQ0EyLmNybDBzBggrBgEF
BQcBAQRnMGUwPAYIKwYBBQUHMAKGMGh0dHA6Ly9jcnQudXNlcnRydXN0LmNvbS9H
YW5kaVN0YW5kYXJkU1NMQ0EyLmNydDAlBggrBgEFBQcwAYYZaHR0cDovL29jc3Au
dXNlcnRydXN0LmNvbTAzBgNVHREELDAqgig4YjMxZGYwMDA4NzE0ODlmODRiYWQ2
NzBiZmVjYzUwYi55YXR1LndzMIIBBQYKKwYBBAHWeQIEAgSB9gSB8wDxAHcAu9nf
vB+KcbWTlCOXqpJ7RzhXlQqrUugakJZkNo4e0YUAAAFrM7rqfQAABAMASDBGAiEA
22t+dkxoN9w/uL4BgZnXJ7kBAeH043JWWSh09iH0E3UCIQCFFchVq2s5tTQOm4uh
12fw9AfEoUxtjc8bkL1NvWymdgB2AF6nc/nfVsDntTZIfdBJ4DJ6kZoMhKESEoQY
dZaBcUVYAAABazO66p0AAAQDAEcwRQIgR2oUResxqRX6A1jux8QjolYj7l/0fxSD
rkjFuBiUADMCIQC3eysQ5lJf2i8r3tewXKRIkaYc2GtLUIQtnCZbY3SigzANBgkq
hkiG9w0BAQsFAAOCAQEALsVSECEgVYh7w2coMoG8wmQkVrg+HNrvK3mKV8BV5KLu
cVHULWDinn78f3Ej79yQ46pezhNSQENYt3M6SX5uQDkawO2Ih/sSZUMUxRiYqwrh
QFx/ZHZraoLphcTbxXDx/RgiyUlMBNtuaGbR3kjrMcZWtpOVCQKNK86f3szivEDO
gFKBtFE9n5HHzr+ZU2Y/p6ZihyAhMXs7dxC1cmL1J5ijN01LhBA/XN1EP/JE8sG8
CX7WLIV5RgUiW2NfOXT+vnHxlLEYekD6yK2evw4WmXdTnDdOT78fiKR59uDUEZK6
4FQXITHsiJmV0jsT3ck/rAL2XJMUDg6OYZD/YyVcBA==
-----END CERTIFICATE-----
There was a problem hiding this comment.
Ooh this is a really good catch. I think I agree that it would be best as a separate lint, but a really good one to add. This is definitely not a good keyUsage :)
There was a problem hiding this comment.
Certainly! I went ahead and threw together #686 for this
christopher-henderson
requested review from
CBonnell and
aarongable
and removed request for
CBonnell
At-mentions for @aarongable and @CBonnell since you both helped with #682 so I would appreciate a perfunctory glance at this follow-on lint if you happen have the time and energy.
This lint is a continuation of #682 wherein KeyUsages were found to be incorrectly padded (that is, there are more trailing
0bits than is declared in the BitString).As per ITU X.690-0207...
In this context, what it means for a bit to be "removed" is to declare it as being "unused". For example, the bit field
01011000has three trailing0bits that should be "removed" (that is, declared to be unused).Test Corpus
There is a large number of failures in the test corpus (over 6500) so it took some digging to confirm that this lint is reasonable.
Let us take the chose text cert as an example (whose ASN1 can be found on crt.sh)
The KU for this certificate is the following...
0x03 0x02 0x05 0x80Or, in binary...
00000011 00000010 00000101 10000000What this is saying is:
0x03: This is a bitstring0x02: That is two bytes long0x05: And has five unused bits0x80: Whose contents are0x80However, we can see clearly that
0x80does not have five unused bits...100 00000...rather, it has seven.
Indeed, 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.
As such, it seems as though that there was likely and ASN1 implementation that is widely used that at some point had this KU encoding bug.