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

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

x/crypto/ssh: server gcmCipher appears to do erroneous check for padding size #18953

twitchyliquid64 opened this issue Feb 6, 2017 · 10 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@twitchyliquid64
Copy link

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 commented Feb 6, 2017

cc @hanwen @davecheney @agl

@twitchyliquid64
Copy link
Author

twitchyliquid64 commented Feb 6, 2017 •
edited

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 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 commented Feb 6, 2017

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

@damz
Copy link

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 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 commented Jun 22, 2017

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

@hanwen
Copy link
Contributor

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.

@hanwen
Copy link
Contributor

hanwen commented Jul 6, 2017

https://go-review.googlesource.com/c/47590/

@gopherbot
Copy link

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

tg123 pushed a commit to tg123/sshpiper that referenced this issue Jul 29, 2017
@hanwen @tg123
ssh: allow up to 255 bytes of padding in AES-GCM
af611da 
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
@hanwen
ssh: allow up to 255 bytes of padding in AES-GCM
d7c04be 
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
@hanwen
ssh: allow up to 255 bytes of padding in AES-GCM
a878228 
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>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@hanwen
ssh: allow up to 255 bytes of padding in AES-GCM
145f99b 
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>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@hanwen
ssh: allow up to 255 bytes of padding in AES-GCM
b2cd39a 
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>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@hanwen
ssh: allow up to 255 bytes of padding in AES-GCM
6807c8c 
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>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@hanwen
ssh: allow up to 255 bytes of padding in AES-GCM
c181d1a 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@hanwen @damz @skalle @twitchyliquid64 @adg @gopherbot