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

x/sys/unix: KeyctlString() panics for key types that can legally contain "no payload at all #54498

Open
fhofmannCF opened this issue Aug 17, 2022 · 2 comments
Open

x/sys/unix: KeyctlString() panics for key types that can legally contain "no payload at all #54498

fhofmannCF opened this issue Aug 17, 2022 · 2 comments
Assignees
@mknyszek
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Unreleased

Comments

@fhofmannCF
Copy link

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

$ go version
go version go1.19 linux/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="/home/fhofmann/.cache/go-build"
GOENV="/home/fhofmann/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/fhofmann/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/fhofmann/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/fhofmann/build/KRN/key-play/go.mod"
GOWORK=""
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 -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3959267050=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://go.dev/play/p/60ZtM3V4_Io

This program needs to run on Linux - and not in a dev/play container (where the keyctl syscall is masked).
On a VM, it panics thus:

$ ./main 
panic: runtime error: slice bounds out of range [:-1]

goroutine 1 [running]:
golang.org/x/sys/unix.KeyctlString(0x49aa72?, 0x40bcdd?)
	/home/fhofmann/go/pkg/mod/golang.org/x/sys@v0.0.0-20220808155132-1c4a2a72c664/unix/syscall_linux.go:1399 +0xb8
main.main()
	/home/fhofmann/build/KRN/key-play/main.go:18 +0xc5

This is because unix.KeyctlString() assumes key lengths are always > 0 (and it can "strip the trailing null byte") at the very least. But It is possible for certain key types (keyrings, notably) to be "legally empty", and a unix.KeyctlBuffer() on these will correctly return zero for the length..

What did you expect to see?

Not panic.
Return"", nil (zero-length content, no error). This would be trivially achievable by changing https://github.com/golang/sys/blob/master/unix/syscall_linux.go#L1392,

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

into:

if err != nil || length == 0 {
...

What did you see instead?

Panic in go standard lib. Completely unnecessary.
@gopherbot gopherbot added this to the Unreleased milestone Aug 17, 2022
@seankhliao seankhliao changed the title x/sys: unix.KeyctlString() panics for key types that can legally contain "no payload at all x/sys/unix: KeyctlString() panics for key types that can legally contain "no payload at all Aug 17, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 17, 2022
@mknyszek
Copy link
Contributor

Would you be willing to send a patch? :)

@mknyszek mknyszek assigned fhofmannCF and mknyszek and unassigned fhofmannCF Aug 17, 2022
@fhofmannCF
Copy link
Author

I've done a more thorough investigation there.

So the "real" issue is hinted at by the code comments on KeyctlString() - namely, that (only) the two queries KEYCTL_DESCRIBE and KEYCTL_GET_SECURITY return (C-style) strings.

If KeyctlString() is invoked with those queries, it does the right thing - convert the C-style string (that includes the NULL byte, and is guaranteed to be at least one byte long even when empty) to a go-type string, by stripping the trailing NULL byte.

For other keyctl(2) query commands, especially KEYCTL_READ, both of these assumptions do not hold. Those query commands may sometimes succeed yet return nothing (zero-length buffers), and they may return (binary) data where NULL bytes are part of the data and have no other special meanings.

If such a query command is used with KeyctlString(), that will either panic or corrupt (truncate by one byte) the returned payload buffer.

The "real solution" therefore is to have KeyctlString() test whether the query command is returning a C-style string, and error early if not.

golang/sys#133 would do this.

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

@mknyszek mknyszek

Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@mknyszek @gopherbot @fhofmannCF