From 593c81af8aa6582d85a7faaeb996396f63d712a9 Mon Sep 17 00:00:00 2001 From: Damien Miller Date: Thu, 6 Aug 2026 18:06:24 +1000 Subject: [PATCH] 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 Reviewed-by: Nicola Murino Reviewed-by: Dmitri Shuralyov Auto-Submit: Dmitri Shuralyov Reviewed-by: Carlos Amedee --- ssh/transport.go | 12 +++++++++--- ssh/transport_test.go | 20 ++++++++++++++++---- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/ssh/transport.go b/ssh/transport.go index fa3dd6a..540865d 100644 --- a/ssh/transport.go +++ b/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 diff --git a/ssh/transport_test.go b/ssh/transport_test.go index 8445e1e..f20cd96 100644 --- a/ssh/transport_test.go +++ b/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")