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: race in ListenUnix() causes forwarded socket to be rejected after client requests it #64094

Open
spikecurtis opened this issue Nov 13, 2023 · 6 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Milestone

Comments

@spikecurtis
Copy link

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

Not applicable. Affects x/crypto version v0.15.0

Does this issue reproduce with the latest release?

Yes

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

Not applicable

What did you do?

In ListenUnix():

https://github.com/golang/crypto/blob/1cf1811d7195fe9bb436a00e335567575fac9b07/ssh/streamlocal.go#L40

it sends the message to request remote forwarding of Unix domain sockets before it is prepared to receive forwards, which it sets later: https://github.com/golang/crypto/blob/1cf1811d7195fe9bb436a00e335567575fac9b07/ssh/streamlocal.go#L47

This sets up a race between the server forwarding a connection and the client being prepared to receive it. The client should set itself to receive the forward before sending the request (and if the request fails, clean up) to avoid a race condition. I see this in unit testing where a test SSH server can very quickly respond to a request to forward sockets, but in principal it could happen in a live system too.

A similar issue affects TCP sockets.

What did you expect to see?

  1. the SSH server accepts the message to begin forwarding Unix sockets
  2. the SSH server opens a channel back to the client to forward the socket
  3. the client accepts the channel

What did you see instead?

  1. the SSH server accepts the message to begin forwarding Unix sockets
  2. the SSH server opens a channel to forward the socket back to the client
  3. occasionally, the SSH client rejects the channel with "administratively prohibited (no forward for address)", even though it is the address the client just requested.
@gopherbot gopherbot added this to the Unreleased milestone Nov 13, 2023
@bcmills bcmills added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 13, 2023
@bcmills
Copy link
Contributor

bcmills commented Nov 13, 2023

@spikecurtis, want to send a fix? (And is it feasible to write a regression test that triggers this race at some nonzero rate?)

@bcmills
Copy link
Contributor

bcmills commented Nov 13, 2023

@seankhliao seankhliao changed the title x/crypto/ssh race in ListenUnix() causes forwarded socket to be rejected after client requests it x/crypto/ssh: race in ListenUnix() causes forwarded socket to be rejected after client requests it Nov 15, 2023
@drakkan
Copy link
Member

drakkan commented Dec 29, 2023

@spikecurtis thanks for this report.
If you could provide a test case and/or standalone player it would be very helpful.
I have a local WIP and would like to confirm that it solves the problem for you too before finalizing it. Thank you!

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/553315 mentions this issue: ssh: prevent race conditions in remote forwarding

@drakkan
Copy link
Member

drakkan commented Dec 31, 2023

I can reproduce the problem locally using a test application.

The above CL has no test cases (there is no test case for remote forwarding in general) and so probably can't be merged as is but it solves the problem for me locally for both sockets and TCP connections, @spikecurtis a confirmation that it works in your case too would be much appreciated. Thank you

@spikecurtis
Copy link
Author

Unix changes look good, but in the TCP case it only closes the race when port > 0, which is admittedly all we use for our current use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Security
Projects
None yet
Development

No branches or pull requests

4 participants