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

crypto/x509: ParseCertificate and ParseCertificates return different errors with a single bad certificate supplied #43113

Open
LujunWeng opened this issue Dec 10, 2020 · 2 comments
Open

crypto/x509: ParseCertificate and ParseCertificates return different errors with a single bad certificate supplied #43113

LujunWeng opened this issue Dec 10, 2020 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@LujunWeng
Copy link

LujunWeng commented Dec 10, 2020 •
edited

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

$ go version
go version go1.15.6 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
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/******/Library/Caches/go-build"
GOENV="/Users/******/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/******/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/******/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/nm/1lmrr0s56lvb_00hv_ydg3300000gn/T/go-build015401011=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"crypto/x509"
	"encoding/pem"
	"fmt"
)

const badCerts = `-----BEGIN CERTIFICATE-----
-----END CERTIFICATE-----`

func useParseCertificate(certpem string) {
	block, _ := pem.Decode([]byte(certpem))
	if block == nil {
		fmt.Printf("Failed in parsing pem string\n")
		return
	}

	cert, err := x509.ParseCertificate(block.Bytes)
	fmt.Printf("Certificate: %v, Error: %v\n", cert, err)
}

func useParseCertificates(certpem string) {
	block, _ := pem.Decode([]byte(certpem))
	if block == nil {
		fmt.Printf("Failed in parsing pem string\n")
		return
	}

	certs, err := x509.ParseCertificates(block.Bytes)
	fmt.Printf("Certificates: %v, Error: %v\n", certs, err)
}

func main() {
	useParseCertificate(badCerts)
	useParseCertificates(badCerts)
}

What did you expect to see?

Both x509.ParseCertificate and x509.ParseCertificates return non-nil error.

What did you see instead?

x509.ParseCertificate returns a non-nil error while x509.ParseCertificates returns nil for error.
Output

Certificate: <nil>, Error: asn1: syntax error: sequence truncated
Certificates: [], Error: <nil>

It could be argued that x509.ParserCertificates returns empty slice in this case, but I don't think it can justify a "nil" error. Especially, the returned results are inconsistent here.
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 11, 2020
@toothrot toothrot added this to the Backlog milestone Dec 11, 2020
@toothrot
Copy link
Contributor

/cc @FiloSottile

@antong
Copy link
Contributor

antong commented Aug 20, 2021

x509.ParseCertificates is for parsing a sequence of concatenated, DER encoded certificates. It makes no sense to parse a single certificate PEM block and feed the DER to x509.ParseCertificates. The block contains only a single certificate. It is common to have multiple certificates in the same text, but then you would have concatenated PEM blocks. You would then decode the PEM blocks in a loop, checking for the block type (i.e., "CERTIFICATE"), and calling x509.ParseCertificate separately for each parsed block. Look at x509.AppendCertsFromPEM for an example.

The issue is about how the ParseCertificate and ParseCertificates functions behave. These take DER as input, so the initial PEM or invalid text form is irrelevant. Here is an equivalent, reduced version of the code example in the issue description, with the text and PEM decoding removed: https://play.golang.org/p/qC-VAaodZ9T

This shows how the functions don't really know about the invalid "badCerts". ParseCertificates does not have the information that there ever was an ill-formed PEM certificate. I would say it is the expected behavior for x509.ParseCertificates to return the empty slice without errors when given a zero length input. It is much like that splitting the empty string gives an empty slice. Bonus: And it has the distributive property "parse(der1+der2) = parse(der1)+parse(der2)".

@someview someview mentioned this issue May 22, 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
3 participants
@toothrot @antong @LujunWeng