Skip to content

crypto/tls: segfault when calling tlsrsakex.IncNonDefault() #65991

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
Closed
michaelbeaumont opened this issue Feb 28, 2024 · 9 comments
Closed

crypto/tls: segfault when calling tlsrsakex.IncNonDefault() #65991

michaelbeaumont opened this issue Feb 28, 2024 · 9 comments
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@michaelbeaumont
Copy link
Contributor

michaelbeaumont commented Feb 28, 2024 •
edited
Loading

Go version

go version go1.22.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN='/home/mike/.local/bin'
GOCACHE='/home/mike/.cache/go-build'
GOENV='/home/mike/.config/go/env'
GOEXE=''
GOEXPERIMENT='boringcrypto'
GOFLAGS='-trimpath -tags=opa_no_oci'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/mike/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/mike/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/mike/go/pkg/mod/golang.org/[email protected]'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/mike/go/pkg/mod/golang.org/[email protected]/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/home/mike/projects/project/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1264452044=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Tried to connect to a postgres DB over TLS using jackc/pgx (see stack trace).

What did you see happen?

A panic when crypto/tls calls internal/godebug.(*Setting).IncNonDefault:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x552252]
goroutine 1 [running]:
internal/godebug.(*Setting).IncNonDefault(0x32c3a60?)
	internal/godebug/godebug.go:102 +0x12
crypto/tls.(*clientHandshakeState).pickCipherSuite(0xc00103f450)
	crypto/tls/handshake_client.go:530 +0xbd
crypto/tls.(*clientHandshakeState).processServerHello(0xc00103f450)
	crypto/tls/handshake_client.go:755 +0x25
crypto/tls.(*clientHandshakeState).handshake(0xc00103f450)
	crypto/tls/handshake_client.go:442 +0x35
crypto/tls.(*Conn).clientHandshake(0xc000694e08, {0x475baf8, 0xc00054b1d0})
	crypto/tls/handshake_client.go:274 +0x685
crypto/tls.(*Conn).handshakeContext(0xc000694e08, {0x475b8c8, 0x6d0e820})
	crypto/tls/conn.go:1553 +0x3cb
crypto/tls.(*Conn).HandshakeContext(...)
	crypto/tls/conn.go:1493
crypto/tls.(*Conn).Handshake(...)
	crypto/tls/conn.go:1477
crypto/tls.(*Conn).Write(0x30d31c0?, {0xc001002e40, 0x21, 0x40})
	crypto/tls/conn.go:1194 +0xe5
github.com/jackc/pgx/v5/pgproto3.(*Frontend).Flush(0xc0007ca008)
	github.com/jackc/pgx/[email protected]/pgproto3/frontend.go:88 +0x3d
github.com/jackc/pgx/v5/pgconn.(*PgConn).flushWithPotentialWriteReadDeadlock(0xc001230008)
	github.com/jackc/pgx/[email protected]/pgconn/pgconn.go:1818 +0x73
github.com/jackc/pgx/v5/pgconn.connect({0x475bb68, 0xc000020070}, 0xc0007ab680, 0xc000b2c160, 0x0)
	github.com/jackc/pgx/[email protected]/pgconn/pgconn.go:337 +0xbd3
github.com/jackc/pgx/v5/pgconn.ConnectConfig({0x475b8c8, 0x6d0e820}, 0xc0007ab680)
	github.com/jackc/pgx/[email protected]/pgconn/pgconn.go:176 +0x4f3
github.com/jackc/pgx/v5.connect({0x475b8c8?, 0x6d0e820?}, 0xc0007ab680)
	github.com/jackc/pgx/[email protected]/conn.go:257 +0x34b
github.com/jackc/pgx/v5.ConnectConfig({0x475b8c8, 0x6d0e820}, 0xc0007ab560)
	github.com/jackc/pgx/[email protected]/conn.go:141 +0xf0
github.com/jackc/pgx/v5/stdlib.(*driverConnector).Connect(0xc000fd6b28, {0x475b8c8, 0x6d0e820})
	github.com/jackc/pgx/[email protected]/stdlib/sql.go:365 +0xcd
database/sql.(*DB).conn(0xc000c7d790, {0x475b8c8, 0x6d0e820}, 0x1)
	database/sql/sql.go:1415 +0x71e
database/sql.(*DB).PingContext.func1(0xc8?)
	database/sql/sql.go:883 +0x3a
database/sql.(*DB).retry(0xc001087288?, 0xc001087258)
	database/sql/sql.go:1566 +0x42
database/sql.(*DB).PingContext(0xc000c7d790, {0x475b8c8, 0x6d0e820})
	database/sql/sql.go:882 +0x89
database/sql.(*DB).Ping(...)
	database/sql/sql.go:900
...

What did you expect to see?

Expected no segfault/panic. Looking at crypto/tls, it's my suspicion that, due to having goboring enabled and thus needing FIPS, tlsrsakex.Value() has not been called when tlsrsakex.IncNonDefault() is called. This could probably be triggered by having non default .CipherSuites set as well. Perhaps the underlying issue is that RSA ciphers aren't filtered out of fipsCipherSuites() in the first place, which is what allows the branch with IncNonDefault() to even be taken.
@seankhliao
Copy link
Member

cc @golang/security

@FiloSottile
Copy link
Contributor

The good news is that the IncNonDefault call is gated by hs.c.config.CipherSuites == nil, so it's not reachable by having non default CipherSuites.

Definitely an oversight in Go+BoringCrypto mode.

@FiloSottile
Copy link
Contributor

@gopherbot please open a Go 1.22 backport issue. This is a severe regression in Go+BoringCrypto mode.

@gopherbot gopherbot mentioned this issue Feb 28, 2024
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #65994 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@slonka slonka mentioned this issue Feb 28, 2024
Merged
5 tasks
@dmitshur dmitshur added this to the Go1.23 milestone Feb 28, 2024
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 29, 2024
@brianstrauch brianstrauch mentioned this issue Mar 8, 2024
Merged
1 task
@michaelbeaumont
Copy link
Contributor Author

michaelbeaumont commented Apr 18, 2024 •
edited
Loading

@FiloSottile Not sure who to ping on this, would it be enough to just solve the segfault?

@bartsmykla bartsmykla mentioned this issue Apr 26, 2024
Merged
5 tasks
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/582315 mentions this issue: crypto/tls: don't call tlsrsakex.IncNonDefault with FIPS

@etodd etodd mentioned this issue May 2, 2024
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 15, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/586755 mentions this issue: crypto/tls: ensure GODEBUGs are initialized

gopherbot pushed a commit that referenced this issue May 22, 2024
@FiloSottile @gopherbot
crypto/tls: ensure GODEBUGs are initialized
1d3d7d3 
IncNonDefault panics if Value was not called. That's too much DoS risk
in crypto/tls, when the call to Value is distant from the call to
IncNonDefault (see #65991). Value is cheap, though, so we can just call
it before each isolated IncNonDefault.

Change-Id: I6dbed345381e60e029b0a5ef2232e846aa089736
Reviewed-on: https://go-review.googlesource.com/c/go/+/586755
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
@lyoung-confluent
Copy link

FYI for anyone scrolling to the bottom of this the cherry-pick/backport of this patch into a 1.22 release is being tracked in #65994

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/593395 mentions this issue: [release-branch.go1.22] crypto/tls: don't call tlsrsakex.IncNonDefault with FIPS

gopherbot pushed a commit that referenced this issue Jun 24, 2024
@michaelbeaumont @joedian
[release-branch.go1.22] crypto/tls: don't call tlsrsakex.IncNonDefaul…
81fc616 
…t with FIPS

We haven't called tlsrsakex.Value() yet at this point if we're using
FIPS, like if CipherSuites != nil. This adds needFIPS as a gate next to
CipherSuites != nil. FIPS specifies suites that would be skipped if
tlsarsakex were set.

For #65991.
Fixes #65994.

Change-Id: I8070d8f43f27c04067490af8cc7ec5e787f2b9bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/582315
Reviewed-by: Filippo Valsorda <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
TryBot-Bypass: Filippo Valsorda <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
(cherry picked from commit 78e50d0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/593395
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
@gabyhelp gabyhelp mentioned this issue Nov 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
7 participants
@cagedmantis @FiloSottile @dmitshur @michaelbeaumont @gopherbot @seankhliao @lyoung-confluent