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

[fhofmannCF]

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/[email protected]/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.

[mknyszek]

Would you be willing to send a patch? :)

[fhofmannCF]

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.