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

[tarndt]

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

[bcmills]

CC @hanwen

[hanwen]

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]

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]

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]

#24205 seems closely related.

[bcmills]

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

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

[bcmills]

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