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 <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Adam Langley <agl@golang.org>

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()
 

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,

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)

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:

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()