ssh: correctly ignore pre-banner lines

Reading the peer's benner in readVersion() was incorrectly counting
lines sent by the server before the actual `SSH-2.0` banner against
the line limit for the banner itself.

This is incorrect per RFC 4253 section 4.2, where the limit of 255
characters is written to apply only to the `SSH-` banner string
itself.

This instead limits pre-banner lines to 255 characters each but
instead allows a maximum of 1024 lines.

Change-Id: Ief014bb3413e26e8d2077d3eb204ceaea9dc9444
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/811320
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>

ssh/transport.go

@@ -331,13 +331,19 @@ func exchangeVersions(rw io.ReadWriter, versionLine []byte) (them []byte, err er
 // chars
 const maxVersionStringBytes = 255
 
+// maxPreVersionLines is the maximum number of lines sent by the peer
+// before the version string. Each of these lines is limited to a maximum
+// of maxVersionStringBytes chars. Lines sent before the version string
+// are silently ignored.
+const maxPreVersionLines = 1024
+
 // Read version string as specified by RFC 4253, section 4.2.
 func readVersion(r io.Reader) ([]byte, error) {
 	versionString := make([]byte, 0, 64)
 	var ok bool
 	var buf [1]byte
 
-	for length := 0; length < maxVersionStringBytes; length++ {
+	for lines := 0; len(versionString) < maxVersionStringBytes && lines < maxPreVersionLines; {
 		_, err := io.ReadFull(r, buf[:])
 		if err != nil {
 			return nil, err
@@ -347,9 +353,9 @@ func readVersion(r io.Reader) ([]byte, error) {
 		if buf[0] == '\n' {
 			if !bytes.HasPrefix(versionString, []byte("SSH-")) {
 				// RFC 4253 says we need to ignore all version string lines
-				// except the one containing the SSH version (provided that
-				// all the lines do not exceed 255 bytes in total).
+				// except the one containing the SSH version.
 				versionString = versionString[:0]
+				lines++
 				continue
 			}
 			ok = true

ssh/transport_test.go

@@ -35,11 +35,15 @@ func TestReadVersion(t *testing.T) {
 }
 
 func TestReadVersionError(t *testing.T) {
-	longVersion := strings.Repeat("SSH-2.0-bla", 50)[:253]
-	multiLineVersion := strings.Repeat("ignored\r\n", 50) + "SSH-2.0-bla\r\n"
+	tooLongVersion := strings.Repeat("SSH-2.0-bla", 60) + "\r\n"
+	multiLine := strings.Repeat("ignored\r\n", 50)
+	tooManyLines := strings.Repeat("ignored\r\n", 2000)
+	tooLongLine := strings.Repeat("blah ", 100) + "\r\n"
 	cases := []string{
-		longVersion + "too-long\r\n",
-		multiLineVersion,
+		tooLongVersion,
+		multiLine + tooLongVersion,
+		tooManyLines + "SSH-2.0-bla\r\n",
+		tooLongLine + "SSH-2.0-bla\r\n",
 	}
 	for _, in := range cases {
 		if _, err := readVersion(bytes.NewBufferString(in)); err == nil {
@@ -48,6 +52,14 @@ func TestReadVersionError(t *testing.T) {
 	}
 }
 
+func TestIgnorePreVersion(t *testing.T) {
+	longishLine := strings.Repeat("blah ", 30) + "\r\n"
+	multiLineVersion := strings.Repeat(longishLine, 100) + "SSH-2.0-bla\r\n"
+	if _, err := readVersion(bytes.NewBufferString(multiLineVersion)); err != nil {
+		t.Errorf("readVersion() got error %v, expected success", err)
+	}
+}
+
 func TestExchangeVersionsBasic(t *testing.T) {
 	v := "SSH-2.0-bla"
 	buf := bytes.NewBufferString(v + "\r\n")