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.

Sign up for GitHub

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: The if's condition checks that err is not nil but returns nil #49135

Closed
tenntenn opened this issue Oct 24, 2021 · 5 comments
Closed

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

tenntenn opened this issue Oct 24, 2021 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@tenntenn
Copy link
Contributor

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 seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 24, 2021
@seankhliao
Copy link
Member

cc @FiloSottile

@ulikunitz
Copy link
Contributor

ulikunitz commented Oct 25, 2021 •
edited

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
Copy link

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
Copy link

This issue can be closed now?

@seankhliao seankhliao mentioned this issue Jan 12, 2022
Closed
@zpavlinovic
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@tenntenn @zpavlinovic @ulikunitz @gopherbot @seankhliao @guodongli-google