Skip to content

crypto/x509: SPKI and signature parsers accept non-whole number of bytes #81178

Description

@davidben

Go version

go version go1.27.0 darwin/arm64

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOARCH='arm64'
GOARM64='v8.0'
GOAUTH='netrc'
GOBIN='/Users/davidben/go/bin'
GOCACHE='/Users/davidben/Library/Caches/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/Users/davidben/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/cq/9rwcly3578n38glphc2q47nw003f5q/T/go-build3310819320=/tmp/go-build -gno-record-gcc-switches -fno-common'
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMOD='/dev/null'
GOMODCACHE='/Users/davidben/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPACKAGESDRIVER=''
GOPATH='/Users/davidben/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/davidben/Library/Application Support/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.27.0'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Looked for some way to check if Certificate.Signature was not a multiple of 8 bits. Also checked how Go internally did this check.

What did you see happen?

No check. Instead, Go uses RightAlign on both the signature and the SPKI value, and discards how many bits were padding:

What did you expect to see?

x509.ParsePKIXPublicKey should, at least for known public key algorithms, reject any subjectPublicKey fields that aren't a whole number of bytes.

The combination of x509.ParseCertificate and x509.Certificate.CheckSignature should not accept any signatureValues that aren't a whole number of bytes.

The most pedantically correct thing would probably be for the general parser to accept it because an unknown algorithm might make use of the full space of BIT STRING values, and that enforcement only apply once you dispatch on the algorithm itself. (This is what BoringSSL does.)

However, this makes for a larger state space of values in x509.Certificate. E.g. you would probably need to add some field that captures the number of padding bits. Using BIT STRING instead of OCTET STRING here was clearly just a mistake in X.509. Nothing reasonable would ever use the full space of BIT STRING values. So while I was too timid to reject it early in the parser in our implementations, I would offer my moral support to rejecting it at the ParseCertificate layer. :-)

Relatedly, see #81177

Compatibility-wise, BoringSSL will reject padding bits in these fields, at least for known algorithms. Also Go's use of RightAlign is very unusual. I would expect most folks with lax parsers to keep the bit string left-aligned. That means it's highly unlikely that anyone is relying on this behavior.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    NeedsInvestigationSomeone must examine and confirm this is a valid issue and not a duplicate of an existing one.

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions