x/crypto/ssh: data race in diffie-hellman-group-exchange-sha256

[KarthikNath]

@FiloSottile @breml Opening issue in reference to #17230 (comment)

Running into failures on concurrent ssh with diffiehelman group exchange. Random hosts are failing always. Seems more like its running into race condition with diffiehelman addition. Works perfectly with default kex's. Below is concurrency code snippet

              
                config := &ssh.ClientConfig{
                        User: “XXXXX”,
                        Auth: []ssh.AuthMethod{
                                PublicKeyFile(privatekey),
                        },
                        Config: ssh.Config{
                                       KeyExchanges: []string{
                                                "[email protected]",
                                                "ecdh-sha2-nistp256",
                                                "ecdh-sha2-nistp384",
                                                "ecdh-sha2-nistp521",
                                                "diffie-hellman-group14-sha1",
                                                "diffie-hellman-group-exchange-sha256", // needed for rhel6
                        },
                },
                        HostKeyCallback: ssh.InsecureIgnoreHostKey(),
                        Timeout:         time.Second * 20,
                }

                if err := addKeyToAgent(); err != nil {
                        return nil, nil, fmt.Errorf("error adding key type %s", err)
                }

              for _, hostname := range hosts {
                 wg.Add(1)
                 go func(hostname string) {
                        defer wg.Done()
                        result, err := executeCmd(config, cmd, hostname)
                        if err != nil {
                                logger.Printf("error running cmd on host  %s: %s", hostname, err)
                                if strings.Contains(err.Error(), "Process") {
                                        hostname = hostname + "_command"
                                } else {
                                        hostname = hostname + "_ssh"
                                }
                                res.fail = hostname
                                Results <- *res
			        return
                        }
                        logger2.Printf("%s : %s", hostname, result)
                        res.succ = result
                        Results <- *res
                         return
                }(hostname)
        }

        wg.Wait()

Below is the race condition errors i see.

WARNING: DATA RACE
Write at 0x00c00021ace8 by goroutine 85:
golang.org/x/crypto/ssh.(*dhGEXSHA).Client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/kex.go:602 +0x43d
golang.org/x/crypto/ssh.(*handshakeTransport).client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:627 +0x127
golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:587 +0xaa3
golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:301 +0x296

Previous write at 0x00c00021ace8 by goroutine 203:
golang.org/x/crypto/ssh.(*dhGEXSHA).Client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/kex.go:602 +0x43d
golang.org/x/crypto/ssh.(*handshakeTransport).client()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:627 +0x127
golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:587 +0xaa3
golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:301 +0x296

Goroutine 85 (running) created at:
golang.org/x/crypto/ssh.newClientTransport()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:135 +0x311
golang.org/x/crypto/ssh.(*connection).clientHandshake()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:105 +0x47c
golang.org/x/crypto/ssh.NewClientConn()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:83 +0x1d1
golang.org/x/crypto/ssh.Dial()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:177 +0xe6
xxxxxxx/force.Sshconnect()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:226 +0xca
xxxxxxx/force.executeCmd()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:183 +0x7d
xxxxxxx/force.Input.func1()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:380 +0x87

Goroutine 203 (running) created at:
golang.org/x/crypto/ssh.newClientTransport()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/handshake.go:135 +0x311
golang.org/x/crypto/ssh.(*connection).clientHandshake()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:105 +0x47c
golang.org/x/crypto/ssh.NewClientConn()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:83 +0x1d1
golang.org/x/crypto/ssh.Dial()
xxxxxxxxxxx/go/src/golang.org/x/crypto/ssh/client.go:177 +0xe6
xxxxxxx/force.Sshconnect()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:226 +0xca
xxxxxxx/force.executeCmd()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:183 +0x7d
xxxxxxx/force.Input.func1()
xxxxxxxxxxx/go/src/xxxxxxx/force/forcessh.go:380 +0x87

[breml]

A "simple" fix I could think of is to remove these two lines in diffie-hellman-group-exchange-sha256
Client

and replace the usage of gex.P with kexDHGexGroup.P and gex.G with kexDHGexGroup.G from there on. This would also include to change the call to gex.diffieHellman(kexDHGexReply.Y, x), which would then take kexDHGexGroup.P as an additional argument like this: gex.diffieHellman(kexDHGexGroup.P, kexDHGexInit.X, y) (we could also change diffieHellman from a method to a function).

For completeness sake we could adopt the changes mentioned above for the diffie-hellman-group-exchange-sha256 Server as well.

@FiloSottile, @hanwen: What do you think? Should I provide a PR with these changes or do you have a different solution in mind?

[breml]

I looked a little bit more into this issue and how to fix it. This is what I have found out so far:

• The problem is reproducible in tests.
• It looks like the tests for package golang.org/x/crypto/ssh are not executed with the race detector enabled by default, because on current master (golang/crypto@78000ba) the tests are failing for me with the race detector enabled in TestCertLogin.
• I implemented a fix for this issue (and the test mentioned above) in ssh: fix data race in dh group exchange sha256 crypto#126, which is based on the idea mentioned in the comment by @hanwen
• The test case provided in the above PR does trigger, if the change in Client of diffie-hellman-group-exchange-sha256 is reverted and the tests are executed with the race detector enabled.

[gopherbot]

Change https://golang.org/cl/222078 mentions this issue: ssh: fix data race in dh group exchange sha256

[KarthikNath]

any update on this?

@breml i tried your fix, still seeing handshake failures. A few connections go through initially, the remaining hangs and times out of handshake failure. Although, no data race is observed.

ssh: handshake failed: write tcp XX.XX.XXX.XXXX:39850->XX.XXX.XXX.XX:22: write: broken pipe

[breml]

@KarthikNath I am not sure what you mean. There is an open CL (https://go-review.googlesource.com/c/crypto/+/222078/), which waits to get merged and then released. There is currently nothing I can do about this.

Which fix did you try?

[KarthikNath]

I updated my library with your changes in the PR to see if its working. Im running into same issue as before. Wondering if thats not enough to get the library working?

[KarthikNath]

Just an update. Key exchange still doesn't work on a huge set of hosts ran concurrently. Although data race is no longer observed.