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: parse CSR with elided Attributes #56901

Closed
cipherboy opened this issue Nov 22, 2022 · 4 comments
Closed

crypto/x509: parse CSR with elided Attributes #56901

cipherboy opened this issue Nov 22, 2022 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@cipherboy
Copy link
Contributor

cipherboy commented Nov 22, 2022 •
edited

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

$ go version
go version go1.19.3 linux/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cipherboy/.cache/go-build"
GOENV="/home/cipherboy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/cipherboy/go/pkg/mod"
GONOPROXY="github.com/hashicorp"
GONOSUMDB="github.com/hashicorp"
GOOS="linux"
GOPATH="/home/cipherboy/go"
GOPRIVATE="github.com/hashicorp"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build929116826=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Go's CSR parsing capabilities does not support CSRs with incorrectly elided Attributes.

See also: hashicorp/vault#17918

In particular, while RFC 2986 (dated Nov. 2000) required the inner CertificationRequestInfo to have four elements (the last of which is an explicit Attributes), the earlier RFC 2314 (dated Mar. 1998) allowed this to have IMPLICITtype tagging. I believe some people incorrectly took this to be OPTIONAL. In the CSR included with the issue, it'll parse successfully with both openssl asn1parse and openssl req:

OpenSSL invocations 
[cipherboy@xps15 40]$ openssl asn1parse -in csr.pem 
    0:d=0  hl=4 l= 706 cons: SEQUENCE          
    4:d=1  hl=4 l= 426 cons: SEQUENCE          
    8:d=2  hl=2 l=   1 prim: INTEGER           :00
   11:d=2  hl=2 l= 127 cons: SEQUENCE          
   13:d=3  hl=2 l=  11 cons: SET               
   15:d=4  hl=2 l=   9 cons: SEQUENCE          
   17:d=5  hl=2 l=   3 prim: OBJECT            :countryName
   22:d=5  hl=2 l=   2 prim: PRINTABLESTRING   :DE
   26:d=3  hl=2 l=  15 cons: SET               
   28:d=4  hl=2 l=  13 cons: SEQUENCE          
   30:d=5  hl=2 l=   3 prim: OBJECT            :stateOrProvinceName
   35:d=5  hl=2 l=   6 prim: PRINTABLESTRING   :Bayern
   43:d=3  hl=2 l=  28 cons: SET               
   45:d=4  hl=2 l=  26 cons: SEQUENCE          
   47:d=5  hl=2 l=   3 prim: OBJECT            :localityName
   52:d=5  hl=2 l=  19 prim: PRINTABLESTRING   :Lauf an der Pegnitz
   73:d=3  hl=2 l=  12 cons: SET               
   75:d=4  hl=2 l=  10 cons: SEQUENCE          
   77:d=5  hl=2 l=   3 prim: OBJECT            :organizationName
   82:d=5  hl=2 l=   3 prim: PRINTABLESTRING   :ABL
   87:d=3  hl=2 l=  18 cons: SET               
   89:d=4  hl=2 l=  16 cons: SEQUENCE          
   91:d=5  hl=2 l=   3 prim: OBJECT            :organizationalUnitName
   96:d=5  hl=2 l=   9 prim: PRINTABLESTRING   :Chargebox
  107:d=3  hl=2 l=  31 cons: SET               
  109:d=4  hl=2 l=  29 cons: SEQUENCE          
  111:d=5  hl=2 l=   3 prim: OBJECT            :commonName
  116:d=5  hl=2 l=  22 prim: PRINTABLESTRING   :Chargebox 808829186073
  140:d=2  hl=4 l= 290 cons: SEQUENCE          
  144:d=3  hl=2 l=  13 cons: SEQUENCE          
  146:d=4  hl=2 l=   9 prim: OBJECT            :rsaEncryption
  157:d=4  hl=2 l=   0 prim: NULL              
  159:d=3  hl=4 l= 271 prim: BIT STRING        
  434:d=1  hl=2 l=  13 cons: SEQUENCE          
  436:d=2  hl=2 l=   9 prim: OBJECT            :sha256WithRSAEncryption
  447:d=2  hl=2 l=   0 prim: NULL              
  449:d=1  hl=4 l= 257 prim: BIT STRING        
[cipherboy@xps15 40]$ openssl req -in csr.pem  -text
Certificate Request:
    Data:
        Version: 1 (0x0)
        Subject: C = DE, ST = Bayern, L = Lauf an der Pegnitz, O = ABL, OU = Chargebox, CN = Chargebox 808829186073
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
                Modulus:
                    00:9a:9d:84:e0:e7:f0:34:8a:90:c2:8d:54:1f:05:
                    9c:73:8d:fd:80:33:b6:3a:9a:d3:00:e7:12:07:8c:
                    9d:e4:23:56:bd:b8:9e:35:d4:5a:8b:89:76:63:90:
                    72:d5:77:d4:ce:5c:e4:10:cf:3a:94:a6:35:0f:26:
                    a4:d5:16:5c:81:22:14:0a:ff:c4:00:7b:71:1e:9f:
                    3e:e9:c2:01:d5:a7:a0:3f:58:cc:3b:0c:0c:d3:75:
                    07:d2:44:b6:cf:46:84:24:08:32:f1:91:eb:32:5d:
                    b9:30:5c:10:7f:27:b6:5a:e4:a6:a9:82:c1:5f:fc:
                    9c:63:64:1f:49:ca:8a:62:23:d0:2f:70:cf:cd:e9:
                    f4:5b:b6:14:85:a7:0f:94:42:90:0d:1a:38:8c:fc:
                    c2:07:99:58:9b:e7:bd:0e:4d:09:e4:e3:a1:98:6e:
                    f5:ed:df:6b:48:91:dd:4b:b6:15:3e:fb:a1:24:97:
                    19:a8:c2:1c:96:38:77:c3:5c:f5:82:97:ea:94:ec:
                    9a:40:f9:8d:96:85:89:ad:11:b6:1b:63:02:ea:b4:
                    44:52:24:79:8c:58:b2:bd:cb:90:0f:c8:29:a8:9a:
                    05:6c:c9:f5:8b:ea:f6:66:12:fd:47:50:61:f4:e4:
                    0a:47:e0:72:c9:5b:cb:c8:b0:e6:83:96:ff:37:aa:
                    56:a7
                Exponent: 65537 (0x10001)
        Attributes:
            Requested Extensions:
    Signature Algorithm: sha256WithRSAEncryption
    Signature Value:
        33:ba:e0:2d:37:77:f4:25:7b:34:fc:5e:38:4a:dc:9c:14:1d:
        62:8e:07:76:00:85:2b:cc:0a:3b:3e:6b:af:46:e6:5d:88:b9:
        3d:4d:8b:98:f0:df:ab:17:59:60:34:65:b4:12:c1:93:b8:c2:
        52:20:39:fe:b6:52:3f:fa:d6:91:1f:89:38:97:d5:f5:22:6b:
        1e:58:9d:53:de:fb:08:ab:f5:65:3d:43:35:9c:4d:4e:c3:33:
        b5:bd:98:2a:57:55:3c:d6:79:99:76:39:44:24:10:08:1e:8c:
        25:89:75:23:57:c0:45:ae:57:cb:2a:46:a3:48:1f:a6:e5:90:
        cc:87:65:3d:fc:a7:35:c1:dc:a6:0d:67:b9:79:5e:e3:59:f9:
        23:7b:d9:af:64:4d:64:57:e9:e3:ec:4e:94:ef:38:bb:fc:6d:
        8a:b1:7b:43:e0:43:35:4d:ca:35:2a:57:b7:57:3d:40:ac:ca:
        aa:9b:92:4c:0f:66:62:89:38:2c:b3:b6:14:ab:18:af:3e:de:
        3f:00:49:52:04:e1:fb:64:79:a5:37:c6:09:43:c2:bc:7b:22:
        66:68:78:6e:0a:01:f3:83:be:05:64:83:ee:a0:a8:29:80:74:
        1e:f9:0d:78:17:02:46:de:4b:14:71:2d:9d:c4:cf:c8:48:ae:
        43:fa:e0:7d
-----BEGIN CERTIFICATE REQUEST-----
MIICwjCCAaoCAQAwfzELMAkGA1UEBhMCREUxDzANBgNVBAgTBkJheWVybjEcMBoG
A1UEBxMTTGF1ZiBhbiBkZXIgUGVnbml0ejEMMAoGA1UEChMDQUJMMRIwEAYDVQQL
EwlDaGFyZ2Vib3gxHzAdBgNVBAMTFkNoYXJnZWJveCA4MDg4MjkxODYwNzMwggEi
MA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCanYTg5/A0ipDCjVQfBZxzjf2A
M7Y6mtMA5xIHjJ3kI1a9uJ411FqLiXZjkHLVd9TOXOQQzzqUpjUPJqTVFlyBIhQK
/8QAe3Eenz7pwgHVp6A/WMw7DAzTdQfSRLbPRoQkCDLxkesyXbkwXBB/J7Za5Kap
gsFf/JxjZB9JyopiI9AvcM/N6fRbthSFpw+UQpANGjiM/MIHmVib570OTQnk46GY
bvXt32tIkd1LthU++6EklxmowhyWOHfDXPWCl+qU7JpA+Y2WhYmtEbYbYwLqtERS
JHmMWLK9y5APyCmomgVsyfWL6vZmEv1HUGH05ApH4HLJW8vIsOaDlv83qlanAgMB
AAEwDQYJKoZIhvcNAQELBQADggEBADO64C03d/QlezT8XjhK3JwUHWKOB3YAhSvM
Cjs+a69G5l2IuT1Ni5jw36sXWWA0ZbQSwZO4wlIgOf62Uj/61pEfiTiX1fUiax5Y
nVPe+wir9WU9QzWcTU7DM7W9mCpXVTzWeZl2OUQkEAgejCWJdSNXwEWuV8sqRqNI
H6blkMyHZT38pzXB3KYNZ7l5XuNZ+SN72a9kTWRX6ePsTpTvOLv8bYqxe0PgQzVN
yjUqV7dXPUCsyqqbkkwPZmKJOCyzthSrGK8+3j8ASVIE4ftkeaU3xglDwrx7ImZo
eG4KAfODvgVkg+6gqCmAdB75DXgXAkbeSxRxLZ3Ez8hIrkP64H0=
-----END CERTIFICATE REQUEST-----

However, the same fails with Golang:

https://go.dev/play/p/nAyLYuDZWQh

Notably, OpenSSL includes this comment:

    /*
     * Zero or more attributes.
     * NB: although attributes is a mandatory field some broken
     * encodings omit it so this may be NULL in that case.
     */

OpenJDK 7+ does as well:

        // Cope with a somewhat common illegal PKCS #10 format
        if (seq[0].data.available() != 0)
            attributeSet = new PKCS10Attributes(seq[0].data);
        else
            attributeSet = new PKCS10Attributes();

So it might be worthwhile to add such parsing to Go as well. I'm not sure if simply adding optional to the ASN1 info is sufficient on the struct, or if we'll need to need to modify the marshaling code to always provision it, even if empty (so we do not become part of the problem as well).

(Just marking it optional does allow parsing, but I figured I'd open up the issue first for discussion before finalizing a patch approach).

What did you expect to see?

Successful parsing of the CSR.

What did you see instead?

panic: failed to parse CSR: asn1: syntax error: sequence truncated

goroutine 1 [running]:
main.main()
	/tmp/sandbox571007119/prog.go:19 +0x115

Program exited.
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 22, 2022
@seankhliao
Copy link
Member

@golang/security

@rolandshoemaker
Copy link
Member

Our X509 implement tends to be somewhat stricter and more opinionated than OpenSSL and others, who've managed to pick up a significant number of encoding oddities over their lifespan, many of which now serve to reinforce those mistakes.

Unless this is a significantly widespread issue I'd rather not add a workaround for incorrect encodings. The Java and OpenSSL comments suggest this may be a common mistake, but given we've never seen this issue before (as far as I can tell) I suspect they are just holding onto baggage from the past.

@cipherboy
Copy link
Contributor Author

cipherboy commented Nov 22, 2022 •
edited

Makes sense to me, and as I suspected. I'll go ahead and close this, but if someone else revisits this with a stronger case in the future, I'd be happy to provide a fix if desired.

Thank you!

@rolandshoemaker
Copy link
Member

👍 thanks, happy to revisit the issue if it pops up again!

@golang golang locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cipherboy @rolandshoemaker @gopherbot @seankhliao