crypto/x509: The if's condition checks that err is not nil but returns nil

[tenntenn]

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

$ go1.17.2 version
go version go1.17.2 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

$ go1.17.2 env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/tenntenn/Library/Caches/go-build"
GOENV="/Users/tenntenn/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/tenntenn/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/tenntenn/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/tenntenn/sdk/go1.17.2"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/tenntenn/sdk/go1.17.2/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/usr/local/gotip/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/dr/gn16sh9935zcz37xt2ghpk480000gn/T/go-build3211983345=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I had run nilerr which is a static analysis tool. It can find a return statement returning nil when error is not nil as a following.

func f() error {
	err := do()
	if err != nil {
		return nil // miss
	}
}

I had run nilerr for standard packages in Go1.17.2. These output are false positive mostly. But I think crypt/x509 package has a wrong return statement.

$ go1.17.2 vet -vettool=`which nilerr` std
# io/fs
/Users/tenntenn/sdk/go1.17.2/src/io/fs/glob.go:44:4: error is not nil (line 43) but it returns nil
/Users/tenntenn/sdk/go1.17.2/src/io/fs/glob.go:93:3: error is not nil (line 91) but it returns nil
# archive/zip
/Users/tenntenn/sdk/go1.17.2/src/archive/zip/reader.go:265:3: error is nil (line 261) but it returns error
# crypto/x509
/Users/tenntenn/sdk/go1.17.2/src/crypto/x509/x509.go:1293:3: error is not nil (line 1291) but it returns nil
# go/format
/Users/tenntenn/sdk/go1.17.2/src/go/format/internal.go:51:3: error is nil (line 43) but it returns error
# go/build
/Users/tenntenn/sdk/go1.17.2/src/go/build/read.go:422:3: error is not nil (line 421) but it returns nil
/Users/tenntenn/sdk/go1.17.2/src/go/build/deps_test.go:541:4: error is not nil (line 539) but it returns nil
# internal/poll
/Users/tenntenn/sdk/go1.17.2/src/internal/poll/fd_unix.go:395:4: error is nil (line 393) but it returns error
# log/syslog
/Users/tenntenn/sdk/go1.17.2/src/log/syslog/syslog.go:259:4: error is nil (line 258) but it returns error
# net/textproto
/Users/tenntenn/sdk/go1.17.2/src/net/textproto/reader.go:170:2: error is not nil (line 163) but it returns nil
# path/filepath
/Users/tenntenn/sdk/go1.17.2/src/path/filepath/match.go:250:4: error is not nil (line 249) but it returns nil
# net/http
/Users/tenntenn/sdk/go1.17.2/src/net/http/header.go:124:4: error is nil (line 122) but it returns error

What did you expect to see?

The if's condition (in crypto/x509/x509.go) checks that err is not nil but returns nil.

	if err != nil {
		return ext, nil
	}
	return ext, nil

https://cs.opensource.google/go/go/+/master:src/crypto/x509/x509.go;l=1293

Is the return statement in the if block is wrong?

What did you see instead?

I don't know what is correct because I don't catch details of code.

[seankhliao]

cc @FiloSottile

[ulikunitz]

Comparing marshalBasicConstraints to marshalExtKeyUsage and marshalCertificatePolicies it looks like an error. But I don't think that the error path is reachable, since I don't know how asn1.Marshal should fail.

It is more concerning that the function would encode maxPathLen < -1 and parseBasicConstraintsExtension would parse that without raising an error. Certficate.isValid() would not detect a problem because it does only a check if the value is not negative.

With a simple test for marshalBasicConstraints I could create following DER value: 30060101ff0201fe, which contains a -2 for pathLen and could parse it with parseBasicConstraintsExtension.

The encoding is in violation of RFC5280 section 4.2.1.9 which defines basic constraints as:

id-ce-basicConstraints OBJECT IDENTIFIER ::=  { id-ce 19 }

   BasicConstraints ::= SEQUENCE {
        cA                      BOOLEAN DEFAULT FALSE,
        pathLenConstraint       INTEGER (0..MAX) OPTIONAL }

[guodongli-google]

This seems to be a TP and have been fixed in https://cs.opensource.google/go/go/+/6b223e872a255b2722ea921c9d42adcbb5d1d4d5. Now it looks like

	ext.Value, err = asn1.Marshal(oids)
	if err != nil {
		return ext, err
	}
	return ext, nil

This code is not very clean though, since it is equivalent to

	ext.Value, err = asn1.Marshal(oids)
	return ext, err

[guodongli-google]

This issue can be closed now?

[zpavlinovic]

Closing due to inactivity and, more importantly, the fact that the bug in OP is fixed now.