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: Fix Protocol Version Exchange (for multi-line) #23194

Closed
jgracenin opened this issue Dec 20, 2017 · 14 comments
Closed

x/crypto/ssh: Fix Protocol Version Exchange (for multi-line) #23194

jgracenin opened this issue Dec 20, 2017 · 14 comments

Comments

@jgracenin
Copy link

@jgracenin jgracenin commented Dec 20, 2017

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

1.9.2

Does this issue reproduce with the latest release?

yes

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

linux amd64

What did you do?

Dial to an SSH device that adds an additional line before the SSH version string.

What did you expect to see?

A successful connection.

What did you see instead?

An error (I added in the packet length for debugging purposes):
ssh: handshake failed: ssh: invalid packet length, packet too large: 1397966893


As it turns out, I have several network devices that do this:
some_other_line
SSH-2.0-OpenSSH

After some painful debugging:

  • The Go ssh library thought "some_other_line" was the version string.
  • Then, by definition of the SSH RFC, bytes following that must be an key exchange protocol packet. This produced some sporadic behavior in readPacket since it was using the version string as key exchange.
  • s.prefix byte array was: [1010011 1010011 1001000 101101 110010] (or SSH-2 in ASCII).
  • I printed out s.prefix right above this line: https://github.com/golang/crypto/blob/master/ssh/cipher.go#L162

I don't like it, but OpenSSH clients work fine and I found out that these other lines are actually allowed as part of the RFC (provided the whole exchange doesn't exceed 255 bytes):
https://tools.ietf.org/html/rfc4253#section-4.2

The important point from there:
"The server MAY send other lines of data before sending the version string. Each line SHOULD be terminated by a Carriage Return and Line Feed. Such lines MUST NOT begin with "SSH-".

The patch is pretty easy. In transport,go, I modified this block (https://github.com/golang/crypto/blob/master/ssh/transport.go#L352-L355) to below, which solved my problem (works for devices that have the extra string and devices that don't).

if buf[0] == '\n' {
     if !strings.HasPrefix(string(versionString), "SSH-") {
          // Ignore all lines except the line containing the SSH version string.
          versionString = make([]byte, 0, 64)
          continue
     }
     ok = true
     break
}

The above code is still bounds protected by maxVersionStringBytes.

I can send out a Gerrit CL with a test if you want (my company has the CLA in place already). I'd prefer not to maintain a custom fork.

Thanks.

@gopherbot gopherbot added this to the Unreleased milestone Dec 20, 2017
@jgracenin
Copy link
Author

@jgracenin jgracenin commented Dec 20, 2017

Oh, one more thing. To maintain the maxVersionStringBytes bounds across resetting the versionString slice, I changed the loop containing that block to:

for length := 0; length < maxVersionStringBytes; length++ {
  ...
}

Each loop iteration reads 1 byte at a time.

@ALTree
Copy link
Member

@ALTree ALTree commented Dec 20, 2017

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 20, 2017

Thanks for the detailed report. I think we'd be happy to take the CL since it's RFC behavior. Make sure it doesn't let the server send new lines forever keeping the client busy.

/cc @hanwen

@jgracenin
Copy link
Author

@jgracenin jgracenin commented Dec 20, 2017

Yeah, the new loop condition should cover that. I'll make sure my unit test has a case for this though.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Dec 20, 2017

I vaguely remember reports of SSH version strings that did not start with SSH- , which would be hard to combine with reading up to the first SSH- line.

Let me try to see if I can find them.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Dec 20, 2017

see here https://codereview.appspot.com/14641044/

I don't want to add this feature. You can work around a server that sends garbage before the version string by wrapping net.Conn to eat the first few bytes.

(conversely, you cannot work around a broken SSH version string, since the version string is hashed into the key exchange)

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 20, 2017

Hmm, what does OpenSSH do? Given how popular OpenSSH is I imagine there are very very few servers that don't interoperate with it, so if we match it in sticking to the RFC we shouldn't have a problem.

@jgracenin
Copy link
Author

@jgracenin jgracenin commented Dec 20, 2017

Yeah, I don't really like it either. However, it isn't not garbage, it is part of the RFC. As it stands, Go is not conforming to it.

Unfortunately, wrapping net.Conn won't work that well since these are variable length / content strings. I'd have to keep a list of servers that have this behavior and only call the wrapper in that case to chomp off that data.

Otherwise, for servers that don't send this, I can't read anything or the connection won't work (since I'd already would have read SSH-2 off of the buffer). Yes, in that case I could close my current conn, reconnect, and then hand that new conn off, but that's pretty ugly :)

OpenSSH clients handle this transparently with a similar prefix check:
https://github.com/openssh/openssh-portable/blob/e0ce54c0b9ca3a9388f9c50f4fa6cc25c28a3240/sshconnect.c#L580-L581

Re: can't work around a broken SSH version string:
Agreed. I opened up this issue because I'm running into that now :). I tested my fix on servers that respond with SSH-2 and ones that don't. It seems like a simple fix. Is there another concern I'm missing here?

@hanwen
Copy link
Contributor

@hanwen hanwen commented Dec 20, 2017

ah right: " The server MAY send other lines of data before sending the version
string. Each line SHOULD be terminated by a Carriage Return and Line
Feed. Such lines MUST NOT begin with "SSH-", and SHOULD be encoded
in ISO-10646 UTF-8 [RFC3629] (language is not specified). Clients
MUST be able to process such lines. Such lines MAY be silently
ignored, or MAY be displayed to the client user. If they are
displayed, control character filtering, as discussed in [SSH-ARCH],
SHOULD be used. The primary use of this feature is to allow TCP-
wrappers to display an error message before disconnecting."

How about a config option:

FirstLineVersionString bool

which if set, forces the first line to be interpreted as version string (current behavior). If not, we follow RFC and gobble up to the first SSH-x-y line.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 20, 2017

Options like this are gross API bloat.

I'd rather do whatever OpenSSH does.

@taralx
Copy link

@taralx taralx commented Dec 20, 2017

+1 -- if this works in OpenSSH, we should have it just work here. The (hopefully) small number of people with non-compliant devices that send version strings that don't start with SSH- are, well, non-compliant. Cross that bridge when we get to it.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Dec 20, 2017

ok. Let's do it then. You can send a change; I may not be able to look at it until Jan. 7 or so, though.

@jgracenin
Copy link
Author

@jgracenin jgracenin commented Dec 20, 2017

The other issue is, readVersion is a function (not a method), so we'd have to change the signature to pipe that option through unfortunately.

Yeah, a non-compliant string will break after this change (but I suppose we could point to the RFC at that point). I tested on a sample size of ~1000 hosts so far running many different implementations of ssh (some returning an extra line, most not) and they look good.

I'm out next week, so I'm not sure if I'll get around to sending a CL until the first week of January either. Thanks everyone.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 8, 2018

Change https://golang.org/cl/86675 mentions this issue: x/crypto/ssh: Fix Protocol Version Exchange (for multi-line)

@golang golang locked and limited conversation to collaborators Jan 11, 2019
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
Fixes golang/go#23194

During SSH Protocol Version Exchange, a client may send metadata lines
prior to sending the SSH version string. To conform to the RFC, all SSH
implementations must support this (minimally, clients can ignore the
metadata lines).

For example, this is valid:
some-metadata
SSH-2.0-OpenSSH

The current Go implementation takes the first line it sees as
the version string (in this case, some-metadata). Then, it uses
the next line (SSH-2.0-OpenSSH) as part of key exchange, which
is guaranteed to fail.

Unfortunately, this SSH feature is used by some vendors and is part
of the official RFC: https://tools.ietf.org/html/rfc4253#section-4.2

Change-Id: I7be61700a07756353875bf43aad09a580ba533ff
Reviewed-on: https://go-review.googlesource.com/86675
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
Fixes golang/go#23194

During SSH Protocol Version Exchange, a client may send metadata lines
prior to sending the SSH version string. To conform to the RFC, all SSH
implementations must support this (minimally, clients can ignore the
metadata lines).

For example, this is valid:
some-metadata
SSH-2.0-OpenSSH

The current Go implementation takes the first line it sees as
the version string (in this case, some-metadata). Then, it uses
the next line (SSH-2.0-OpenSSH) as part of key exchange, which
is guaranteed to fail.

Unfortunately, this SSH feature is used by some vendors and is part
of the official RFC: https://tools.ietf.org/html/rfc4253#section-4.2

Change-Id: I7be61700a07756353875bf43aad09a580ba533ff
Reviewed-on: https://go-review.googlesource.com/86675
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
bored-engineer pushed a commit to bored-engineer/ssh that referenced this issue Oct 13, 2019
Fixes golang/go#23194

During SSH Protocol Version Exchange, a client may send metadata lines
prior to sending the SSH version string. To conform to the RFC, all SSH
implementations must support this (minimally, clients can ignore the
metadata lines).

For example, this is valid:
some-metadata
SSH-2.0-OpenSSH

The current Go implementation takes the first line it sees as
the version string (in this case, some-metadata). Then, it uses
the next line (SSH-2.0-OpenSSH) as part of key exchange, which
is guaranteed to fail.

Unfortunately, this SSH feature is used by some vendors and is part
of the official RFC: https://tools.ietf.org/html/rfc4253#section-4.2

Change-Id: I7be61700a07756353875bf43aad09a580ba533ff
Reviewed-on: https://go-review.googlesource.com/86675
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
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
7 participants
You can’t perform that action at this time.