|
| 1 | +From cb14bc802f17b4501bbdea20a66999158136158a Mon Sep 17 00:00:00 2001 |
| 2 | +From: Nicola Murino <nicola.murino@gmail.com> |
| 3 | +Date: Sun, 14 Dec 2025 15:32:31 +0100 |
| 4 | +Subject: [PATCH] ssh: fix infinite loop on large channel writes due to integer |
| 5 | + overflow |
| 6 | + |
| 7 | +The internal 'min' helper function in channel.go incorrectly cast the |
| 8 | +input data length (int) to uint32 before comparing it with the |
| 9 | +maximum packet size. On 64-bit systems, if the data length is a |
| 10 | +multiple of 2^32 (approx. 4GB), this cast results in 0. |
| 11 | + |
| 12 | +Consequently, the function returns 0, causing the WriteExtended loop |
| 13 | +to spin indefinitely because it attempts to reserve 0 bytes while |
| 14 | +the remaining data length is still positive. |
| 15 | + |
| 16 | +This change renames the helper to 'minPayloadSize' to avoid confusion |
| 17 | +with the Go 1.21 built-in 'min' and updates the logic to use int64 |
| 18 | +for comparisons, preventing truncation and the resulting infinite loop. |
| 19 | + |
| 20 | +This issue was found during a security audit by NCC Group Cryptography |
| 21 | +Services, sponsored by Teleport. |
| 22 | + |
| 23 | +Fixes golang/go#79567 |
| 24 | +Fixes CVE-2026-39834 |
| 25 | + |
| 26 | +Change-Id: Id5bf81d9f06c7042452acffe1c76580ff878665e |
| 27 | +Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781663 |
| 28 | +Reviewed-by: Neal Patel <nealpatel@google.com> |
| 29 | +Reviewed-by: Roland Shoemaker <roland@golang.org> |
| 30 | +LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> |
| 31 | +Signed-off-by: Azure Linux Security Servicing Account <azurelinux-security@microsoft.com> |
| 32 | +Upstream-reference: https://github.com/TheDegenerateDev5150/crypto/commit/e052873987615dc96fe67607a9a6adb76311344f.patch |
| 33 | +--- |
| 34 | + ssh/channel_test.go | 140 ++++++++++++++++++ |
| 35 | + .../vendor/golang.org/x/crypto/ssh/channel.go | 16 +- |
| 36 | + 2 files changed, 151 insertions(+), 5 deletions(-) |
| 37 | + create mode 100644 ssh/channel_test.go |
| 38 | + |
| 39 | +diff --git a/ssh/channel_test.go b/ssh/channel_test.go |
| 40 | +new file mode 100644 |
| 41 | +index 0000000..2266863 |
| 42 | +--- /dev/null |
| 43 | ++++ b/ssh/channel_test.go |
| 44 | +@@ -0,0 +1,140 @@ |
| 45 | ++// Copyright 2025 The Go Authors. All rights reserved. |
| 46 | ++// Use of this source code is governed by a BSD-style |
| 47 | ++// license that can be found in the LICENSE file. |
| 48 | ++ |
| 49 | ++package ssh |
| 50 | ++ |
| 51 | ++import ( |
| 52 | ++ "math/bits" |
| 53 | ++ "testing" |
| 54 | ++ "time" |
| 55 | ++ "unsafe" |
| 56 | ++) |
| 57 | ++ |
| 58 | ++func TestMinPayloadSize(t *testing.T) { |
| 59 | ++ // 4 GiB (2^32). Declared as a var (not a const) so that int(bigPayload) |
| 60 | ++ // is a runtime conversion: a constant conversion would fail to compile |
| 61 | ++ // on 32-bit platforms with "constant 4294967296 overflows int". On |
| 62 | ++ // 32-bit the value truncates to 0 at runtime, but the is64Bit cases |
| 63 | ++ // that reference it are skipped by the runtime check below. |
| 64 | ++ var bigPayload int64 = 1 << 32 |
| 65 | ++ |
| 66 | ++ tests := []struct { |
| 67 | ++ name string |
| 68 | ++ maxPayload uint32 |
| 69 | ++ dataLen int |
| 70 | ++ want uint32 |
| 71 | ++ is64Bit bool // Flag to run only on 64-bit architectures |
| 72 | ++ }{ |
| 73 | ++ { |
| 74 | ++ name: "Normal Case - Data fits in payload", |
| 75 | ++ maxPayload: 32768, |
| 76 | ++ dataLen: 1000, |
| 77 | ++ want: 1000, |
| 78 | ++ }, |
| 79 | ++ { |
| 80 | ++ name: "Normal Case - Data larger than payload", |
| 81 | ++ maxPayload: 32768, |
| 82 | ++ dataLen: 50000, |
| 83 | ++ want: 32768, |
| 84 | ++ }, |
| 85 | ++ { |
| 86 | ++ name: "Boundary Case - Data zero", |
| 87 | ++ maxPayload: 32768, |
| 88 | ++ dataLen: 0, |
| 89 | ++ want: 0, |
| 90 | ++ }, |
| 91 | ++ { |
| 92 | ++ name: "Overflow Case - Data is exactly 4GB (1<<32)", |
| 93 | ++ maxPayload: 32768, |
| 94 | ++ dataLen: int(bigPayload), |
| 95 | ++ want: 32768, |
| 96 | ++ is64Bit: true, |
| 97 | ++ }, |
| 98 | ++ { |
| 99 | ++ name: "Overflow Case - Data is 4GB + small amount", |
| 100 | ++ maxPayload: 32768, |
| 101 | ++ dataLen: int(bigPayload + 100), |
| 102 | ++ want: 32768, |
| 103 | ++ is64Bit: true, |
| 104 | ++ }, |
| 105 | ++ } |
| 106 | ++ |
| 107 | ++ is64Bit := bits.UintSize == 64 |
| 108 | ++ |
| 109 | ++ for _, tt := range tests { |
| 110 | ++ t.Run(tt.name, func(t *testing.T) { |
| 111 | ++ if tt.is64Bit && !is64Bit { |
| 112 | ++ t.Skip("Skipping test requiring 64-bit int") |
| 113 | ++ } |
| 114 | ++ got := minPayloadSize(tt.maxPayload, tt.dataLen) |
| 115 | ++ if got != tt.want { |
| 116 | ++ t.Errorf("minPayloadSize(%d, %d) = %d; want %d", tt.maxPayload, tt.dataLen, got, tt.want) |
| 117 | ++ } |
| 118 | ++ }) |
| 119 | ++ } |
| 120 | ++} |
| 121 | ++ |
| 122 | ++// TestWriteExtendedNoInfiniteLoopOnLargeWrite is an end-to-end regression |
| 123 | ++// test for the integer-overflow bug in WriteExtended. Before the fix, a |
| 124 | ++// write whose len(data) was a multiple of 2^32 caused minPayloadSize to |
| 125 | ++// return 0; WriteExtended then spun forever, reserving 0 bytes per |
| 126 | ++// iteration and never advancing the data slice. |
| 127 | ++// |
| 128 | ++// We exercise the real WriteExtended path with a slice whose declared |
| 129 | ++// length is exactly 2^32. Allocating 4 GiB is unnecessary: each iteration |
| 130 | ++// only reads up to maxRemotePayload bytes from the head of the slice, and |
| 131 | ++// the loop blocks in remoteWin.reserve() once the channel window is |
| 132 | ++// exhausted — before the slice base advances past the underlying buffer. |
| 133 | ++// |
| 134 | ++// With the fix, the loop blocks in reserve(); we detect that via |
| 135 | ++// waitWriterBlocked(), then close the window to let WriteExtended return. |
| 136 | ++// With the bug, the loop never blocks and the test times out. |
| 137 | ++// |
| 138 | ++//go:nocheckptr |
| 139 | ++func TestWriteExtendedNoInfiniteLoopOnLargeWrite(t *testing.T) { |
| 140 | ++ if bits.UintSize < 64 { |
| 141 | ++ t.Skip("test requires 64-bit int to construct a slice with len >= 2^32") |
| 142 | ++ } |
| 143 | ++ |
| 144 | ++ reader, writer, mux := channelPair(t) |
| 145 | ++ defer reader.Close() |
| 146 | ++ defer writer.Close() |
| 147 | ++ defer mux.Close() |
| 148 | ++ |
| 149 | ++ // Sized to hold the full pre-update remote window so that no iteration |
| 150 | ++ // reads past the backing buffer before reserve() blocks. |
| 151 | ++ backing := make([]byte, channelWindowSize) |
| 152 | ++ var bigLen int64 = 1 << 32 |
| 153 | ++ bigSlice := unsafe.Slice(&backing[0], int(bigLen)) |
| 154 | ++ |
| 155 | ++ done := make(chan int, 1) |
| 156 | ++ go func() { |
| 157 | ++ n, _ := writer.Write(bigSlice) |
| 158 | ++ done <- n |
| 159 | ++ }() |
| 160 | ++ |
| 161 | ++ blocked := make(chan struct{}) |
| 162 | ++ go func() { |
| 163 | ++ writer.remoteWin.waitWriterBlocked() |
| 164 | ++ close(blocked) |
| 165 | ++ }() |
| 166 | ++ |
| 167 | ++ select { |
| 168 | ++ case <-blocked: |
| 169 | ++ // Good — the loop made progress and is now blocked in reserve(). |
| 170 | ++ // Close the window to let WriteExtended return. |
| 171 | ++ writer.remoteWin.close() |
| 172 | ++ case <-time.After(2 * time.Second): |
| 173 | ++ t.Fatal("WriteExtended did not block in reserve within 2s — minPayloadSize likely returned 0 (integer overflow regression)") |
| 174 | ++ } |
| 175 | ++ |
| 176 | ++ select { |
| 177 | ++ case n := <-done: |
| 178 | ++ if n == 0 { |
| 179 | ++ t.Fatalf("WriteExtended returned n=0; expected progress") |
| 180 | ++ } |
| 181 | ++ case <-time.After(2 * time.Second): |
| 182 | ++ t.Fatal("WriteExtended did not return after closing the window") |
| 183 | ++ } |
| 184 | ++} |
| 185 | +diff --git a/tests/vendor/golang.org/x/crypto/ssh/channel.go b/tests/vendor/golang.org/x/crypto/ssh/channel.go |
| 186 | +index cc0bb7a..3967b65 100644 |
| 187 | +--- a/tests/vendor/golang.org/x/crypto/ssh/channel.go |
| 188 | ++++ b/tests/vendor/golang.org/x/crypto/ssh/channel.go |
| 189 | +@@ -131,11 +131,17 @@ func (r RejectionReason) String() string { |
| 190 | + return fmt.Sprintf("unknown reason %d", int(r)) |
| 191 | + } |
| 192 | + |
| 193 | +-func min(a uint32, b int) uint32 { |
| 194 | +- if a < uint32(b) { |
| 195 | +- return a |
| 196 | ++// minPayloadSize returns min(limit, length) clamped to a uint32. It is used |
| 197 | ++// to compute the size of the next channel data packet from the remaining |
| 198 | ++// payload. The comparison is done in int64 because length is an int — on |
| 199 | ++// 64-bit systems len(data) can exceed 2^32, and a direct uint32(length) |
| 200 | ++// cast would silently truncate to 0 at every multiple of 2^32, causing |
| 201 | ++// WriteExtended's loop to spin without making progress. |
| 202 | ++func minPayloadSize(limit uint32, length int) uint32 { |
| 203 | ++ if int64(length) > int64(limit) { |
| 204 | ++ return limit |
| 205 | + } |
| 206 | +- return uint32(b) |
| 207 | ++ return uint32(length) |
| 208 | + } |
| 209 | + |
| 210 | + type channelDirection uint8 |
| 211 | +@@ -251,7 +257,7 @@ func (ch *channel) WriteExtended(data []byte, extendedCode uint32) (n int, err e |
| 212 | + ch.writeMu.Unlock() |
| 213 | + |
| 214 | + for len(data) > 0 { |
| 215 | +- space := min(ch.maxRemotePayload, len(data)) |
| 216 | ++ space := minPayloadSize(ch.maxRemotePayload, len(data)) |
| 217 | + if space, err = ch.remoteWin.reserve(space); err != nil { |
| 218 | + return n, err |
| 219 | + } |
| 220 | +-- |
| 221 | +2.45.4 |
| 222 | + |
0 commit comments