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

x/crypto/ssh: Marshal silently ignores fields with unsupported types #69944

Open
libmonsoon-dev opened this issue Oct 20, 2024 · 6 comments
Open
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@libmonsoon-dev
Copy link

Go version

go version go1.23.1 linux/amd64

Output of go env in your module/workspace:

$ go env
GO111MODULE='on'
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/daniil_stepanenko/.cache/go-build'
GOENV='/home/daniil_stepanenko/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/daniil_stepanenko/go/pkg/mod'
GOOS='linux'
GOPATH='/home/daniil_stepanenko/go'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/daniil_stepanenko/go/go1.23.1'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/daniil_stepanenko/go/go1.23.1/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/daniil_stepanenko/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/daniil_stepanenko/work/upstream/gomplate/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 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2364282043=/tmp/go-build -gno-record-gcc-switches'

What did you do?

https://go.dev/play/p/D6fgIrq7PpV

What did you see happen?

Marshal succeeds, but its result cannot be used. Unmarshal returns an error.

prog.go:24: ssh: unmarshal error for field E of type PublicKey: unsupported type: int
prog.go:25: ssh: short read
prog.go:36: ssh: unmarshal error for field E of type PublicKey: unsupported type: int
prog.go:37: ssh: short read

What did you expect to see?

ssh.Marshal+ssh.Unmarshal work successfully with rsa.PublicKey (and keys of other algorithms)

Or at least Marshal panics when it encounters a public field without the ssh:"skip" tag on an unsupported type.

@gopherbot gopherbot added this to the Unreleased milestone Oct 20, 2024
@libmonsoon-dev
Copy link
Author

libmonsoon-dev commented Oct 20, 2024

@libmonsoon-dev libmonsoon-dev changed the title x/crypto: ssh.Marshal silently ignores fields with unsupported types x/crypto/ssh: Marshal silently ignores fields with unsupported types Oct 20, 2024
@ianlancetaylor
Copy link
Contributor

This normal for Go marshal/unmarshal functions, because of the way that the reflect package is implemented: you can't unmarshal into an unexported field. We should just document this restriction.

@ianlancetaylor
Copy link
Contributor

Apologies, I think I misunderstood the issue here. I don't think it has to do with unexported types. The playground example is failing because ssh.Marshal doesn't handle the type int, among other types. The function is documented as using the "SSH wire format" but I'm not sure what that is.

CC @golang/security

@libmonsoon-dev
Copy link
Author

libmonsoon-dev commented Oct 21, 2024

Apologies, I think I misunderstood the issue here. I don't think it has to do with unexported types. The playground example is failing because ssh.Marshal doesn't handle the type int, among other types. The function is documented as using the "SSH wire format" but I'm not sure what that is.

CC @golang/security

In my example there are two options. In the first case, as you said, the reason is that there is a field with the int type, in the second because there is a field with the struct type. In both cases, ssh.Marshal simply ignores them. But at the same time, if you pass a structure with a pointer or slice of an inappropriate type, a panic will occur.

I suggest considering 2 proposals:

  1. Panic for any inappropriate type (except for fields with the ssh:"skip" tag) for consistency.
  2. Create ssh.Marshaler and ssh.Unmarshaler interfaces similar to json/xml/etc and implement them for rsa.PublicKey, ecdsa.PublicKey and ecdsa.PublicKey

@libmonsoon-dev
Copy link
Author

RSA workaround
Ed25519 workaround

This code can be used to implement ssh.Marshaler and ssh.Unmarshaler interfaces.

@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants