Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
hujun-open opened this issue Dec 26, 2022 · 2 comments
Open

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

hujun-open opened this issue Dec 26, 2022 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@hujun-open
Copy link

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
				}
@mengzhuo mengzhuo changed the title affected/package: crypto/x509, the ParseRevocationList() doesn't seem to populate the AuthorityKeyId correctly crypto/x509: the ParseRevocationList() doesn't seem to populate the AuthorityKeyId correctly Dec 26, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 26, 2022
@seankhliao
Copy link
Member

cc @golang/security

@hujun-open
Copy link
Author

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()

@seankhliao seankhliao added this to the Unplanned milestone Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
2 participants
@hujun-open @seankhliao