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: CreateCertificate creates invalid serial number field #33310

Closed
azsn opened this issue Jul 26, 2019 · 4 comments
Closed

crypto/x509: CreateCertificate creates invalid serial number field #33310

azsn opened this issue Jul 26, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@azsn
Copy link

azsn commented Jul 26, 2019

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

go1.12.7 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.7/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
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?

I created an x509 certificate with x509.CreateCertificate from crypto/x509 with the following code (modified from Martian's mitm NewAuthority function):

package main

import (
	"bytes"
	"crypto/rand"
	"crypto/rsa"
	"crypto/sha1"
	"crypto/x509"
	"crypto/x509/pkix"
	"fmt"
	"log"
	"math/big"
	"time"
)

var MaxSerialNumber = big.NewInt(0).SetBytes(bytes.Repeat([]byte{255}, 20))

func main() {
	name := "Test name"
	organization := "Test Org"
	validity := time.Duration(24 * time.Hour)

	priv, err := rsa.GenerateKey(rand.Reader, 2048)
	if err != nil {
		log.Fatal(err)
	}
	pub := priv.Public()

	// Subject Key Identifier support for end entity certificate.
	// https://www.ietf.org/rfc/rfc3280.txt (section 4.2.1.2)
	pkixpub, err := x509.MarshalPKIXPublicKey(pub)
	if err != nil {
		log.Fatal(err)
	}
	h := sha1.New()
	h.Write(pkixpub)
	keyID := h.Sum(nil)

	serial := MaxSerialNumber

	tmpl := &x509.Certificate{
		SerialNumber: serial,
		Subject: pkix.Name{
			CommonName:   name,
			Organization: []string{organization},
		},
		SubjectKeyId:          keyID,
		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
		BasicConstraintsValid: true,
		NotBefore:             time.Now().Add(-validity),
		NotAfter:              time.Now().Add(validity),
		DNSNames:              []string{name},
		IsCA:                  true,
	}

	fmt.Println(MaxSerialNumber)
	fmt.Println(serial)

	raw, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, pub, priv)
	fmt.Println(err, len(raw), raw)
}

(Unfortunately, this won't run in play.golang.org; it errors with process took too long.)

What did you expect to see?

A certificate with 20 bytes of serial number (which is the maximum allowed) where all 20 bytes are 255.

What did you see instead?

A certificate with 21 bytes of serial number, specifically, a 0 byte followed by 20 255 bytes. The field length correctly indicates 21 bytes, but some software will refuse to read a certificate with that many bytes.

The leading 0 byte is added whenever the big-end byte is >= 128. This can be seen by changing serial := MaxSerialNumber to serial := big.NewInt(127) and then serial := big.NewInt(128). The 127 serial number will have a field length of 1 byte, followed by the single byte 127. The 128 serial number will have a field length of 2 bytes, followed by the the two bytes 0 128.

@julieqiu julieqiu changed the title crypto/x509 CreateCertificate creates invalid serial number field crypto/x509: CreateCertificate creates invalid serial number field Jul 29, 2019
@julieqiu
Copy link
Member

/cc @FiloSottile

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 29, 2019
@FiloSottile FiloSottile added this to the Go1.14 milestone Jul 30, 2019
@FiloSottile FiloSottile self-assigned this Jul 30, 2019
@FiloSottile
Copy link
Contributor

What you are observing is a quirk of ASN.1 encoding. Numbers that start with a byte higher than 127 are negative numbers, so a sequence of 20 255 bytes would be negative, which is also not something that conformant CAs are supposed to do. To express a positive number that would otherwise start with a byte higher than 127, we have to add a 0 byte in front of it.

This means that the highest serial number you can fit in 20 bytes is []byte{127, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255}.

@azsn
Copy link
Author

azsn commented Oct 1, 2019

The reason for adding a 0 makes sense, but given that some software (in my case, Mozilla's NSS library) fails to read certificates with more than 20 serial number bytes, one option is to not add the 0 if it results in 21 total bytes. The RFC says

 Note: Non-conforming CAs may issue certificates with serial numbers
   that are negative, or zero.  Certificate users SHOULD be prepared to
   gracefully handle such certificates.

so leaving a negative serial number is not ideal, but software should be prepared to handle that situation.

@FiloSottile
Copy link
Contributor

We can't turn a number negative against the wishes of the application. If you want to set a negative number, you can set SerialNumber to a negative big.Int.

@golang golang locked and limited conversation to collaborators Oct 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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

4 participants