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

crypto/x509: buildExtensions issues certificates rejected by Mozilla NSS #22616

pcarrier opened this issue Nov 7, 2017 · 6 comments
Assignees
Milestone

Comments

@pcarrier
Copy link

@pcarrier 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 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 odeke-em commented Nov 7, 2017

@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 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 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 label Nov 7, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 8, 2017

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

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 8, 2017

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>
@gopherbot gopherbot closed this in d005736 Nov 12, 2017
@golang golang locked and limited conversation to collaborators Nov 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.