go.crypto/ssh: never send more data than maxpacket

RFC 4254 s5.2 is clear that a client must never send a data
packet larger than the value of maximum packet supplied by the
remote side during channel setup. The client was not honoring
this value, in fact it wasn't even recording it.

Thanks to Albert Strasheim for the bug report.

R=agl, fullung
CC=golang-dev
https://golang.org/cl/6448128

ssh/channel.go

@@ -74,6 +74,7 @@ type channel struct {
 	conn              // the underlying transport
 	localId, remoteId uint32
 	remoteWin         window
+	maxPacketSize     uint32
 
 	theyClosed  bool // indicates the close msg has been received from the remote side
 	weClosed    bool // incidates the close msg has been sent from our side
@@ -119,10 +120,9 @@ type serverChan struct {
 	chanType  string
 	extraData []byte
 
-	serverConn    *ServerConn
-	myWindow      uint32
-	maxPacketSize uint32
-	err           error
+	serverConn *ServerConn
+	myWindow   uint32
+	err        error
 
 	pendingRequests []ChannelRequest
 	pendingData     []byte

ssh/client.go

@@ -499,6 +499,8 @@ func (c *clientChan) waitForChannelOpenResponse() error {
 	case *channelOpenConfirmMsg:
 		// fixup remoteId field
 		c.remoteId = msg.MyId
+		// TODO(dfc) asset this is < 2^31.
+		c.maxPacketSize = msg.MaxPacketSize
 		c.remoteWin.add(msg.MyWindow)
 		return nil
 	case *channelOpenFailureMsg:
@@ -582,9 +584,10 @@ type chanWriter struct {
 // Write writes data to the remote process's standard input.
 func (w *chanWriter) Write(data []byte) (written int, err error) {
 	for len(data) > 0 {
-		// n cannot be larger than 2^31 as len(data) cannot
-		// be larger than 2^31
-		n := int(w.remoteWin.reserve(uint32(len(data))))
+		// never send more data than maxPacketSize even if
+		// there is sufficent window.
+		n := min(int(w.maxPacketSize), len(data))
+		n = int(w.remoteWin.reserve(uint32(n)))
 		remoteId := w.remoteId
 		packet := []byte{
 			msgChannelData,

ssh/server.go

@@ -569,14 +569,15 @@ func (s *ServerConn) Accept() (Channel, error) {
 						conn:      s,
 						remoteId:  msg.PeersId,
 						remoteWin: window{Cond: newCond()},
+						// TODO(dfc) assert this param is < 2^31.
+						maxPacketSize: msg.MaxPacketSize,
 					},
-					chanType:      msg.ChanType,
-					maxPacketSize: msg.MaxPacketSize,
-					extraData:     msg.TypeSpecificData,
-					myWindow:      defaultWindowSize,
-					serverConn:    s,
-					cond:          newCond(),
-					pendingData:   make([]byte, defaultWindowSize),
+					chanType:    msg.ChanType,
+					extraData:   msg.TypeSpecificData,
+					myWindow:    defaultWindowSize,
+					serverConn:  s,
+					cond:        newCond(),
+					pendingData: make([]byte, defaultWindowSize),
 				}
 				c.remoteWin.add(msg.PeersWindow)
 				s.lock.Lock()

ssh/session_test.go

@@ -9,6 +9,7 @@ package ssh
 import (
 	"bytes"
 	"io"
+	"io/ioutil"
 	"net"
 	"testing"
 
@@ -338,6 +339,26 @@ func TestServerZeroWindowAdjust(t *testing.T) {
 	}
 }
 
+// Verify that we never send a packet larger than maxpacket.
+func TestClientStdinRespectsMaxPacketSize(t *testing.T) {
+	conn := dial(discardHandler, t)
+	defer conn.Close()
+	session, err := conn.NewSession()
+	if err != nil {
+		t.Fatalf("Unable to request new session: %s", err)
+	}
+	defer session.Close()
+	if err := session.Shell(); err != nil {
+		t.Fatalf("Unable to execute command: %s", err)
+	}
+	// try to stuff 128k of data into a 32k hole.
+	const size = 128 * 1024
+	n, err := session.clientChan.stdin.Write(make([]byte, size))
+	if n != size || err != nil {
+		t.Fatalf("failed to write: %d, %v", n, err)
+	}
+}
+
 type exitStatusMsg struct {
 	PeersId   uint32
 	Request   string
@@ -456,3 +477,13 @@ func sendZeroWindowAdjust(ch *serverChan) {
 	shell.ReadLine()
 	sendStatus(0, ch)
 }
+
+func discardHandler(ch *serverChan) {
+	defer ch.Close()
+	// grow the window to avoid being fooled by
+	// the initial 1 << 14 window.
+	ch.sendWindowAdj(1024 * 1024)
+	shell := newServerShell(ch, "> ")
+	shell.ReadLine()
+	io.Copy(ioutil.Discard, ch.serverConn)
+}