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: invalid authority key identifier parsing older root "Starfield Class 2 Certification Authority" #46854

Closed
joeshaw opened this issue Jun 21, 2021 · 4 comments
Closed

crypto/x509: invalid authority key identifier parsing older root "Starfield Class 2 Certification Authority" #46854

joeshaw opened this issue Jun 21, 2021 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.17

Comments

@joeshaw
Copy link
Contributor

joeshaw commented Jun 21, 2021 •
edited

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

$ go version
go version devel go1.17-7a5e704 Mon Jun 21 14:58:02 2021 +0000 darwin/amd64

Does this issue reproduce with the latest release?

It does not reproduce with Go 1.16. It only occurs on tip on or after 51ff3a6, when the certificate parser was rewritten.

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

darwin/amd64, macOS 11.4. It has also been reproduced on Linux.

What did you do?

Given the sample program:

package main

import (
	"crypto/x509"
	"encoding/pem"
	"io/ioutil"
	"log"
	"os"
)

func main() {
	d, err := ioutil.ReadFile(os.Args[1])
	if err != nil {
		log.Fatal(err)
	}

	block, _ := pem.Decode(d)
	_, err = x509.ParseCertificate(block.Bytes)
	if err != nil {
		log.Fatal(err)
	}
}

And the "Starfield Class 2 Certification Authority" certificate: https://crt.sh/?q=d73494e3446b02167573b3cde3ae1c8584ac26e15e45ac3ec0326708425d90fb

Running this program on Go 1.16.5 produces no error. On tip (7a5e704) following 1.17beta1, you get the error x509: invalid authority key identifier.

What did you expect to see?

The program should terminate without error.

What did you see instead?

x509: invalid authority key identifier
@joeshaw
Copy link
Contributor Author

joeshaw commented Jun 21, 2021 •
edited

Old parser code:

type authKeyId struct {
        Id []byte `asn1:"optional,tag:0"`
}

// RFC 5280, 4.2.1.1
var a authKeyId
if rest, err := asn1.Unmarshal(e.Value, &a); err != nil {
	return nil, err
} else if len(rest) != 0 {
	return nil, errors.New("x509: trailing data after X.509 authority key-id")
}
out.AuthorityKeyId = a.Id

New parser code:

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

This certificate does have a weird structure compared to modern certificates in the wild. OpenSSL renders it as:

X509v3 Authority Key Identifier:
    DirName:/L=ValiCert Validation Network/O=ValiCert, Inc./OU=ValiCert Class 2 Policy Validation Authority/CN=http://www.valicert.com//emailAddress=info@valicert.com
    serial:01

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 21, 2021
@toothrot
Copy link
Contributor

/cc @rolandshoemaker @FiloSottile

@toothrot toothrot added this to the Backlog milestone Jun 21, 2021
@rolandshoemaker
Copy link
Member

Blergh. This is caused by the new parser being somewhat stricter about what it accepts and sort of a bug. Whereas the old parser would allow SEQUENCEs to lack expected fields, and contain unexpected trailing fields (the latter of which is a perfectly reasonable thing to do in ASN.1).

We're expecting an optional field to be present (because it's the only actual field that is actually used) and ignoring other optional fields, really we shouldn't do the former but should continue to do the latter. I'll write up a fix for this.

@rolandshoemaker rolandshoemaker added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 22, 2021
@rolandshoemaker rolandshoemaker self-assigned this Jun 22, 2021
@rolandshoemaker rolandshoemaker modified the milestones: Backlog, Go1.17 Jun 25, 2021
@gopherbot
Copy link

Change https://golang.org/cl/331689 mentions this issue: crypto/x509: don't fail on optional auth key id fields

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
4 participants
@toothrot @joeshaw @rolandshoemaker @gopherbot