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: buildExtensions issues certificates rejected by Mozilla NSS #22616

Closed
pcarrier opened this issue Nov 7, 2017 · 6 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@pcarrier
Copy link

pcarrier commented Nov 7, 2017

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

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/pcarrier/go"
GORACE=""
GOROOT="/home/pcarrier/opt/go"
GOTOOLDIR="/home/pcarrier/opt/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build684866675=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Generated a CA and used it to issue a server certificate using main.go shared @ https://bugzilla.mozilla.org/show_bug.cgi?id=1415181 ; installed the root CA in Firefox by navigating to it; tried to visit an HTTPS website using the server certificate.

What did you expect to see?

Connections going through.

What did you see instead?

Firefox erroring out with SEC_ERROR_BAD_DER.


Based on Firefox's feedback, the problem is in the following snippet:

	if (len(template.PermittedDNSDomains) > 0 || len(template.ExcludedDNSDomains) > 0) &&
		!oidInExtensions(oidExtensionNameConstraints, template.ExtraExtensions) {
		ret[n].Id = oidExtensionNameConstraints
		ret[n].Critical = template.PermittedDNSDomainsCritical

		var out nameConstraints

		out.Permitted = make([]generalSubtree, len(template.PermittedDNSDomains))
		for i, permitted := range template.PermittedDNSDomains {
			out.Permitted[i] = generalSubtree{Name: permitted}
		}
		out.Excluded = make([]generalSubtree, len(template.ExcludedDNSDomains))
		for i, excluded := range template.ExcludedDNSDomains {
			out.Excluded[i] = generalSubtree{Name: excluded}
		}

		ret[n].Value, err = asn1.Marshal(out)
		if err != nil {
			return
		}
		n++
	}

If ExcludedDNSDomains is empty, the DER shouldn't contain a second empty sequence.
out.Excluded should be nil instead of []generalSubtree{}.

Apparently Mozilla's strict DER checking rejects the certificate chain as a result.

I will update this ticket once confirmed.

@pcarrier
Copy link
Author

pcarrier commented Nov 7, 2017

Issue confirmed, "fixed" cert generator is included @ https://bugzilla.mozilla.org/show_bug.cgi?id=1415181
The broken certs were replaced in place by the fixed certs @ https://gcarrier.fr/certs

@odeke-em
Copy link
Member

odeke-em commented Nov 7, 2017

/cc @agl @FiloSottile

@odeke-em odeke-em changed the title crypto/x509/x509.go: buildExtensions issues certificates rejected by Mozilla NSS crypto/x509: buildExtensions issues certificates rejected by Mozilla NSS Nov 7, 2017
@agl
Copy link
Contributor

agl commented Nov 7, 2017

Already have a fix for this in https://go-review.googlesource.com/c/go/+/74271

@agl agl self-assigned this Nov 7, 2017
@odeke-em
Copy link
Member

odeke-em commented Nov 7, 2017

Awesome foresight @agl!

@odeke-em odeke-em added this to the Go1.10 milestone Nov 7, 2017
@odeke-em odeke-em added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 7, 2017
@gopherbot
Copy link

Change https://golang.org/cl/74270 mentions this issue: vendor: add golang.org/x/crypto/cryptobyte

@gopherbot
Copy link

Change https://golang.org/cl/74271 mentions this issue: crypto/x509: handle name constraints with cryptobyte

gopherbot pushed a commit that referenced this issue Nov 8, 2017
This change adds the cryptobyte package from x/crypto at git revision
faadfbd.

Updates #22616, #15196

Change-Id: Iffd0b022ca129d340ef429697e05b581f04e5c4f
Reviewed-on: https://go-review.googlesource.com/74270
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Nov 12, 2018
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants