crypto/x509: the ParseRevocationList() doesn't seem to populate the AuthorityKeyId correctly

[hujun-open]

What version of Go are you using (go version)?

$ go version
go version go1.19.4 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\junhu\AppData\Local\go-build
set GOENV=C:\Users\junhu\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\junhu\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\junhu\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.19.4
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\hujun\gomodules\src\certrevochk\go.mod
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\junhu\AppData\Local\Temp\go-build1060665108=/tmp/go-build -gno-record-gcc-switches

What did you do?

Use x509.ParseRevocationList to parse a CRL file; the parse result of the CRL file using openssl is following

Certificate Revocation List (CRL):
        Version 2 (0x1)
        Signature Algorithm: ecdsa-with-SHA256
        Issuer: CN = TestRootCA-0
        Last Update: Dec 26 03:03:57 2022 GMT
        Next Update: Dec 26 03:13:57 2022 GMT
        CRL extensions:
            X509v3 Authority Key Identifier:
                keyid:DA:E0:15:3C:4B:5D:BB:04:3E:D7:AE:75:DF:39:5A:D1:F6:B6:9A:CD

            X509v3 CRL Number:
                0
Revoked Certificates:
    Serial Number: 01
        Revocation Date: Dec 26 03:03:58 2022 GMT
    Serial Number: 02
        Revocation Date: Dec 26 03:03:58 2022 GMT
    Serial Number: 03
        Revocation Date: Dec 26 03:03:58 2022 GMT
    Serial Number: 04
        Revocation Date: Dec 26 03:03:58 2022 GMT
    Serial Number: 05
        Revocation Date: Dec 26 03:03:58 2022 GMT
    Signature Algorithm: ecdsa-with-SHA256
         30:45:02:21:00:ac:91:78:d2:cd:ff:f0:a2:90:8b:3b:c6:58:
         ae:d1:48:6f:cf:4c:4e:05:9e:1e:1f:7b:17:be:77:0b:6e:64:
         27:02:20:7d:77:a5:3b:94:03:04:f0:6f:02:5e:fc:85:56:f0:
         0e:da:de:66:a8:ea:78:7b:e7:4f:04:3b:68:fe:0a:ce:c5

What did you expect to see?

the RevocationList.AuthorityKeyId == DA:E0:15:3C:4B:5D:BB:04:3E:D7:AE:75:DF:39:5A:D1:F6:B6:9A:CD

What did you see instead?

the RevocationList.AuthorityKeyId == 30168014DAE0153C4B5DBB043ED7AE75DF395AD1F6B69ACD
There are additional 4 bytes 30168014, I am no expert of ASN1 encoding, but I guess these 4 bytes are some sort of ASN1 encoding header/overhead.

after reading the parser.go, I found the ParseRevocationList() has following code to populate AKID

			if ext.Id.Equal(oidExtensionAuthorityKeyId) {
				rl.AuthorityKeyId = ext.Value

which is different from processExtensions() used by parseCertificate()

				// RFC 5280, 4.2.1.1
				val := cryptobyte.String(e.Value)
				var akid cryptobyte.String
				if !val.ReadASN1(&akid, cryptobyte_asn1.SEQUENCE) {
					return errors.New("x509: invalid authority key identifier")
				}
				if akid.PeekASN1Tag(cryptobyte_asn1.Tag(0).ContextSpecific()) {
					if !akid.ReadASN1(&akid, cryptobyte_asn1.Tag(0).ContextSpecific()) {
						return errors.New("x509: invalid authority key identifier")
					}
					out.AuthorityKeyId = akid
				}

[seankhliao]

cc @golang/security

[hujun-open]

after some reading of RFC5280, the AKID is defined in section 4.2.1.1 as:

   AuthorityKeyIdentifier ::= SEQUENCE {
      keyIdentifier             [0] KeyIdentifier           OPTIONAL,
      authorityCertIssuer       [1] GeneralNames            OPTIONAL,
      authorityCertSerialNumber [2] CertificateSerialNumber OPTIONAL  }

   KeyIdentifier ::= OCTET STRING

which means it is a sequence of 3 fields, RFC allows either of following two cases:

1. keyIdentifier only
2. authorityCertIssuer + authorityCertSerialNumber

so it seems processExtensions() code only populate the AKID when it is case-1, while ParseRevocationList() simply uses the whole DER bytes.

case1 is the common case, however RFC does also allow case2, I think it is ok to populate AuthorityKeyId with case1 and providing raw extensions access via Extensions field to cover case2. but I do think current ParseRevocationList() should use same method as processExtensions()

[gopherbot]

Change https://go.dev/cl/587455 mentions this issue: crypto/x509: properly pouplate the RevocationList.AuthorityKeyId field