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: server gcmCipher appears to do erroneous check for padding size #18953

Closed
twitchyliquid64 opened this issue Feb 6, 2017 · 10 comments
Closed
Milestone

Comments

@twitchyliquid64
Copy link

@twitchyliquid64 twitchyliquid64 commented Feb 6, 2017

I think the problem is here: https://github.com/golang/crypto/blob/master/ssh/cipher.go#L347

Based on my reading of the RFC, padding up to 255 is permitted. https://tools.ietf.org/html/rfc4253#section-6

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

go version devel +4cce27a3fa Sat Jan 21 03:20:55 2017 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH=""
GORACE=""
GOROOT="/usr/lib/google-golang"
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build525614664=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

  1. Setup key: ssh-keygen -f key -N '' -t rsa
  2. Run server: https://play.golang.org/p/yP-jDiYz86
  3. SSH using a client that has more padding (ask jsonp@ or agl@ which one I am referring to - dont think I can write it here)

What did you expect to see?

Successful SSH login.

What did you see instead?

Server has error "ssh: illegal padding 79".
Client says server closed connection.

@adg
Copy link
Contributor

@adg adg commented Feb 6, 2017

@twitchyliquid64
Copy link
Author

@twitchyliquid64 twitchyliquid64 commented Feb 6, 2017

Hmmm. I tried removing the check and testing to see if it works. Now I get input_userauth_error: bad message during authentication: type 20 on the client. Hmmmm.

Likely something more going on here. Regardless, I think it warrants investigation as this client is pretty widely used.

I guess now I cannot be sure if the problem is in Go or in this client.

@odeke-em odeke-em changed the title ssh server gcmClient appears to do erroneous check for padding size x/crypto/ssh: server gcmCipher appears to do erroneous check for padding size Feb 6, 2017
@hanwen
Copy link
Contributor

@hanwen hanwen commented Feb 6, 2017

there is a test under test/ which runs all ciphers against openSSH. If the cipher doesn't follow spec, it should fail.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Feb 6, 2017

for reference, AES-GCM was reviewed here: https://codereview.appspot.com/57720043/

@damz
Copy link

@damz damz commented Feb 16, 2017

For reference, I'm hitting this on OpenSSH 7.4p1:

$ ssh -V
OpenSSH_7.4p1 Debian-6, OpenSSL 1.0.2k  26 Jan 2017
@gopherbot gopherbot added this to the Unreleased milestone Mar 21, 2017
@skalle
Copy link

@skalle skalle commented Jun 22, 2017

I'm having something similar happening to me.

Scenario:

I'm running a Golang SSH server on localhost for tests.

  • Connecting with the Golang SSH client -> Works fine no problems.
  • Using the OpenSSH client -> Errors out with "ssh: illegal padding 71" ..

Changing the gcmReadPacket to skip the max padding size:

https://github.com/golang/crypto/blob/master/ssh/cipher.go#L395

padding := plain[0]
if padding < 4 || padding >= 20 {
	return nil, fmt.Errorf("ssh: illegal padding %d", padding)
}

To:
padding := plain[0]
if padding < 4 {
return nil, fmt.Errorf("ssh: illegal padding %d", padding)
}

Makes the OpenSSH client work fine.

My Q is what's different with the gcm not allowing >= 20 bytes of padding?

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jun 22, 2017

GCM was added in https://codereview.appspot.com/57720043/ , by me.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jun 22, 2017

20 check was present in PS1 too

https://codereview.appspot.com/57720043/diff/20001/ssh/cipher.go

assumption: I put this in because the writing side only needs up to 19 bytes of padding (https://codereview.appspot.com/57720043/diff/20001/ssh/cipher.go, line 249).

https://tools.ietf.org/html/rfc5647 says

byte padding_length; // 4 <= padding_length < 256

so the padding can be larger. Should probably check how OpenSSH calculates the padding to make sure we are following their spec.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 6, 2017

CL https://golang.org/cl/47590 mentions this issue.

tg123 added a commit to tg123/sshpiper that referenced this issue Jul 29, 2017
The writing side would generate a maximum of 19 bytes of padding, so
the reading side erroneously checked this. However, RFC 5647 specifies
255 as the maximum amount of padding for AES-GCM.

Fixes golang/go#18953.

Change-Id: I416b0023c6e4cbd91a6a1b4214a03f1663b77248
Reviewed-on: https://go-review.googlesource.com/47590
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jul 6, 2018
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
The writing side would generate a maximum of 19 bytes of padding, so
the reading side erroneously checked this. However, RFC 5647 specifies
255 as the maximum amount of padding for AES-GCM.

Fixes golang/go#18953.

Change-Id: I416b0023c6e4cbd91a6a1b4214a03f1663b77248
Reviewed-on: https://go-review.googlesource.com/47590
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
The writing side would generate a maximum of 19 bytes of padding, so
the reading side erroneously checked this. However, RFC 5647 specifies
255 as the maximum amount of padding for AES-GCM.

Fixes golang/go#18953.

Change-Id: I416b0023c6e4cbd91a6a1b4214a03f1663b77248
Reviewed-on: https://go-review.googlesource.com/47590
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.