x/crypto/ssh: race in ListenUnix() causes forwarded socket to be rejected after client requests it

[spikecurtis]

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.

[bcmills]

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

[bcmills]

(attn @drakkan; CC @FiloSottile @rolandshoemaker per https://dev.golang.org/owners)

[drakkan]

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

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

[drakkan]

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]

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.