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: data race in diffie-hellman-group-exchange-sha256 #37607

Closed
KarthikNath opened this issue Mar 2, 2020 · 7 comments
Closed

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

KarthikNath opened this issue Mar 2, 2020 · 7 comments
Labels
Milestone

Comments

@KarthikNath
Copy link

@KarthikNath KarthikNath commented Mar 2, 2020

@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{
                                                "curve25519-sha256@libssh.org",
                                                "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
@FiloSottile FiloSottile changed the title Concurrency support for Diffie-Hellman Group Exchange x/crypto/ssh: data race in diffie-hellman-group-exchange-sha256 Mar 2, 2020
@gopherbot gopherbot added this to the Unreleased milestone Mar 2, 2020
@breml
Copy link
Contributor

@breml breml commented Mar 2, 2020

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
Copy link
Contributor

@breml breml commented Mar 4, 2020

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 golang/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
Copy link

@gopherbot gopherbot commented Mar 4, 2020

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

breml added a commit to breml/crypto that referenced this issue Mar 5, 2020
breml added a commit to breml/crypto that referenced this issue Mar 6, 2020
@KarthikNath
Copy link
Author

@KarthikNath KarthikNath commented Mar 9, 2020

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
Copy link
Contributor

@breml breml commented Mar 9, 2020

@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
Copy link
Author

@KarthikNath KarthikNath commented Mar 9, 2020

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
Copy link
Author

@KarthikNath KarthikNath commented Apr 23, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.