ssh: close net.Conn on all NewServerConn errors

This PR ensures that the net.Conn passed to ssh.NewServerConn is closed
on all error return paths, not just after a failed handshake. This matches
the behavior of ssh.NewClientConn.

Change-Id: Id8a51d10ae8d575cbbe26f2ef6b37de7cca840ec
GitHub-Last-Rev: 81bb2e5
GitHub-Pull-Request: #279
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/549095
Run-TryBot: Nicola Murino <[email protected]>
Auto-Submit: Nicola Murino <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-by: Nicola Murino <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Pratt <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

ssh/server.go

@@ -213,13 +213,15 @@ func NewServerConn(c net.Conn, config *ServerConfig) (*ServerConn, <-chan NewCha
  } else {
  for _, algo := range fullConf.PublicKeyAuthAlgorithms {
  if !contains(supportedPubKeyAuthAlgos, algo) {
+ c.Close()
  return nil, nil, nil, fmt.Errorf("ssh: unsupported public key authentication algorithm %s", algo)
  }
  }
  }
  // Check if the config contains any unsupported key exchanges
  for _, kex := range fullConf.KeyExchanges {
  if _, ok := serverForbiddenKexAlgos[kex]; ok {
+ c.Close()
  return nil, nil, nil, fmt.Errorf("ssh: unsupported key exchange %s for server", kex)
  }
  }

ssh/server_test.go

@@ -5,7 +5,11 @@
 package ssh
 
 import (
+ "io"
+ "net"
+ "sync/atomic"
  "testing"
+ "time"
 )
 
 func TestClientAuthRestrictedPublicKeyAlgos(t *testing.T) {
@@ -59,27 +63,78 @@ func TestClientAuthRestrictedPublicKeyAlgos(t *testing.T) {
 }
 
 func TestNewServerConnValidationErrors(t *testing.T) {
- c1, c2, err := netPipe()
- if err != nil {
- t.Fatalf("netPipe: %v", err)
- }
- defer c1.Close()
- defer c2.Close()
-
  serverConf := &ServerConfig{
  PublicKeyAuthAlgorithms: []string{CertAlgoRSAv01},
  }
- _, _, _, err = NewServerConn(c1, serverConf)
+ c := &markerConn{}
+ _, _, _, err := NewServerConn(c, serverConf)
  if err == nil {
  t.Fatal("NewServerConn with invalid public key auth algorithms succeeded")
  }
+ if !c.isClosed() {
+ t.Fatal("NewServerConn with invalid public key auth algorithms left connection open")
+ }
+ if c.isUsed() {
+ t.Fatal("NewServerConn with invalid public key auth algorithms used connection")
+ }
+
  serverConf = &ServerConfig{
  Config: Config{
  KeyExchanges: []string{kexAlgoDHGEXSHA256},
  },
  }
- _, _, _, err = NewServerConn(c1, serverConf)
+ c = &markerConn{}
+ _, _, _, err = NewServerConn(c, serverConf)
  if err == nil {
  t.Fatal("NewServerConn with unsupported key exchange succeeded")
  }
+ if !c.isClosed() {
+ t.Fatal("NewServerConn with unsupported key exchange left connection open")
+ }
+ if c.isUsed() {
+ t.Fatal("NewServerConn with unsupported key exchange used connection")
+ }
+}
+
+type markerConn struct {
+ closed uint32
+ used uint32
 }
+
+func (c *markerConn) isClosed() bool {
+ return atomic.LoadUint32(&c.closed) != 0
+}
+
+func (c *markerConn) isUsed() bool {
+ return atomic.LoadUint32(&c.used) != 0
+}
+
+func (c *markerConn) Close() error {
+ atomic.StoreUint32(&c.closed, 1)
+ return nil
+}
+
+func (c *markerConn) Read(b []byte) (n int, err error) {
+ atomic.StoreUint32(&c.used, 1)
+ if atomic.LoadUint32(&c.closed) != 0 {
+ return 0, net.ErrClosed
+ } else {
+ return 0, io.EOF
+ }
+}
+
+func (c *markerConn) Write(b []byte) (n int, err error) {
+ atomic.StoreUint32(&c.used, 1)
+ if atomic.LoadUint32(&c.closed) != 0 {
+ return 0, net.ErrClosed
+ } else {
+ return 0, io.ErrClosedPipe
+ }
+}
+
+func (*markerConn) LocalAddr() net.Addr { return nil }
+func (*markerConn) RemoteAddr() net.Addr { return nil }
+
+func (*markerConn) SetDeadline(t time.Time) error { return nil }
+func (*markerConn) SetReadDeadline(t time.Time) error { return nil }
+func (*markerConn) SetWriteDeadline(t time.Time) error { return nil }