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

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

Comments

@vdice
Copy link

@vdice 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 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 vdice commented Mar 7, 2017

@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 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 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 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)

@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
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 11, 2017

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

@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
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
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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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