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.

Sign up for GitHub

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: DSA code sometimes generates 32 byte signatures, leading to crash. #19424

Closed
vdice opened this issue Mar 6, 2017 · 6 comments
Closed

x/crypto/ssh: DSA code sometimes generates 32 byte signatures, leading to crash. #19424

vdice opened this issue Mar 6, 2017 · 6 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@vdice
Copy link

vdice commented Mar 6, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.5 darwin/amd64

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

GOARCH="amd64"
GOBIN="/Users/vaughndice/work/go/bin"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/vaughndice/work/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.5/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.5/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/j4/8656z8jd0hgcq78_0ykzkf4w0000gn/T/go-build720938580=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

The Builder component of the Workflow PaaS is frequently affected by panics in the underlying golang/x/crypto/ssh library (see deis/builder#462) when handling ssh handshake data.

What did you expect to see?

I'd expect the underlying code to return/handle appropriate error(s) instead of panicking.

What did you see instead?

See deis/builder#462 (comment) for an example of the behavior and full stacktrace.

Local testing:

I went to the area where the panic occurs (https://github.com/golang/crypto/blob/master/ssh/keys.go#L457) and added detection immediately prior:

...
if len(rb) > 20 {
  return nil, errors.New(fmt.Sprintf("Error: length of rb (%v) is greater than 20!", len(rb)))
}

copy(sig[20-len(rb):20], rb)
...

Which, after deploying the resulting builder image into a Kubernetes test cluster, leads to the following (after 2 days in this case):

$ kd logs deis-builder-1885705597-b32kb
Accepted connection.
---> [ERROR] Failed handshake: Error: length of rb (32) is greater than 20!
Accepted connection.
---> [ERROR] Failed handshake: Error: length of rb (32) is greater than 20!
Accepted connection.
---> [ERROR] Failed handshake: Error: length of rb (32) is greater than 20!
Accepted connection.
---> [ERROR] Failed handshake: Error: length of rb (32) is greater than 20!
Accepted connection.
---> [ERROR] Failed handshake: Error: length of rb (32) is greater than 20!

Although I am not familiar with the protocols/conventions of signature/key handling, it definitely appears that this code could be updated to allow for or otherwise handle byte arrays of length greater than the hard-coded 20 currently used, assuming an array of 32 bytes (as seen fairly commonly in logs above) is valid.

If so, would simply returning the error as done above be sufficient? Looking for the best path forward to gracefully handle such cases as opposed to panicking, so that issues such as the aforementioned may be resolved/avoided.
@bradfitz bradfitz changed the title x/crypto/ssh/keys.go: panic: runtime error: slice bounds out of range x/crypto/ssh: panic: runtime error: slice bounds out of range Mar 6, 2017
@bradfitz bradfitz added this to the Unreleased milestone Mar 6, 2017
@hanwen
Copy link
Contributor

hanwen commented Mar 7, 2017

the easy solution is to not use DSA/DSS. It is an uncommon algorithm, and it was a mistake to have added it to SSH in the first place. ECDSA is faster, more compact, and ed25519 is more secure if you're paranoid about the NSA

DSA/DSS is specified to use 160 bit integers, per https://tools.ietf.org/html/rfc4253#ref-FIPS-186-2 page 14, so just pretending it's 32 bytes sometimes is probably not the right approach.

Is this crash reproducible or non-deterministic?

@vdice
Copy link
Author

vdice commented Mar 7, 2017 •
edited

@hanwen thank you for the background information.

Is this crash reproducible or non-deterministic?

Alas, we haven't been able to pin down the scenario leading to the crash -- but it can reliably hit after some time running Workflow (more background listed in linked issue deis/builder#462)

the easy solution is to not use DSA/DSS

I'm unclear; would this mean a change in the x/crypto/ssh library itself? Assuming it wouldn't be a contentious change and there is a possibility it could happen fairly soon, that would be great.

so just pretending it's 32 bytes sometimes is probably not the right approach

Agreed -- but perhaps some sort of handling can be added to avoid otherwise panicking in this case (and any other case of unexpected byte array size)? Maybe just returning an error as is done in the description and claiming such byte arrays are unsupported/unprocessable?

@hanwen
Copy link
Contributor

hanwen commented Mar 8, 2017

your server is crashing in a signing operation of the handshake, which uses the host private key. You are using a DSA (aka ssh-dss) key. You could use a RSA, ECDSA or ed25519 key instead.

@vdice
Copy link
Author

vdice commented Mar 8, 2017

Thank you @hanwen ; so is DSA then not supported in this library? If it is not, it seems we need some logic to stop processing (handshaking) earlier (return an error saying DSA is not supported) instead of panicking. If it is, it sounds like there is a corner case/add'l logic needed to fully support all variants. How does that sound?

@hanwen
Copy link
Contributor

hanwen commented Mar 8, 2017

I'm not sure if I got this across - whether or not to use DSA key is a choice of the server, which you control, ie. should you change that decision, there is no logic changes you need: the client will adapt to what host key the server offers.

I'm saying 1) clearly there is a bug in the DSA code somewhere 2) if you use a different type of hostkey you will be immune to this bug. 3) DSA is being deprecated in openSSH, so it may not be a bad idea to move off those.

(see http://security.stackexchange.com/questions/112802/why-openssh-deprecated-dsa-keys)

@vdice vdice mentioned this issue Mar 8, 2017
Merged
@hanwen hanwen changed the title x/crypto/ssh: panic: runtime error: slice bounds out of range x/crypto/ssh: DSA code sometimes generates 32 byte signatures, leading to crash. Mar 29, 2017
@felixbuenemann felixbuenemann mentioned this issue Jun 17, 2017
Closed
@gopherbot
Copy link

Change https://golang.org/cl/62870 mentions this issue: ssh: reject unsupported DSA key sizes

@hanwen hanwen mentioned this issue Feb 9, 2018
Open
@golang golang locked and limited conversation to collaborators Sep 12, 2018
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
@hanwen
ssh: reject unsupported DSA key sizes
1690449 
Fixes golang/go#19424.

Change-Id: I73370603dd612979420d608b73d67e673a52362b
Reviewed-on: https://go-review.googlesource.com/62870
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
@hanwen
ssh: reject unsupported DSA key sizes
061aba3 
Fixes golang/go#19424.

Change-Id: I73370603dd612979420d608b73d67e673a52362b
Reviewed-on: https://go-review.googlesource.com/62870
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@hanwen
ssh: reject unsupported DSA key sizes
6bf902e 
Fixes golang/go#19424.

Change-Id: I73370603dd612979420d608b73d67e673a52362b
Reviewed-on: https://go-review.googlesource.com/62870
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@hanwen
ssh: reject unsupported DSA key sizes
1cc763b 
Fixes golang/go#19424.

Change-Id: I73370603dd612979420d608b73d67e673a52362b
Reviewed-on: https://go-review.googlesource.com/62870
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
@rsc rsc unassigned hanwen Jun 23, 2022
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@hanwen
ssh: reject unsupported DSA key sizes
38ec4cb 
Fixes golang/go#19424.

Change-Id: I73370603dd612979420d608b73d67e673a52362b
Reviewed-on: https://go-review.googlesource.com/62870
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@hanwen
ssh: reject unsupported DSA key sizes
85a0789 
Fixes golang/go#19424.

Change-Id: I73370603dd612979420d608b73d67e673a52362b
Reviewed-on: https://go-review.googlesource.com/62870
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <hi@filippo.io>
Reviewed-by: Adam Langley <agl@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
4 participants
@bradfitz @hanwen @gopherbot @vdice