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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone

Comments

@KarthikNath
Copy link

@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
@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages) labels Mar 2, 2020
@breml
Copy link
Contributor

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

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

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

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

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

@golang golang locked and limited conversation to collaborators Apr 23, 2021
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
Fixes golang/go#37607

Change-Id: Iedf6522ec9b9a676ac51c054407a6aef894885f5
GitHub-Last-Rev: 8cb2460c59d2e32bc3f0480bcd7867a113361c67
GitHub-Pull-Request: golang/crypto#126
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222078
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Fixes golang/go#37607

Change-Id: Iedf6522ec9b9a676ac51c054407a6aef894885f5
GitHub-Last-Rev: 8cb2460c59d2e32bc3f0480bcd7867a113361c67
GitHub-Pull-Request: golang/crypto#126
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222078
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
Fixes golang/go#37607

Change-Id: Iedf6522ec9b9a676ac51c054407a6aef894885f5
GitHub-Last-Rev: 8cb2460c59d2e32bc3f0480bcd7867a113361c67
GitHub-Pull-Request: golang/crypto#126
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222078
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
Fixes golang/go#37607

Change-Id: Iedf6522ec9b9a676ac51c054407a6aef894885f5
GitHub-Last-Rev: 8cb2460c59d2e32bc3f0480bcd7867a113361c67
GitHub-Pull-Request: golang/crypto#126
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222078
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
Fixes golang/go#37607

Change-Id: Iedf6522ec9b9a676ac51c054407a6aef894885f5
GitHub-Last-Rev: 8cb2460
GitHub-Pull-Request: golang#126
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222078
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants