x/crypto/ssh: TestClientAuthMaxAuthTriesPublicKey failures with WSAECONNABORTED

[bcmills]

--- FAIL: TestClientAuthMaxAuthTriesPublicKey (0.01s)
    client_auth_test.go:642: client: got ssh: handshake failed: read tcp 127.0.0.1:50098->127.0.0.1:50099: wsarecv: An established connection was aborted by the software in your host machine., want ssh: handshake failed: ssh: disconnect, reason 2: too many authentication failures
FAIL
FAIL	golang.org/x/crypto/ssh	3.622s

greplogs --dashboard -md -l -e 'FAIL: TestClientAuthMaxAuthTriesPublicKey.*\n(?: .*\n)* .*An established connection was aborted' --since=2021-01-01

2022-01-25T00:39:08-5e0467b-16d6a52/windows-arm64-10
2022-01-11T17:13:50-e495a2d-1abe9c1/windows-arm64-10
2022-01-10T20:53:01-e495a2d-55d10ac/windows-arm64-10
2021-12-04T01:07:28-5770296-fa88ba1/windows-amd64-longtest
2021-12-01T10:19:34-ae814b3-0e1d553/windows-arm64-10
2021-11-29T00:58:50-ae814b3-1ea4d3b/windows-arm64-10
2021-11-20T00:37:28-ae814b3-d2f4c93/windows-amd64-longtest
2021-10-26T17:10:45-089bfa5-76cef81/windows-arm64-10
2021-09-17T23:21:29-c084706-f01721e/windows-arm64-10
2021-09-17T22:19:21-c084706-3fa35b5/windows-amd64-2016
2021-07-07T22:29:01-5ff15b2-5c59e11/windows-amd64-longtest
2021-03-17T00:18:28-e6e6c4f-63b0a0a/windows-amd64-longtest
2021-03-13T18:09:48-5ea612d-4bd4dfe/windows-amd64-longtest

(CC @golang/security)

[bcmills]

Looks like the expected error comes from the server side, here:
https://cs.opensource.google/go/x/crypto/+/master:ssh/server.go;l=409-413;drc=b4de73f9ece8163b492578e101e4ef8923ac2c5c

The actual error message looks like it is coming from the client side, from the connection being dropped without receiving the response from the server.

Possibly related: #33891, #28852.

[bcmills]

We could mask this on the builders by adding a test skip, but note that it affects windows/amd64, which is a first-class port.

Marking as release-blocker (per #11811) pending a decision from the security team as to whether to skip the test on GOOS=windows or investigate the underlying failure mode.

[bcmills]

#50161 may be closely related, in that the client is observing connection reset by peer in place of an expected error from the server.

I think the connection reset by peer error reported there (on macOS) is analogous to WSAECONNABORTED on Windows.

[dmitshur]

CC @golang/security Can you please take a look at whether this should be a test skip or if additional investigation is need?

[ianlancetaylor]

This error appears to mean that the client is writing to a socket that the server has already closed, and then trying to read the server's response. It seems that on Windows in this scenario the write to a socket that the server has closed can put the socket into an error condition that is reported during the read.

This is the normal sequence of packets in this test:

• client: send userAuthRequestMsg
• server: receive userAuthRequestMsg, send userAuthFailureMsg
• client: receive userAuthFailureMsg, send userAuthRequestMsg
• repeat this sequence six times in total
• server: receive userAuthRequestMsg, send userAuthFailureMsg, send disconnectMsg, close socket

As far as I can tell, there is nothing that prevents the client from seeing the final userAuthFailureMsg and the sending another userAuthRequestMsg. This normally doesn't happen because the client sees the disconnectMsg. But I don't see anything that forces that to happen first. The packets are being read in a loop in a goroutine and sent on a channel. Once that goroutine reads a disconnect packet it will mark an error and close the channel. But if the client gets in first and writes the next userAuthRequestMsg, then I think we can see this error when the goroutine tries to read the disconnect packet.

[gopherbot]

Change https://golang.org/cl/381614 mentions this issue: ssh: accept WSAECONNABORTED in TestClientAuthMaxAuthTriesPublicKey

[bcmills]

server: receive userAuthRequestMsg, send userAuthFailureMsg, send disconnectMsg, close socket

If I understand correctly, the race you're describing is between the client responding to the final userAuthFailureMessage and reading the final disconnectMsg. But the userAuthFailureMessage includes an explicit list of “authentications that continue” (https://datatracker.ietf.org/doc/html/rfc4252#section-5.1) — why is that list non-empty if the server does not intend to continue authentication? (Or is the client proceeding to retry an authentication that the server has not indicated?)

[ianlancetaylor]

The server code does not consider whether it has reached the maximum number of permitted authentication requests when it assembles its list of permitted authentication methods.

I could certainly change the server code so that it returns no permitted methods when it has exceeded the number of authentication failures. All the tests pass when I do so.

I sent that CL too. @rolandshoemaker can choose (or do something else entirely).

Thanks.

[gopherbot]

Change https://golang.org/cl/381615 mentions this issue: ssh: if too many auth failures, don't permit any more methods

[rolandshoemaker]

I'm not sure it's particularly necessary to send the SSH_MSG_USERAUTH_FAILURE packet if we are going to immediately send the SSH_MSG_DISCONNECT packet, although the spec is somewhat confusing on this (my reading is that it suggests it is necessary to always send the failure message regardless of what you're going to do next). OpenSSH seems to just immediately send the SSH_MSG_DISCONNECT packet and terminate, which is generally the source of truth for SSH.

I'm not really strongly opinionated on this, I think I err of the side of just matching OpenSSH behavior if we are going to change anything here (rather than just sending an empty methods list). In that case I think we'd just want to add a if authFailures >= config.MaxAuthTries { continue } after authFailures is incremented.

[rolandshoemaker]

(Also +2'd CL381614, but I think equally just immediately sending the disconnect packet would also solve this race.)