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.

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

crypto/x509: inner and outer signature algorithm identifiers don't match #47689

Open
groob opened this issue Aug 13, 2021 · 6 comments
Open

crypto/x509: inner and outer signature algorithm identifiers don't match #47689

groob opened this issue Aug 13, 2021 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@groob
Copy link
Contributor

groob commented Aug 13, 2021

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

go version go1.17rc2 darwin/arm64

Does this issue reproduce with the latest release?

Does not reproduce in go version go1.16.6 darwin/arm64

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

go env Output
$ go env

GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/vvrantchan/Library/Caches/go-build"
GOENV="/Users/vvrantchan/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/vvrantchan/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/vvrantchan/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.16.6/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.16.6/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.16.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/vvrantchan/code/x/b196191547/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/kq/g8xycx_95j54cxffqtx0s_xm00lql0/T/go-build1627430943=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/-rDc14aTqgV

The com.apple.systemdefault certificate is a self signed root that Apple generates during macOS setup. It lives in most (all?) macOS user keychain. I recently discovered that go.17rc2 fails to parse some (but not all) of the Apple system keychain certificates. The one in the example was issued in 2015. It's possible Apple issued some of these certs erroneously, and fixed the mismatch in a follow-up release.

I've only detected the failure on about 10 out of 100k macOS devices, but our environment has quick refresh cycles for devices. It's possible the problem would be more widespread for other users parsing the macOS System Keychain with Go.

What did you expect to see?

com.apple.systemdefault
2015-03-25 21:10:26 +0000 UTC

What did you see instead?

2021/08/13 10:01:51 x509: inner and outer signature algorithm identifiers don't match
exit status 1
@seankhliao seankhliao changed the title x509: inner and outer signature algorithm identifiers don't match crypto/x509: inner and outer signature algorithm identifiers don't match Aug 13, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 13, 2021
@seankhliao
Copy link
Member

cc @FiloSottile @rolandshoemaker
related #44237 (?)

@FiloSottile
Copy link
Contributor

SEQUENCE (1 elem)
    OBJECT IDENTIFIER 1.2.840.113549.1.1.5 sha1WithRSAEncryption (PKCS #1)

vs

SEQUENCE (2 elem)
    OBJECT IDENTIFIER 1.2.840.113549.1.1.5 sha1WithRSAEncryption (PKCS #1)
    NULL

Sigh. We might have to tolerate a NULL parameters mismatch? @rolandshoemaker

FWIW the certificate can be patched to parse correctly, since the outer signature algorithm is not signed.

@rolandshoemaker
Copy link
Member

We've managed to mostly excise special exceptions for broken certificates, and I'm loathe to add more. This seems relatively rare, unless we start to see a significant number of breakages (that cannot be reasonably worked around) I think we should just accept that we're not going to parse some malformed certificates.

@danielbobbert
Copy link

danielbobbert commented Nov 22, 2021

According to the spec (https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.1.2) the "parameter" field is optional, so you should treat both representations (with or without the optional "NULL" field) as equivalent.
I just stumbled upon this issue, because the kubernetes certmanager wan't accepting my root CA certificate (which is properly accepted by all browsers) because of the missing "NULL" (that certificate was created using OpenSSL, so it's probably not such a rare case...)

@jhalag
Copy link

jhalag commented Jan 17, 2022

I've just run into this error myself. It is in relation to a third party cert I have no control over, and thus cannot change.

Firefox has no issue loading the certificate (aside from the usual self-signed warning). It "just works". However I'm unable to establish a TLS connection within go at all because it flat-out refuses to parse the certificates. This seems less than desirable from a usability perspective. I don't even have a way to override (such as with InsecureSkipVerify or VerifyPeerCertificate). At this point I don't see how I can connect to this particular host from golang, period.

Edit, 22 days later: As I have no way to use a replace directive on a core module (see #35283 - TLDR; feature was declined), I had to:

  • fork the go codebase
    • split out crypto/tls and crypto/x509
  • also fork the library I am using that utilizes crypto/tls
  • edit the forked library to refer to my crypto/tls fork
  • edit my crypto/tls fork to refer to my crypto/x509 fork
  • comment out lines 867-869 of /src/crypto/x509/parser.go

Now my TLS connection works!

An awful lot of work to go to just in order to get it to accept a non-ideal certificate, whereas a web browser does it without any grief whatsoever. I'm all for standards-compliance - but a tls.config flag to allow it to be lax in this regard would have saved me hours and hours (and now, a much more difficult build process for my project).

@tux-mind
Copy link

TL;DR

different ASN1 values can represent the same algorithm and golang wrongly compare the ASN1 bytes to check such similarity.

Why

I believe that the issue relies upon the ambiguity in RFC 5280:

   AlgorithmIdentifier  ::=  SEQUENCE  {
        algorithm               OBJECT IDENTIFIER,
        parameters              ANY DEFINED BY algorithm OPTIONAL  }

Section 4.1.1.2:

This field MUST contain the same algorithm identifier as the
signature field in the sequence tbsCertificate (Section 4.1.2.3).

Section 4.1.2.3:

This field MUST contain the same algorithm identifier as the
signatureAlgorithm field in the sequence Certificate (Section 4.1.1.2).
The contents of the optional parameters field will vary according to the
algorithm identified.

As you can see, it is not clear whether it refers to the entire sequence, including the parameters, or just the OBJECT IDENTIFIER specifying the algorithm itself.

Regardless of this ambiguity, the RFC doesn't define same, and I can assume that the similarity check depends on the algorithm type.
For instance, in Section 2.2.1 of RFC 3279, which defines :sha1WithRSAEncryption, you can read that:

When any of these three OIDs appears within the ASN.1 type
AlgorithmIdentifier, the parameters component of that type SHALL be
the ASN.1 type NULL.

The above implies that

   18:d=2  hl=2 l=  11 cons: SEQUENCE          
   20:d=3  hl=2 l=   9 prim: OBJECT            :sha1WithRSAEncryption

and

  471:d=1  hl=2 l=  13 cons: SEQUENCE          
  473:d=2  hl=2 l=   9 prim: OBJECT            :sha1WithRSAEncryption
  484:d=2  hl=2 l=   0 prim: NULL

Are the same, satisfying Section 4.1.1.2 and Section 4.1.2.3.

This is also true for sha1, sha224, sha256, sha384 and sha512 according to Section 2.1 of RFC 4055:

All implementations MUST accept both NULL and absent parameters as
legal and equivalent encodings.

In conclusion, it appears that different ASN1 values can represent the same algorithm and golang wrongly compare the ASN1 bytes to check such similarity.

Thank you for all your help and effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants