From 1582bf0781a894a66c3168ac9e7be32995bc1e73 Mon Sep 17 00:00:00 2001 From: Dave Cheney Date: Tue, 30 Oct 2012 18:13:59 +1100 Subject: [PATCH] go.crypto/ssh: never negotiate unsupported ciphers Fixes golang/go#4285. Adding a new cipher that is supported by the remote end, but not supported by our client causes that cipher to be considered a valid candidate. This fails later in setupKeys when there is no cipherModes configuration. In summary, unsupported ciphers cannot be willed into existence by adding them to the client config. This change enforces this. R=golang-dev, agl CC=golang-dev https://golang.org/cl/6780047 --- ssh/cipher.go | 3 +++ ssh/client_auth_test.go | 20 +++++++++++++++ ssh/common.go | 21 ++++++++++++---- ssh/test/session_test.go | 12 ++++----- ssh/test/tcpip_test.go | 2 +- ssh/test/test_unix_test.go | 50 +++++++++++++++++++++++--------------- 6 files changed, 76 insertions(+), 32 deletions(-) diff --git a/ssh/cipher.go b/ssh/cipher.go index 0646e1e..97e5d9b 100644 --- a/ssh/cipher.go +++ b/ssh/cipher.go @@ -74,6 +74,9 @@ var DefaultCipherOrder = []string{ "arcfour256", "arcfour128", } +// cipherModes documents properties of supported ciphers. Ciphers not included +// are not supported and will not be negotiated, even if explicitly requested in +// ClientConfig.Crypto.Ciphers. var cipherModes = map[string]*cipherMode{ // Ciphers from RFC4344, which introduced many CTR-based ciphers. Algorithms // are defined in the order specified in the RFC. diff --git a/ssh/client_auth_test.go b/ssh/client_auth_test.go index 11c7227..d4002c6 100644 --- a/ssh/client_auth_test.go +++ b/ssh/client_auth_test.go @@ -276,3 +276,23 @@ func TestClientHMAC(t *testing.T) { c.Close() } } + +// issue 4285. +func TestClientUnsupportedCipher(t *testing.T) { + kc := new(keychain) + kc.keys = append(kc.keys, rsakey) + config := &ClientConfig{ + User: "testuser", + Auth: []ClientAuth{ + ClientAuthKeyring(kc), + }, + Crypto: CryptoConfig{ + Ciphers: []string{"aes128-cbc"}, // not currently supported + }, + } + c, err := Dial("tcp", newMockAuthServer(t), config) + if err == nil { + t.Errorf("expected no ciphers in common") + c.Close() + } +} diff --git a/ssh/common.go b/ssh/common.go index 9cb740d..f62b255 100644 --- a/ssh/common.go +++ b/ssh/common.go @@ -8,8 +8,8 @@ import ( "crypto/dsa" "crypto/rsa" "errors" + "fmt" "math/big" - "strconv" "sync" ) @@ -77,7 +77,7 @@ type UnexpectedMessageError struct { } func (u UnexpectedMessageError) Error() string { - return "ssh: unexpected message type " + strconv.Itoa(int(u.got)) + " (expected " + strconv.Itoa(int(u.expected)) + ")" + return fmt.Sprintf("ssh: unexpected message type %d (expected %d)", u.got, u.expected) } // ParseError results from a malformed SSH message. @@ -86,7 +86,7 @@ type ParseError struct { } func (p ParseError) Error() string { - return "ssh: parse error in message type " + strconv.Itoa(int(p.msgType)) + return fmt.Sprintf("ssh: parse error in message type %d", p.msgType) } type handshakeMagics struct { @@ -102,7 +102,18 @@ func findCommonAlgorithm(clientAlgos []string, serverAlgos []string) (commonAlgo } } } + return +} +func findCommonCipher(clientCiphers []string, serverCiphers []string) (commonCipher string, ok bool) { + for _, clientCipher := range clientCiphers { + for _, serverCipher := range serverCiphers { + // reject the cipher if we have no cipherModes definition + if clientCipher == serverCipher && cipherModes[clientCipher] != nil { + return clientCipher, true + } + } + } return } @@ -117,12 +128,12 @@ func findAgreedAlgorithms(transport *transport, clientKexInit, serverKexInit *ke return } - transport.writer.cipherAlgo, ok = findCommonAlgorithm(clientKexInit.CiphersClientServer, serverKexInit.CiphersClientServer) + transport.writer.cipherAlgo, ok = findCommonCipher(clientKexInit.CiphersClientServer, serverKexInit.CiphersClientServer) if !ok { return } - transport.reader.cipherAlgo, ok = findCommonAlgorithm(clientKexInit.CiphersServerClient, serverKexInit.CiphersServerClient) + transport.reader.cipherAlgo, ok = findCommonCipher(clientKexInit.CiphersServerClient, serverKexInit.CiphersServerClient) if !ok { return } diff --git a/ssh/test/session_test.go b/ssh/test/session_test.go index 91dea2a..532bb7b 100644 --- a/ssh/test/session_test.go +++ b/ssh/test/session_test.go @@ -20,7 +20,7 @@ import ( func TestRunCommandSuccess(t *testing.T) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() session, err := conn.NewSession() @@ -37,7 +37,7 @@ func TestRunCommandSuccess(t *testing.T) { func TestRunCommandFailed(t *testing.T) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() session, err := conn.NewSession() @@ -54,7 +54,7 @@ func TestRunCommandFailed(t *testing.T) { func TestRunCommandWeClosed(t *testing.T) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() session, err := conn.NewSession() @@ -74,7 +74,7 @@ func TestRunCommandWeClosed(t *testing.T) { func TestFuncLargeRead(t *testing.T) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() session, err := conn.NewSession() @@ -106,7 +106,7 @@ func TestFuncLargeRead(t *testing.T) { func TestInvalidTerminalMode(t *testing.T) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() session, err := conn.NewSession() @@ -123,7 +123,7 @@ func TestInvalidTerminalMode(t *testing.T) { func TestValidTerminalMode(t *testing.T) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() session, err := conn.NewSession() diff --git a/ssh/test/tcpip_test.go b/ssh/test/tcpip_test.go index 9ef4877..ee06b60 100644 --- a/ssh/test/tcpip_test.go +++ b/ssh/test/tcpip_test.go @@ -27,7 +27,7 @@ func TestTCPIPHTTPS(t *testing.T) { func doTest(t *testing.T, url string) { server := newServer(t) defer server.Shutdown() - conn := server.Dial() + conn := server.Dial(clientConfig()) defer conn.Close() tr := &http.Transport{ diff --git a/ssh/test/test_unix_test.go b/ssh/test/test_unix_test.go index 4409e84..9df54ba 100644 --- a/ssh/test/test_unix_test.go +++ b/ssh/test/test_unix_test.go @@ -9,6 +9,7 @@ package test // functional test harness for unix. import ( + "bytes" "crypto" "crypto/dsa" "crypto/rsa" @@ -40,7 +41,7 @@ Pidfile {{.Dir}}/sshd.pid KeyRegenerationInterval 3600 ServerKeyBits 768 SyslogFacility AUTH -LogLevel INFO +LogLevel DEBUG2 LoginGraceTime 120 PermitRootLogin no StrictModes no @@ -146,29 +147,13 @@ type server struct { cleanup func() // executed during Shutdown configfile string cmd *exec.Cmd + output bytes.Buffer // holds stderr from sshd process } -func (s *server) Dial() *ssh.ClientConn { - s.cmd = exec.Command("sshd", "-f", s.configfile, "-i") - stdin, err := s.cmd.StdinPipe() - if err != nil { - s.t.Fatal(err) - } - stdout, err := s.cmd.StdoutPipe() - if err != nil { - s.t.Fatal(err) - } - s.cmd.Stderr = os.Stderr - err = s.cmd.Start() - if err != nil { - s.Shutdown() - s.t.Fatal(err) - } - +func clientConfig() *ssh.ClientConfig { user, err := user.Current() if err != nil { - s.Shutdown() - s.t.Fatal(err) + panic(err) } kc := new(keychain) kc.keys = append(kc.keys, rsakey) @@ -178,8 +163,29 @@ func (s *server) Dial() *ssh.ClientConn { ssh.ClientAuthKeyring(kc), }, } + return config +} + +func (s *server) Dial(config *ssh.ClientConfig) *ssh.ClientConn { + s.cmd = exec.Command("sshd", "-f", s.configfile, "-i") + stdin, err := s.cmd.StdinPipe() + if err != nil { + s.t.Fatal(err) + } + stdout, err := s.cmd.StdoutPipe() + if err != nil { + s.t.Fatal(err) + } + s.cmd.Stderr = os.Stderr // &s.output + err = s.cmd.Start() + if err != nil { + s.t.FailNow() + s.Shutdown() + s.t.Fatal(err) + } conn, err := ssh.Client(&client{stdin, stdout}, config) if err != nil { + s.t.FailNow() s.Shutdown() s.t.Fatal(err) } @@ -190,6 +196,10 @@ func (s *server) Shutdown() { if err := s.cmd.Process.Kill(); err != nil { s.t.Error(err) } + if s.t.Failed() { + // log any output from sshd process + s.t.Log(s.output.String()) + } s.cleanup() }