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

Jump to bottom

x/crypto/ssh: SSH handshake fails over some net.Conn implementations (ex. net.Pipe) #32990

Open
tarndt opened this issue Jul 8, 2019 · 8 comments
Open

x/crypto/ssh: SSH handshake fails over some net.Conn implementations (ex. net.Pipe) #32990

tarndt opened this issue Jul 8, 2019 · 8 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@tarndt
Copy link

tarndt commented Jul 8, 2019

What version of Go are you using (go version)?

Tested on:

go version go1.12 linux/amd64
go version go1.12.4 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

linux/windows amd64

What did you do?

Run the following unit test:
$ go test -timeout 30s sshproxy -run TestHandshakeOverPipe

package sshproxy

import (
	"log"
	"net"
	"testing"

	"github.com/pkg/errors"
	"golang.org/x/crypto/ssh"
)

const (
	testUserName = "test-user"
	testUserPass = "test-password"
)

var testServerCfg = &ssh.ServerConfig{
	PasswordCallback: func(c ssh.ConnMetadata, pass []byte) (*ssh.Permissions, error) {
		if c.User() == testUserName && string(pass) == testUserPass {
			return nil, nil
		}
		return nil, errors.Errorf("User %q supplied an incorrect password", c.User())
	},
}

var testClientCfg = &ssh.ClientConfig{
	User: testUserName,
	Auth: []ssh.AuthMethod{
		ssh.Password(testUserPass),
	},
	HostKeyCallback: func(hostname string, remote net.Addr, key ssh.PublicKey) error {
		return nil
	},
}

func init() {
	privateKey, err := ssh.ParsePrivateKey([]byte(testPrivateKey))
	if err != nil {
		log.Fatalf("Unit test init: Failed to parse private key; Details: %s", err)
	}
	testServerCfg.AddHostKey(privateKey)
}

func TestHandshakeOverPipe(t *testing.T) {
	srvConn, cliConn := net.Pipe()
	defer srvConn.Close()
	defer cliConn.Close()	

	errCh := make(chan error, 1)
	go func() {
		_, _, _, err := ssh.NewServerConn(srvConn, testServerCfg)
		errCh <- err
	}()

	if _, _, _, err := ssh.NewClientConn(cliConn, "localhost", testClientCfg); err != nil {
		t.Fatalf("Client SSH handshake failed; Details: %s", err)
	} else if err = <-errCh; err != nil {
		t.Fatalf("Server SSH handshake failed; Details: %s", err)
	}
}

//redacted
//const testPrivateKey = ...

What did you expect to see?

Test pass (test passes when modified to use a real TCP connection)

What did you see instead?

Tests hang with both client and sever writing to write to the net.Conn during ssh.exchangeVersions called from clientHandshake and serverHandshake respectively.

$ go test -timeout 30s sshproxy -run TestHandshakeOverPipe
panic: test timed out after 30s

goroutine 18 [running]:
testing.(*M).startAlarm.func1()
        /usr/local/go/src/testing/testing.go:1334 +0xdf
created by time.goFunc
        /usr/local/go/src/time/sleep.go:169 +0x44

goroutine 1 [chan receive]:
testing.(*T).Run(0xc0000ee100, 0x659a66, 0x15, 0x663b30, 0x476aa6)
        /usr/local/go/src/testing/testing.go:917 +0x37e
testing.runTests.func1(0xc0000ee000)
        /usr/local/go/src/testing/testing.go:1157 +0x78
testing.tRunner(0xc0000ee000, 0xc0000a7e30)
        /usr/local/go/src/testing/testing.go:865 +0xc0
testing.runTests(0xc00000ed00, 0x80c9a0, 0x2, 0x2, 0x0)
        /usr/local/go/src/testing/testing.go:1155 +0x2a9
testing.(*M).Run(0xc0000d8000, 0x0)
        /usr/local/go/src/testing/testing.go:1072 +0x162
main.main()
        _testmain.go:44 +0x13e

goroutine 6 [select]:
net.(*pipe).write(0xc0000d8100, 0xc0000183a0, 0xc, 0x20, 0x0, 0x0, 0x0)
        /usr/local/go/src/net/pipe.go:199 +0x27a
net.(*pipe).Write(0xc0000d8100, 0xc0000183a0, 0xc, 0x20, 0xc, 0xc0000183a0, 0xa)
        /usr/local/go/src/net/pipe.go:179 +0x4d
golang.org/x/crypto/ssh.exchangeVersions(0x7f95454b5060, 0xc0000d8100, 0xc000016520, 0xa, 0xa, 0x8, 0x4, 0x4, 0xa, 0xc000042660)
        go/pkg/mod/golang.org/x/crypto@v0.0.0-20190308221718-c2843e01d9a2/ssh/transport.go:297 +0xef
golang.org/x/crypto/ssh.(*connection).clientHandshake(0xc0000d8200, 0x656b21, 0x9, 0xc00008eb60, 0xc000082a01, 0xc00006a360)
        go/pkg/mod/golang.org/x/crypto@v0.0.0-20190308221718-c2843e01d9a2/ssh/client.go:100 +0xe7
golang.org/x/crypto/ssh.NewClientConn(0x69e200, 0xc0000d8100, 0x656b21, 0x9, 0x810da0, 0xc0000128c0, 0x24, 0x31d, 0x4cd7d0, 0x1, ...)
        go/pkg/mod/golang.org/x/crypto@v0.0.0-20190308221718-c2843e01d9a2/ssh/client.go:83 +0xfe
sshproxy.TestHandshakeOverPipe(0xc0000ee100)
        sshproxy/tunnel_test.go:60 +0xf3
testing.tRunner(0xc0000ee100, 0x663b30)
        /usr/local/go/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:916 +0x357

goroutine 7 [select]:
net.(*pipe).write(0xc0000d8080, 0xc0000183c0, 0xc, 0x20, 0x0, 0x0, 0x0)
        /usr/local/go/src/net/pipe.go:199 +0x27a
net.(*pipe).Write(0xc0000d8080, 0xc0000183c0, 0xc, 0x20, 0xc, 0xc0000183c0, 0xa)
        /usr/local/go/src/net/pipe.go:179 +0x4d
golang.org/x/crypto/ssh.exchangeVersions(0x7f95454b5060, 0xc0000d8080, 0xc000016530, 0xa, 0xa, 0xc000042ef8, 0x78, 0x78, 0xc0000d8300, 0x4)
        go/pkg/mod/golang.org/x/crypto@v0.0.0-20190308221718-c2843e01d9a2/ssh/transport.go:297 +0xef
golang.org/x/crypto/ssh.(*connection).serverHandshake(0xc0000d8300, 0xc0000f2000, 0x0, 0x0, 0x0)
        go/pkg/mod/golang.org/x/crypto@v0.0.0-20190308221718-c2843e01d9a2/ssh/server.go:217 +0x136
golang.org/x/crypto/ssh.NewServerConn(0x69e200, 0xc0000d8080, 0x8109e0, 0x0, 0x0, 0x0, 0x0, 0x0)
        go/pkg/mod/golang.org/x/crypto@v0.0.0-20190308221718-c2843e01d9a2/ssh/server.go:182 +0xd6
sshproxy.TestHandshakeOverPipe.func1(0x69e200, 0xc0000d8080, 0xc00006a360)
        sshproxy/tunnel_test.go:56 +0x49
created by sshproxy.TestHandshakeOverPipe
        sshproxy/tunnel_test.go:55 +0xaf
FAIL    sshproxy  30.012s
@gopherbot gopherbot added this to the Unreleased milestone Jul 8, 2019
@bcmills bcmills added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 9, 2019
@bcmills
Copy link
Contributor

bcmills commented Jul 9, 2019

CC @hanwen

@hanwen
Copy link
Contributor

hanwen commented Jul 9, 2019

yes, don't do this.

This is the reason why the SSH package has a netPipe function, see

https://go.googlesource.com/crypto/+/4def268fd1a49955bfb3dda92fe3db4f924f2285/ssh/handshake_test.go#39

if this a defect, then it is a defect in net.Pipe.

There is no way to make a SSH connection that doesn't start with both sides writing simultaneously.

@tarndt
Copy link
Author

tarndt commented Jul 9, 2019

Thanks for the quick response.

if this a defect, then it is a defect in net.Pipe.
There is no way to make a SSH connection that doesn't start with both sides writing simultaneously.

I totally get where you are coming from with this, but let me play devils advocate for a moment. If you can have a correct implementation of a net.Conn that fails here, does that mean the issue is actually with the package for accepting a net.Conn when a non-implementation defined detail can cause issues? To me that means net.Conn is not a sufficient contract to describe the behavior you desire of the c net.Conn parameter provided to the functions in question.

One way around this would be to require a TCPConn instead perhaps? At the vary least I think this caveat needs to be added to the comments for any exported functions or method that accept a net.Conn.

This is the reason why the SSH package has a netPipe function...

While its simple enough to re-implement, I think you should consider exporting ssh.netPipe so that folks writing unit tests for code built on this package can reuse that function.

Thanks!

@hanwen
Copy link
Contributor

hanwen commented Jul 9, 2019

TCPConn is a concrete type, and I think it is better if the SSH accepts an interface. However, interfaces are just a collection of methods, and behaviors such as the one we're interested in here can't be encoded in an interface.

I think it would be useful to have a in-memory connection that allows writes from both sides, but I'll leave that up to the core Go team to decide where that should be.

In any case, I think it is wrong for the SSH package to provide this as a utility.

@bcmills
Copy link
Contributor

bcmills commented Jul 10, 2019

#24205 seems closely related.

@bcmills
Copy link
Contributor

bcmills commented Jul 10, 2019

@hanwen, I would argue that this really is a defect in the ssh library. The fact that there is no Read call in that goroutine dump is telling: if both sides of the connection need to perform initial writes, then they both need to start reading at that point too. Otherwise, the implementation is unduly sensitive to kernel buffer sizes.

One very simple fix would be to execute the Write and readVersion calls in exchangeVersions concurrently.

Compare:
https://play.golang.org/p/YzdjL4qtrrZ
https://play.golang.org/p/p27dDyLoMgu

@hanwen
Copy link
Contributor

hanwen commented Jul 10, 2019

@bcmills - good idea. I'll look into that.

@bcmills
Copy link
Contributor

bcmills commented Jul 10, 2019

I think you'll need read-write concurrency in a few other places too (particularly clientAuthenticate and serverAuthenticate).

@bcmills bcmills mentioned this issue Oct 16, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@hanwen @tarndt @bcmills @gopherbot