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

Jump to bottom

encoding/asn1, crypto/x509: failed to parse Intermediate CA certificate #38737

Open
sebastien-rosset opened this issue Apr 29, 2020 · 8 comments
Open

encoding/asn1, crypto/x509: failed to parse Intermediate CA certificate #38737

sebastien-rosset opened this issue Apr 29, 2020 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@sebastien-rosset
Copy link

sebastien-rosset commented Apr 29, 2020 •
edited

A intermediate CA certificate contains X.509 certificate policy extension with OID 1.2.36.67006527840.666.66.1.1. The fourth oid (67006527840) in that extension is greater than math.MaxInt32, which causes a certificate unmarshaling error. Specifically the error occurs in the asn1.Unmarshal function while trying to unmarshal the certificate policy extension.

The CA cert has been issued by an Internet provider in New Zealand. I am not affiliated with the certifucate authority, and I don't know how that OID was assigned. I cannot force the CA to issue a new CA cert with sub-OIDs that have a value lower than math.MaxInt32; to the best of my understanding there is nothing in the ASN.1 spec that states sub-oids must be lower than 2^31 - 1. I've been able to successfully load the CA certificate using openssl, macOS key chain, and browsers.

I understand it may be challenging to change type ObjectIdentifier []int, but it's also not a good long term option to close this issue.

I saw related issues #30757, #19933, #36881. It looks like in practice there are certificates that have OID values higher than 2^31 - 1. So the reality is golang is unable to process some legitimate certificates.

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

go version go1.14.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/serosset/.cache/go-build"
GOENV="/home/serosset/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/serosset/goroot"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build216516397=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Parse CA certificate:
https://play.golang.org/p/StRBpHZhgDM

Issue narrowed to parsing issue of ASN.1 object identifier:

  1. Encode the ASN.1 Object Identifier 1.2.36.67006527840.666.66.1.1 in DER format using the golang asn1.Marshal function.
  2. Manually verify the DER encoded data is correct. I used 2 different Linux tools to compare the ASN.1 DER encoding with what is produced by asn1.Marshal.
  3. Take the ASN.1 DER encoded byte array and call asn1.Unmarshal.

https://play.golang.org/p/f3mP_NRAiFI

What did you expect to see?

Unmarshal in step (3) should succeed and the same original object identifier should have been retrieved.

Until a long term fix is determined, maybe the golang asn1 (and x509) package documentation should explicitly mention the implementation is not fully compliant with ITU-T Rec X.690, i.e. object identifiers cannot have values higher than 2^31-1. Also, the error message when parsing a certificate is very low-level and does not provide context: asn1: structure error: base 128 integer too large.

What did you see instead?

Step (3) is failing. asn1.Unmarshal fails with error asn1: structure error: base 128 integer too large
@sebastien-rosset sebastien-rosset changed the title Marshal ASN.1 OBJECT IDENTIFIER then unmarshal causes unmarshal error Marshal ASN.1 Object Identifier then unmarshal causes unmarshal error Apr 29, 2020
@sebastien-rosset sebastien-rosset changed the title Marshal ASN.1 Object Identifier then unmarshal causes unmarshal error Fail to Parse Chorus CA certificate Apr 29, 2020
@sebastien-rosset sebastien-rosset changed the title Fail to Parse Chorus CA certificate Fail to Parse Chorus Intermediate CA certificate Apr 29, 2020
@odeke-em odeke-em changed the title Fail to Parse Chorus Intermediate CA certificate crypto/x509: failed to parse Chorus Intermediate CA certificate Apr 29, 2020
@odeke-em odeke-em added early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Apr 29, 2020
@odeke-em odeke-em changed the title crypto/x509: failed to parse Chorus Intermediate CA certificate encoding/asn1, crypto/x509: failed to parse Chorus Intermediate CA certificate Apr 29, 2020
@odeke-em
Copy link
Member

Thank you for this report @sebastien-rosset!

Kindly pinging the crypto squad @katiehockman @FiloSottile

@rolandshoemaker
Copy link
Member

Looks like the original choice made by @agl was to match behavior on all platforms, so the maximum size was limited to 31 bits so that parsing behavior would match on 32 bit and 64 bit builds. In theory this could be supported by simply seeing if casting the parsed int64 -> int caused an overflow and only use the int if it didn't, which was the originally proposed solution to the problem, but this would cause mismatching behavior.

Confusingly this does create a marshal/unmarshal mismatch. asn1.Marshal will happily encode a integer larger than 31 bits, but cannot parse it. It seems like either parsing should be dependent on platform integer size, or marshal/unmarshal should match by limiting encoding to 31 bits on all platforms.

@sebastien-rosset
Copy link
Author

Looks like the original choice made by @agl was to match behavior on all platforms, so the maximum size was limited to 31 bits so that parsing behavior would match on 32 bit and 64 bit builds. In theory this could be supported by simply seeing if casting the parsed int64 -> int caused an overflow and only use the int if it didn't, which was the originally proposed solution to the problem, but this would cause mismatching behavior.

The golang ASN.1 implementation correctly implements the ASN.1 INTEGER type, which can have an arbitrary number of bits: An ASN.1 INTEGER can be written to an int, int32, int64, or *big.Int (from the math/big package).

An object identifier is specified in ITU-T recommendation X.208, which has been superseded bu X.680-683. An object identifier value is semantically an ordered list of object identifier component values:

ObjIdComponent ::=
  NameForm |
  NumberForm |
  NameAndNumberForm

NumberForm ::= number | DefinedValue

Where number can have an arbitrary number of digits. It is not limited to 32 bits and it is not limited to 64 bits either. So it seems that to properly implement unmarshaling of object identifiers, the implementation should do something similar to INTEGER unmarshaling, where the value can be represented either as int32,

Confusingly this does create a marshal/unmarshal mismatch. asn1.Marshal will happily encode a integer larger than 31 bits, but cannot parse it. It seems like either parsing should be dependent on platform integer size, or marshal/unmarshal should match by limiting encoding to 31 bits on all platforms.

@rolandshoemaker
Copy link
Member

Sure, the problem though is that asn1.ObjectIdentifier was defined as a []int, presumably because in the real world it's quite rare that components are ever >int32, and extremely rare that components are >int64. That isn't to say that this doesn't happen, but for whatever reason that was the decision made.

As such this cannot be changed to something that supports arbitrary integer sizes without changing the definition, which would break a lot of software that assumes the definition is []int.

@sebastien-rosset
Copy link
Author

Yes, I understand the challenges of the implementation. However I don't think just because asn1.ObjectIdentifier was defined as a []int, that should mean golang will never be able to parse real-world CA certificates such as the New Zealand example I have provided.

@sebastien-rosset
Copy link
Author

I don't think a solution would involve changing the type of ObjectIdentifier. As you and I pointed out, that would break a lot of code. But it seems this could be done in a way similar to what has been done for integers: the asn1.UnmarshalWithParam() function can take int, int32, int64 and big.Int for the val argument. Similarly, two new types ObjectIdentifierInt64 and ObjectIdentifierBigInt could be defined and the function asn1.UnmarshalWithParam() could have a couple more cases under the switch statement. The asn1.Marshal() function would also have to accept these two new types as input. Optionally there could be a wrapper type. That would cover pure ASN.1 operations.

Under the crypto package, I see 80 use of asn1.ObjectIdentifier.

@mikebros
Copy link

Hi, @sebastien-rosset. I represent the company that is having this issues. Thank you for posting this issues and looking for a resolution.
Can I ask you to please remove the references to our company name and function as we are not comfortable having us directly referenced in this bug report?
Thanks

@sebastien-rosset sebastien-rosset changed the title encoding/asn1, crypto/x509: failed to parse Chorus Intermediate CA certificate encoding/asn1, crypto/x509: failed to parse Intermediate CA certificate May 19, 2020
@sebastien-rosset sebastien-rosset mentioned this issue Jun 24, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/240006 mentions this issue: encoding/asn1: support ObjectIdentifier sub-oid values greater than 2^31-1

@seankhliao seankhliao added this to the Backlog milestone Aug 27, 2022
@rolandshoemaker rolandshoemaker mentioned this issue Mar 6, 2023
Open
@rolandshoemaker rolandshoemaker mentioned this issue Jun 7, 2023
Closed
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
Backlog
Development

No branches or pull requests
6 participants
@odeke-em @mikebros @rolandshoemaker @gopherbot @seankhliao @sebastien-rosset