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: ssh_message_ignore (message type 2) causing an error on Dial() #16927

Closed
darethas opened this issue Aug 30, 2016 · 8 comments
Closed
Milestone

Comments

@darethas
Copy link

@darethas darethas commented Aug 30, 2016

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

go1.7 darwin/amd64

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

darwin, amd64

What did you do?

attempted to connect to an ssh server via /x/crypto/ssh using ssh.Dial()

What did you expect to see?

A successful connection.

What did you see instead?

unexpected message type 2 (expected one of [31])

RFC4253 states "all implementations must understand and subsequently ignore this message at any time" but the package is treating it as an invalid message type, causing the transmission attempt to fail. (see https://tools.ietf.org/html/rfc4253#page-26)

@darethas darethas changed the title /x/crypto/ssh ssh_message_ignore (message type 2) causing an error on Dial() x/crypto/ssh ssh_message_ignore (message type 2) causing an error on Dial() Aug 30, 2016
@quentinmit quentinmit modified the milestone: Unreleased Sep 6, 2016
@quentinmit quentinmit changed the title x/crypto/ssh ssh_message_ignore (message type 2) causing an error on Dial() x/crypto/ssh: ssh_message_ignore (message type 2) causing an error on Dial() Sep 6, 2016
@belak
Copy link

@belak belak commented Dec 1, 2016

We're running into this server-side as well. The issue appears to be that in the handshake transport, it's not ignored properly, as during the handshake, there are multiple messages exchanged in a specific order. However, it looks like it's valid for msgIgnore to come between those.

At the end of enterKeyExchangeLocked we use t.conn.readPacket which can return msgIgnore. As an example, in (t *handshakeTransport) readLoop() msgIgnore is ignored as expected, but when we fall back to the underlying connection, we still get those messages.

@belak
Copy link

@belak belak commented Dec 1, 2016

@darethas
Copy link
Author

@darethas darethas commented Dec 1, 2016

There is one caveat in that the requirement to accept this message type is only levied after the identification string is sent which should happen after the connection is established. I don't think that is the case here since the message type it is expecting is related to key exchanging. Key exchange comes after the identification string has been sent.

@belak
Copy link

@belak belak commented Dec 1, 2016

I'm not as familiar with this part of the protocol (yet) but it seems like the identification string is always the first line sent and it's not a part of a packet. So, anywhere packets are sent and received, msgIgnore can show up.

@belak
Copy link

@belak belak commented Dec 1, 2016

I feel like it should be fine to ignore msgIgnore at the transport level... unless there's any pressing need, couldn't t.conn.readPacket just never return msgIgnore?

Or does that mean other stuff has to be refactored because we translate messages into msgIgnore to drop them in at least one instance.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Dec 5, 2016

you should be able to drop in the code that decrypts a packet.

@athom
Copy link

@athom athom commented Dec 9, 2016

I encounter the same issue when I use crypto/ssh for client side.
Thanks @belak and @treehau5, I made a hacking and solved my problem now.
My solution is doing a retry the t.conn.readPacket every time it return the msgIgnore.
I would like to contribute my code, but I don't think it is a good way to fix.

couldn't t.conn.readPacket just never return msgIgnore?

I feel it was a good suggestion, had voted +1.

you should be able to drop in the code that decrypts a packet

could you explain more?

@belak
Copy link

@belak belak commented Jan 23, 2017

It looks like this should have been closed a few days ago. golang/crypto@7c6cc32

@agl agl closed this Jan 23, 2017
@golang golang locked and limited conversation to collaborators Jan 23, 2018
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Nov 24, 2019
This prevents these messages from confusing higher layers of the
protocol.

Fixes golang#16927.

Change-Id: If18d8d02bdde3c0470e29a7280cd355d3e55ad78
Reviewed-on: https://go-review.googlesource.com/34959
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
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.