ch.msg is only read while the channel open or a channel request with a
reply is pending, so anything the default arm of channel.handlePacket
delivered to it was never consumed. The blocking send there let a
misbehaving peer fill the buffer with well-formed but unexpected message
types carrying a valid channel id and stall the mux read loop,
deadlocking the whole connection.
No conforming peer sends such messages during the connection protocol.
Treat them as a protocol error and tear the connection down, as
handleUnknownChannelPacket already does for the same messages when the
channel id is not in use.
Fixes CVE-2026-56855
Fixesgolang/go#81317
Change-Id: I87420dfe68fcb62a17df4b47dc5ffb6ccd72ba26
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/826524
Reviewed-by: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Neal Patel <nealpatel@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
A channel in the mux's chanList is not usable until it is established:
an outbound channel has no confirmed remote id until the peer's open
confirmation, and an inbound channel is not serviced by the application
until it is accepted. handlePacket processed any channel message on it,
so a misbehaving peer could flood channel requests and block the mux
read loop on the send to incomingRequests, deadlocking the connection,
or close an outbound channel before confirming it, making the victim
tear down a half-initialized channel and emit a close for remote id 0,
an unrelated channel of the peer.
No such packet can be legitimate: the peer learns an inbound channel's
local id only from the confirmation we have not sent yet, and on an
outbound channel RFC 4254 lets it answer the open request only with a
confirmation or a failure.
Add an established flag, set when the channel becomes usable: for an
outbound channel when the open response is received, for an inbound
channel by Accept before the confirmation is sent. Until then
handlePacket drops every packet other than the open response. The flag
is separate from decided, which Reject also sets: a rejected channel is
decided but must never carry traffic.
Fixes CVE-2026-78662
Fixesgolang/go#81316
Change-Id: Ib0983bb216a49808a2db1f4a4d92ee9fe38a3c51
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/826504
Auto-Submit: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Neal Patel <nealpatel@google.com>
Reviewed-by: Nicholas Husin <husin@google.com>
handleData subtracts the length of every data packet from the receive
window, but extended data with a type code other than
SSH2_EXTENDED_DATA_STDERR is then silently discarded and, since no API
exists to read it, its window credit was never returned: adjustWindow
only runs as a side effect of reading a stream. A peer sending
extended data with an unknown type code could therefore permanently
shrink the channel window, down to a complete stall of the channel.
Credit the discarded bytes back through adjustWindow. adjustWindow
returns io.EOF if the local side has already sent a channel close;
ignore it like ReadExtended does, since an error returned here would
terminate the mux read loop and tear down the whole connection. For
comparison, OpenSSH rejects extended data with unexpected type codes
outright and never buffers it, so the dropped bytes are simply never
credited back to the misbehaving peer.
Fixesgolang/go#80333
Change-Id: Ia311ce3ad008e7c89ce9752f1937c91374a4e74f
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/802901
Reviewed-by: Mark Freeman <markfreeman@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
newChannel published the channel into the mux's chanList (via
chanList.add) before assigning its localId, and openChannel/Accept set
maxIncomingPayload only after newChannel returned. For an outbound
channel, whose newChannel runs on an application goroutine, the mux
loop could observe the channel through getChan before those fields
were initialized, reading a zero localId or maxIncomingPayload with no
happens-before relationship.
Assign localId inside add while the chanList lock is held, and set
maxIncomingPayload (always channelMaxPacket) in newChannel itself, so
a channel returned by getChan is always fully initialized.
Change-Id: Ib54a5d10dcb5c1ad8ed4dabe5d8d2790778be298
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/797061
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
Previously, channel.handlePacket sent channelRequestSuccess and
channelRequestFailure messages to ch.msg unconditionally via the default
arm of its type switch. Because ch.msg is a bounded buffer (chanSize),
a peer that sends a burst of unsolicited channel request responses for
an open, idle channel fills the buffer and blocks the mux read loop on
the next send. That stalls all packet processing on the connection,
and because readLoop then backs up on t.incoming, closing the
underlying net.Conn does not unblock either goroutine: user code
observes Close() returning promptly while Wait() hangs and the mux,
readLoop, and kexLoop goroutines leak permanently.
This change mirrors the fix for the mux-level SendRequest path: a
sentRequestPending atomic gate is set while a SendRequest with
WantReply is in flight, handlePacket drops responses when the gate is
closed, and uses a non-blocking send otherwise. SendRequest drains
any spurious response that slipped through before discarding it, so
the caller always observes the reply to its own request.
This aligns with OpenSSH, which silently ignores channel confirm
messages that do not match a pending request.
Fixesgolang/go#79564
Fixes CVE-2026-39830
Change-Id: I15e2add4bf7876bb0c6f921f8b57203d97e83f47
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781664
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Neal Patel <nealpatel@google.com>
Reviewed-by: Neal Patel <nealpatel@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
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.
Fixesgolang/go#79567
Fixes CVE-2026-39834
Change-Id: Id5bf81d9f06c7042452acffe1c76580ff878665e
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781663
Reviewed-by: Neal Patel <nealpatel@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
When a server rejects an incoming channel request via
NewChannel.Reject, the channel is left in the multiplexer's
channel list. Because the channel is never explicitly removed or
closed, its internal buffers and sync primitives remain allocated
for the lifetime of the SSH connection.
A malicious client could exploit this behavior by repeatedly
requesting to open channels that are destined to be rejected,
causing unbounded memory growth and potentially leading to a
Denial of Service (DoS) via resource exhaustion.
This change fixes the leak by calling ch.mux.chanList.remove
within the Reject method, removing the channel from the list and allowing the
garbage collector to reclaim the associated memory immediately.
Fixesgolang/go#35127
Fixes CVE-2026-3982
Change-Id: Iaa177f5dfd151812dd404e528a4a1c77527a0e29
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781320
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>
Sending a window adjustment after every read is unnecessarily chatty,
especially with a series of small reads like with TTY interactions.
Copy OpenSSH's logic for deferring these, which seemingly hasn't changed
since 2007. Note that since channelWindowSize and c.maxIncomingPayload
are currently constants here, the two checks could be combined into a
single check for c.myWindow < 2 MiB - 96 KiB (with the current values
of the constants).
Fixesgolang/go#57424.
Change-Id: Ifcef5be76fcc3f0b1a6dc396096bed9c50d64f21
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/459915
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Nicola Murino <nicola.murino@gmail.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Commit-Queue: Nicola Murino <nicola.murino@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
None are "wrong" per se, but there are a lot of good suggestions and
in one case a docstring that was not present in godoc due to the
presence of an extra newline.
Changed "Id" in struct properties to "ID" in some non-exported
structs. Removed a trailing period from some error messages; I believe
the exact contents of error strings are not covered by the Go
compatibility promise.
Change-Id: I7c620582dc247396f72c52d38c909ccc0ec87b83
Reviewed-on: https://go-review.googlesource.com/80145
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Test that different extended data streams within a channel are
thread-safe.
benchmark old MB/s new MB/s speedup
BenchmarkEndToEnd 79.26 87.98 1.11x
benchmark old allocs new allocs delta
BenchmarkEndToEnd 110 73 -33.64%
benchmark old bytes new bytes delta
BenchmarkEndToEnd 2605720 1299768 -50.12%
LGTM=dave, jpsugar
R=agl, dave, jpsugar
CC=golang-codereviews
https://golang.org/cl/136420043
Increase window size for channels (session and tcpip) to 64 *
max packet size (32 KB), which is the same value that OpenSSH
uses. Also breaks out the relevant harcoded constants into named
constants in channel.go.
Fixesgolang/go#6675.
R=golang-dev, dave, hanwen, agl
CC=golang-dev
https://golang.org/cl/18120043
Adds readPacket() to conn, and renames conn to packetConn.
Key exchanges operate on packetConn, so they can be
unittested.
R=agl, jpsugar, dave
CC=golang-dev
https://golang.org/cl/13352055
I ran go vet over all of go.crypto and removed lots of panic("unreachable") that are no longer needed.
R=golang-dev, r, dgryski
CC=golang-dev
https://golang.org/cl/10113043
Fixesgolang/go#3810.
Fixes chanWriter Write after close behaviour bug.
Fixes serverChan writePacket after close bug.
Addresses final comments by agl on 6405064, plus various cleanups.
R=agl, kardianos, gustav.paul, fullung
CC=golang-dev
https://golang.org/cl/6479056
Fixesgolang/go#3810.
This change introduces an atomic boolean to guard the close
of the clientChan. Previously the client code was quite
lax with the ordering of the close messages and could allow
window adjustment or EOF messages to leak after Close had
been signaled.
Consolidating the changes to the serverChan will be handled
in a following CL.
R=agl, kardianos, gustav.paul
CC=golang-dev
https://golang.org/cl/6405064
This proposal moves the check for max packet into
channel.writePacket. Callers should be aware they cannot
pass a buffer larger than max packet. This is only a
concern to chanWriter.Write and appropriate guards are
already in place.
There was some max packet handling in transport.go but it was
incorrect. This has been removed.
This proposal also cleans up session_test.go.
R=gustav.paul, agl, fullung, huin
CC=golang-dev
https://golang.org/cl/6460075
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
I introduced this problem in change set 33 when I redefined
RejectionReason from a uint32 to an int. I have added a test
to verify the message can be marshaled correctly.
This was a bit hard to track down as the recover in clientConn
mainLoop would catch the panic and shutdown the connection. We
probably shouldn't be using panic inside marshal/unmarshal, at
least not without a way to let the user know why the recover
logic tripped.
R=agl, kardianos, gustav.paul
CC=golang-dev
https://golang.org/cl/6210063
This CL introduces a new struct, channel to hold common shared
functions.
* add a new channel struct, which is embeded in {client,server}Chan.
* move common methods from {client,server}Chan into channel.
* remove unneeded used of serverConn.lock in serverChan
(transport.writePacket has its own mutex).
* remove filteredConn, introduce conn.
R=agl, gustav.paul
CC=golang-dev
https://golang.org/cl/6128059
Add support for server (remote) forwarded tcpip channels.
See RFC4254 Section 7.1
R=gustav.paul, jeff, agl, lieqiewang
CC=golang-dev
https://golang.org/cl/6038047
These are the cosmetic cleanups from the bits of code that I
rereviewed.
1) stringLength now takes a int; the length of the string. Too many
callers were allocating with stringLength([]byte(s)) and
stringLength only needs to call len().
2) agent.go now has sendAndReceive to remove logic that was
duplicated.
3) We now reject negative DH values
4) We now reject empty packets rather than crashing.
R=dave, jonathan.mark.pittman
CC=golang-dev
https://golang.org/cl/6061052
These are the obvious mistakes from my read through. I'll save the
more cosmetic changes for a later CL.
R=dave, kardianos
CC=golang-dev
https://golang.org/cl/6049050
Since a lock is retaken before sync.Cond.Wait returns, this could
deadlock when the for loop attempts to take the lock again. (Reported
by sanjay.m.)
theirWindow was used outside of the lock, therefore concurrent writers
could overrun the window.
theirWindow was never updated to reflect the data written.
R=dave, balasanjay
CC=golang-dev
https://golang.org/cl/5671084
Manual edits to README.
Moved from main Go repository, deleted Makefiles, ran gofix -r go1rename.
Tested with: go test code.google.com/p/go.crypto/...
R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/5564059
Nicola Murino
Will Mortensen
Kevin Burke
Han-Wen Nienhuys
Adam Langley
Jakob Borg
Jonathan Pittman
Gustav Paul
Dave Cheney
Daniel Theophanes
Russ Cox