From b76c864ef1dca1d8f271f917c290cddcce3d9e0d Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 25 Apr 2016 15:45:47 +0200 Subject: [PATCH] x/crypto/ssh: hide msgNewKeys in the transport layer. This ensures that extraneous key exchanges cannot confuse application level code. Change-Id: I1a333e2b7b46f1e484406a79db7a949294e79c6d Reviewed-on: https://go-review.googlesource.com/22417 Reviewed-by: Han-Wen Nienhuys Run-TryBot: Han-Wen Nienhuys Reviewed-by: Adam Langley --- ssh/client.go | 6 ----- ssh/handshake.go | 51 ++++++++++++++++++++++++++++------------ ssh/handshake_test.go | 54 ++++++++++++++++++++++++++++++++----------- ssh/mux.go | 3 --- ssh/server.go | 6 ----- 5 files changed, 77 insertions(+), 43 deletions(-) diff --git a/ssh/client.go b/ssh/client.go index e0f1a4d..0212a20 100644 --- a/ssh/client.go +++ b/ssh/client.go @@ -101,12 +101,6 @@ func (c *connection) clientHandshake(dialAddress string, config *ClientConfig) e return err } - if packet, err := c.transport.readPacket(); err != nil { - return err - } else if packet[0] != msgNewKeys { - return unexpectedMessageError(msgNewKeys, packet[0]) - } - // We just did the key change, so the session ID is established. c.sessionID = c.transport.getSessionID() diff --git a/ssh/handshake.go b/ssh/handshake.go index caf36f1..86e2755 100644 --- a/ssh/handshake.go +++ b/ssh/handshake.go @@ -173,6 +173,17 @@ func (t *handshakeTransport) readOnePacket() ([]byte, error) { } t.mu.Lock() + + // By default, a key exchange is hidden from higher layers by + // translating it into msgIgnore. + successPacket := []byte{msgIgnore} + if t.sessionID == nil { + // sendKexInit() for the first kex waits for + // msgNewKeys so the authentication process is + // guaranteed to happen over an encrypted transport. + successPacket = []byte{msgNewKeys} + } + err = t.enterKeyExchangeLocked(p) if err != nil { // drop connection @@ -196,7 +207,7 @@ func (t *handshakeTransport) readOnePacket() ([]byte, error) { } t.readSinceKex = 0 - return []byte{msgNewKeys}, nil + return successPacket, nil } // keyChangeCategory describes whether a key exchange is the first on a @@ -213,20 +224,37 @@ const ( // blocked until the change is done, and a failed key change will // close the underlying transport. This function is safe for // concurrent use by multiple goroutines. -func (t *handshakeTransport) sendKexInit(isFirst keyChangeCategory) (*kexInitMsg, []byte, error) { +func (t *handshakeTransport) sendKexInit(isFirst keyChangeCategory) error { t.mu.Lock() - defer t.mu.Unlock() - return t.sendKexInitLocked(isFirst) + // If this is the initial key change, but we already have a sessionID, + // then do nothing because the key exchange has already completed + // asynchronously. + if isFirst && t.sessionID != nil { + t.mu.Unlock() + return nil + } + + _, _, err := t.sendKexInitLocked(isFirst) + t.mu.Unlock() + if err != nil { + return err + } + if isFirst { + if packet, err := t.readPacket(); err != nil { + return err + } else if packet[0] != msgNewKeys { + return unexpectedMessageError(msgNewKeys, packet[0]) + } + } + return nil } func (t *handshakeTransport) requestInitialKeyChange() error { - _, _, err := t.sendKexInit(firstKeyExchange) - return err + return t.sendKexInit(firstKeyExchange) } func (t *handshakeTransport) requestKeyChange() error { - _, _, err := t.sendKexInit(subsequentKeyExchange) - return err + return t.sendKexInit(subsequentKeyExchange) } // sendKexInitLocked sends a key change message. t.mu must be locked @@ -240,13 +268,6 @@ func (t *handshakeTransport) sendKexInitLocked(isFirst keyChangeCategory) (*kexI return t.sentInitMsg, t.sentInitPacket, nil } - // If this is the initial key change, but we already have a sessionID, - // then do nothing because the key exchange has already completed - // asynchronously. - if isFirst && t.sessionID != nil { - return nil, nil, nil - } - msg := &kexInitMsg{ KexAlgos: t.config.KeyExchanges, CiphersClientServer: t.config.Ciphers, diff --git a/ssh/handshake_test.go b/ssh/handshake_test.go index ef97d57..da53d3a 100644 --- a/ssh/handshake_test.go +++ b/ssh/handshake_test.go @@ -104,7 +104,7 @@ func TestHandshakeBasic(t *testing.T) { } if i == 5 { // halfway through, we request a key change. - _, _, err := trC.sendKexInit(subsequentKeyExchange) + err := trC.sendKexInit(subsequentKeyExchange) if err != nil { t.Fatalf("sendKexInit: %v", err) } @@ -161,7 +161,7 @@ func TestHandshakeError(t *testing.T) { } // Now request a key change. - _, _, err = trC.sendKexInit(subsequentKeyExchange) + err = trC.sendKexInit(subsequentKeyExchange) if err != nil { t.Errorf("sendKexInit: %v", err) } @@ -184,6 +184,28 @@ func TestHandshakeError(t *testing.T) { } } +func TestForceFirstKex(t *testing.T) { + checker := &testChecker{} + trC, trS, err := handshakePair(&ClientConfig{HostKeyCallback: checker.Check}, "addr") + if err != nil { + t.Fatalf("handshakePair: %v", err) + } + + defer trC.Close() + defer trS.Close() + + trC.writePacket(Marshal(&serviceRequestMsg{serviceUserAuth})) + + // We setup the initial key exchange, but the remote side + // tries to send serviceRequestMsg in cleartext, which is + // disallowed. + + err = trS.sendKexInit(firstKeyExchange) + if err == nil { + t.Errorf("server first kex init should reject unexpected packet") + } +} + func TestHandshakeTwice(t *testing.T) { checker := &testChecker{} trC, trS, err := handshakePair(&ClientConfig{HostKeyCallback: checker.Check}, "addr") @@ -194,18 +216,25 @@ func TestHandshakeTwice(t *testing.T) { defer trC.Close() defer trS.Close() + // Both sides should ask for the first key exchange first. + err = trS.sendKexInit(firstKeyExchange) + if err != nil { + t.Errorf("server sendKexInit: %v", err) + } + + err = trC.sendKexInit(firstKeyExchange) + if err != nil { + t.Errorf("client sendKexInit: %v", err) + } + + sent := 0 // send a packet packet := make([]byte, 5) packet[0] = msgRequestSuccess if err := trC.writePacket(packet); err != nil { t.Errorf("writePacket: %v", err) } - - // Now request a key change. - _, _, err = trC.sendKexInit(subsequentKeyExchange) - if err != nil { - t.Errorf("sendKexInit: %v", err) - } + sent++ // Send another packet. Use a fresh one, since writePacket destroys. packet = make([]byte, 5) @@ -213,9 +242,10 @@ func TestHandshakeTwice(t *testing.T) { if err := trC.writePacket(packet); err != nil { t.Errorf("writePacket: %v", err) } + sent++ // 2nd key change. - _, _, err = trC.sendKexInit(subsequentKeyExchange) + err = trC.sendKexInit(subsequentKeyExchange) if err != nil { t.Errorf("sendKexInit: %v", err) } @@ -225,17 +255,15 @@ func TestHandshakeTwice(t *testing.T) { if err := trC.writePacket(packet); err != nil { t.Errorf("writePacket: %v", err) } + sent++ packet = make([]byte, 5) packet[0] = msgRequestSuccess - for i := 0; i < 5; i++ { + for i := 0; i < sent; i++ { msg, err := trS.readPacket() if err != nil { t.Fatalf("server closed too soon: %v", err) } - if msg[0] == msgNewKeys { - continue - } if bytes.Compare(msg, packet) != 0 { t.Errorf("packet %d: got %q want %q", i, msg, packet) diff --git a/ssh/mux.go b/ssh/mux.go index fb54c8b..f3a3ddd 100644 --- a/ssh/mux.go +++ b/ssh/mux.go @@ -227,9 +227,6 @@ func (m *mux) onePacket() error { } switch packet[0] { - case msgNewKeys: - // Ignore notification of key change. - return nil case msgChannelOpen: return m.handleChannelOpen(packet) case msgGlobalRequest, msgRequestSuccess, msgRequestFailure: diff --git a/ssh/server.go b/ssh/server.go index d530501..9b29b15 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -192,12 +192,6 @@ func (s *connection) serverHandshake(config *ServerConfig) (*Permissions, error) return nil, err } - if packet, err := s.transport.readPacket(); err != nil { - return nil, err - } else if packet[0] != msgNewKeys { - return nil, unexpectedMessageError(msgNewKeys, packet[0]) - } - // We just did the key change, so the session ID is established. s.sessionID = s.transport.getSessionID()