ssh: fix deadlock on unexpected global responses
Previously, the mux implementation handled global request responses by blocking until the response could be sent to the globalResponses channel. Since this channel has a buffer size of 1, unsolicited responses from a server (or responses arriving after a timeout) would fill the buffer. Subsequent unsolicited responses would block handleGlobalPacket, stalling the entire connection's read loop and causing a denial of service. This change modifies handleGlobalPacket to use a non-blocking send. If no goroutine is waiting for a response (or the buffer is full), the message is dropped. This aligns with OpenSSH behavior, which ignores unexpected global responses. Additionally, SendRequest now drains the globalResponses channel after acquiring the mutex but before sending the request. This ensures that any stale responses or "spam" buffered just before the lock was acquired are discarded, preventing race conditions where a legitimate request might otherwise consume an unrelated response. This issue was found during a security audit by NCC Group Cryptography Services, sponsored by Teleport. Fixes golang/go#79564 Fixes CVE-2026-39830 Change-Id: Ia0c46355203d557eadcd432c10b87c8a044e1089 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/781640 Reviewed-by: Roland Shoemaker <roland@golang.org> Reviewed-by: Neal Patel <nealpatel@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This commit is contained in:
committed by
Neal Patel
parent
b25012b37b
commit
4e7a7384ec
+32
-4
@@ -91,9 +91,10 @@ type mux struct {
|
||||
|
||||
incomingChannels chan NewChannel
|
||||
|
||||
globalSentMu sync.Mutex
|
||||
globalResponses chan interface{}
|
||||
incomingRequests chan *Request
|
||||
globalSentMu sync.Mutex
|
||||
globalSentPending atomic.Bool
|
||||
globalResponses chan interface{}
|
||||
incomingRequests chan *Request
|
||||
|
||||
errCond *sync.Cond
|
||||
err error
|
||||
@@ -141,6 +142,24 @@ func (m *mux) SendRequest(name string, wantReply bool, payload []byte) (bool, []
|
||||
if wantReply {
|
||||
m.globalSentMu.Lock()
|
||||
defer m.globalSentMu.Unlock()
|
||||
|
||||
// Open the gate so that responses arriving while this request is in
|
||||
// flight are allowed to reach globalResponses. Any response arriving
|
||||
// while no request is pending is dropped by handleGlobalPacket.
|
||||
m.globalSentPending.Store(true)
|
||||
defer m.globalSentPending.Store(false)
|
||||
|
||||
// Drain any spurious responses that may have been buffered. This prevents
|
||||
// a previously buffered unexpected response from being consumed instead
|
||||
// of the actual response for this request.
|
||||
drain:
|
||||
for {
|
||||
select {
|
||||
case <-m.globalResponses:
|
||||
default:
|
||||
break drain
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if err := m.sendMessage(globalRequestMsg{
|
||||
@@ -267,7 +286,16 @@ func (m *mux) handleGlobalPacket(packet []byte) error {
|
||||
mux: m,
|
||||
}
|
||||
case *globalRequestSuccessMsg, *globalRequestFailureMsg:
|
||||
m.globalResponses <- msg
|
||||
// Drop responses that arrive when no SendRequest is waiting, to
|
||||
// prevent a malicious peer from staging responses for a future
|
||||
// caller.
|
||||
if !m.globalSentPending.Load() {
|
||||
return nil
|
||||
}
|
||||
select {
|
||||
case m.globalResponses <- msg:
|
||||
default:
|
||||
}
|
||||
default:
|
||||
panic(fmt.Sprintf("not a global message %#v", msg))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user