From e052873987615dc96fe67607a9a6adb76311344f Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 14 Dec 2025 15:32:31 +0100 Subject: [PATCH] ssh: fix infinite loop on large channel writes due to integer overflow The internal 'min' helper function in channel.go incorrectly cast the input data length (int) to uint32 before comparing it with the maximum packet size. On 64-bit systems, if the data length is a multiple of 2^32 (approx. 4GB), this cast results in 0. Consequently, the function returns 0, causing the WriteExtended loop to spin indefinitely because it attempts to reserve 0 bytes while the remaining data length is still positive. This change renames the helper to 'minPayloadSize' to avoid confusion with the Go 1.21 built-in 'min' and updates the logic to use int64 for comparisons, preventing truncation and the resulting infinite loop. This issue was found during a security audit by NCC Group Cryptography Services, sponsored by Teleport. Fixes golang/go#79567 Fixes CVE-2026-39834 Change-Id: Id5bf81d9f06c7042452acffe1c76580ff878665e Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781663 Reviewed-by: Neal Patel Reviewed-by: Roland Shoemaker LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com --- ssh/channel.go | 16 +++-- ssh/channel_test.go | 140 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 ssh/channel_test.go diff --git a/ssh/channel.go b/ssh/channel.go index 00e4ceb..77bac19 100644 --- a/ssh/channel.go +++ b/ssh/channel.go @@ -131,11 +131,17 @@ func (r RejectionReason) String() string { return fmt.Sprintf("unknown reason %d", int(r)) } -func min(a uint32, b int) uint32 { - if a < uint32(b) { - return a +// minPayloadSize returns min(limit, length) clamped to a uint32. It is used +// to compute the size of the next channel data packet from the remaining +// payload. The comparison is done in int64 because length is an int — on +// 64-bit systems len(data) can exceed 2^32, and a direct uint32(length) +// cast would silently truncate to 0 at every multiple of 2^32, causing +// WriteExtended's loop to spin without making progress. +func minPayloadSize(limit uint32, length int) uint32 { + if int64(length) > int64(limit) { + return limit } - return uint32(b) + return uint32(length) } type channelDirection uint8 @@ -251,7 +257,7 @@ func (ch *channel) WriteExtended(data []byte, extendedCode uint32) (n int, err e ch.writeMu.Unlock() for len(data) > 0 { - space := min(ch.maxRemotePayload, len(data)) + space := minPayloadSize(ch.maxRemotePayload, len(data)) if space, err = ch.remoteWin.reserve(space); err != nil { return n, err } diff --git a/ssh/channel_test.go b/ssh/channel_test.go new file mode 100644 index 0000000..2266863 --- /dev/null +++ b/ssh/channel_test.go @@ -0,0 +1,140 @@ +// Copyright 2025 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package ssh + +import ( + "math/bits" + "testing" + "time" + "unsafe" +) + +func TestMinPayloadSize(t *testing.T) { + // 4 GiB (2^32). Declared as a var (not a const) so that int(bigPayload) + // is a runtime conversion: a constant conversion would fail to compile + // on 32-bit platforms with "constant 4294967296 overflows int". On + // 32-bit the value truncates to 0 at runtime, but the is64Bit cases + // that reference it are skipped by the runtime check below. + var bigPayload int64 = 1 << 32 + + tests := []struct { + name string + maxPayload uint32 + dataLen int + want uint32 + is64Bit bool // Flag to run only on 64-bit architectures + }{ + { + name: "Normal Case - Data fits in payload", + maxPayload: 32768, + dataLen: 1000, + want: 1000, + }, + { + name: "Normal Case - Data larger than payload", + maxPayload: 32768, + dataLen: 50000, + want: 32768, + }, + { + name: "Boundary Case - Data zero", + maxPayload: 32768, + dataLen: 0, + want: 0, + }, + { + name: "Overflow Case - Data is exactly 4GB (1<<32)", + maxPayload: 32768, + dataLen: int(bigPayload), + want: 32768, + is64Bit: true, + }, + { + name: "Overflow Case - Data is 4GB + small amount", + maxPayload: 32768, + dataLen: int(bigPayload + 100), + want: 32768, + is64Bit: true, + }, + } + + is64Bit := bits.UintSize == 64 + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.is64Bit && !is64Bit { + t.Skip("Skipping test requiring 64-bit int") + } + got := minPayloadSize(tt.maxPayload, tt.dataLen) + if got != tt.want { + t.Errorf("minPayloadSize(%d, %d) = %d; want %d", tt.maxPayload, tt.dataLen, got, tt.want) + } + }) + } +} + +// TestWriteExtendedNoInfiniteLoopOnLargeWrite is an end-to-end regression +// test for the integer-overflow bug in WriteExtended. Before the fix, a +// write whose len(data) was a multiple of 2^32 caused minPayloadSize to +// return 0; WriteExtended then spun forever, reserving 0 bytes per +// iteration and never advancing the data slice. +// +// We exercise the real WriteExtended path with a slice whose declared +// length is exactly 2^32. Allocating 4 GiB is unnecessary: each iteration +// only reads up to maxRemotePayload bytes from the head of the slice, and +// the loop blocks in remoteWin.reserve() once the channel window is +// exhausted — before the slice base advances past the underlying buffer. +// +// With the fix, the loop blocks in reserve(); we detect that via +// waitWriterBlocked(), then close the window to let WriteExtended return. +// With the bug, the loop never blocks and the test times out. +// +//go:nocheckptr +func TestWriteExtendedNoInfiniteLoopOnLargeWrite(t *testing.T) { + if bits.UintSize < 64 { + t.Skip("test requires 64-bit int to construct a slice with len >= 2^32") + } + + reader, writer, mux := channelPair(t) + defer reader.Close() + defer writer.Close() + defer mux.Close() + + // Sized to hold the full pre-update remote window so that no iteration + // reads past the backing buffer before reserve() blocks. + backing := make([]byte, channelWindowSize) + var bigLen int64 = 1 << 32 + bigSlice := unsafe.Slice(&backing[0], int(bigLen)) + + done := make(chan int, 1) + go func() { + n, _ := writer.Write(bigSlice) + done <- n + }() + + blocked := make(chan struct{}) + go func() { + writer.remoteWin.waitWriterBlocked() + close(blocked) + }() + + select { + case <-blocked: + // Good — the loop made progress and is now blocked in reserve(). + // Close the window to let WriteExtended return. + writer.remoteWin.close() + case <-time.After(2 * time.Second): + t.Fatal("WriteExtended did not block in reserve within 2s — minPayloadSize likely returned 0 (integer overflow regression)") + } + + select { + case n := <-done: + if n == 0 { + t.Fatalf("WriteExtended returned n=0; expected progress") + } + case <-time.After(2 * time.Second): + t.Fatal("WriteExtended did not return after closing the window") + } +}