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

[vdice]

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.

[hanwen]

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]

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

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]

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]

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)

[gopherbot]

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