mirror of
https://github.com/henrygd/beszel.git
synced 2026-09-29 12:57:50 +02:00
fix(hub): bound SSH handshake so a silent peer can't hang the updater
ssh.ClientConfig.Timeout only covers the TCP connect. A peer that accepts the connection but never sends an SSH banner blocked ssh.NewClientConn forever, wedging the system's updater goroutine and leaking the socket. Set a deadline on the conn for the handshake and clear it afterward.
This commit is contained in:
@@ -3,13 +3,17 @@
|
||||
package systems
|
||||
|
||||
import (
|
||||
"crypto/ed25519"
|
||||
"crypto/rand"
|
||||
"errors"
|
||||
"net"
|
||||
"sync"
|
||||
"testing"
|
||||
"testing/synctest"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/crypto/ssh"
|
||||
)
|
||||
|
||||
@@ -91,3 +95,120 @@ func TestCreateSessionDuringClose(t *testing.T) {
|
||||
wg.Wait()
|
||||
}
|
||||
}
|
||||
|
||||
// TestDialSSHHandshakeTimeout covers a peer that accepts the TCP connection but
|
||||
// never sends an SSH banner. Without a handshake deadline the dial blocks the
|
||||
// updater forever (GHSA-h9jh-29rh-w464).
|
||||
func TestDialSSHHandshakeTimeout(t *testing.T) {
|
||||
prev := sshHandshakeTimeout
|
||||
sshHandshakeTimeout = 200 * time.Millisecond
|
||||
t.Cleanup(func() { sshHandshakeTimeout = prev })
|
||||
|
||||
ln, err := net.Listen("tcp", "127.0.0.1:0")
|
||||
require.NoError(t, err)
|
||||
defer ln.Close()
|
||||
|
||||
accepted := make(chan net.Conn, 1)
|
||||
go func() {
|
||||
conn, err := ln.Accept()
|
||||
if err == nil {
|
||||
accepted <- conn
|
||||
}
|
||||
}()
|
||||
|
||||
config := &ssh.ClientConfig{
|
||||
User: "u",
|
||||
HostKeyCallback: ssh.InsecureIgnoreHostKey(),
|
||||
Timeout: 4 * time.Second,
|
||||
}
|
||||
|
||||
done := make(chan error, 1)
|
||||
go func() {
|
||||
client, err := dialSSHWithKeepAlive("tcp", ln.Addr().String(), config)
|
||||
if client != nil {
|
||||
client.Close()
|
||||
}
|
||||
done <- err
|
||||
}()
|
||||
|
||||
select {
|
||||
case err := <-done:
|
||||
assert.Error(t, err, "a silent peer must fail the handshake")
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("dial blocked on a peer that never sends an SSH banner")
|
||||
}
|
||||
|
||||
// the hub must close its side of the connection
|
||||
conn := <-accepted
|
||||
defer conn.Close()
|
||||
_ = conn.SetReadDeadline(time.Now().Add(2 * time.Second))
|
||||
_, err = conn.Read(make([]byte, 256))
|
||||
for err == nil {
|
||||
_, err = conn.Read(make([]byte, 256))
|
||||
}
|
||||
var netErr net.Error
|
||||
assert.False(t, errors.As(err, &netErr) && netErr.Timeout(), "hub should close the connection, got %v", err)
|
||||
}
|
||||
|
||||
// TestDialSSHClearsHandshakeDeadline ensures the handshake deadline does not
|
||||
// carry over to the established connection, which is reused for many updates.
|
||||
func TestDialSSHClearsHandshakeDeadline(t *testing.T) {
|
||||
prev := sshHandshakeTimeout
|
||||
sshHandshakeTimeout = 200 * time.Millisecond
|
||||
t.Cleanup(func() { sshHandshakeTimeout = prev })
|
||||
|
||||
_, hostPriv, err := ed25519.GenerateKey(rand.Reader)
|
||||
require.NoError(t, err)
|
||||
hostSigner, err := ssh.NewSignerFromKey(hostPriv)
|
||||
require.NoError(t, err)
|
||||
_, clientPriv, err := ed25519.GenerateKey(rand.Reader)
|
||||
require.NoError(t, err)
|
||||
clientSigner, err := ssh.NewSignerFromKey(clientPriv)
|
||||
require.NoError(t, err)
|
||||
|
||||
serverConfig := &ssh.ServerConfig{
|
||||
PublicKeyCallback: func(ssh.ConnMetadata, ssh.PublicKey) (*ssh.Permissions, error) { return nil, nil },
|
||||
}
|
||||
serverConfig.AddHostKey(hostSigner)
|
||||
|
||||
ln, err := net.Listen("tcp", "127.0.0.1:0")
|
||||
require.NoError(t, err)
|
||||
defer ln.Close()
|
||||
|
||||
go func() {
|
||||
conn, err := ln.Accept()
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
defer conn.Close()
|
||||
_, chans, reqs, err := ssh.NewServerConn(conn, serverConfig)
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
go ssh.DiscardRequests(reqs)
|
||||
for newChan := range chans {
|
||||
ch, chReqs, err := newChan.Accept()
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
go ssh.DiscardRequests(chReqs)
|
||||
ch.Close()
|
||||
}
|
||||
}()
|
||||
|
||||
config := &ssh.ClientConfig{
|
||||
User: "u",
|
||||
Auth: []ssh.AuthMethod{ssh.PublicKeys(clientSigner)},
|
||||
HostKeyCallback: ssh.InsecureIgnoreHostKey(),
|
||||
Timeout: 4 * time.Second,
|
||||
}
|
||||
client, err := dialSSHWithKeepAlive("tcp", ln.Addr().String(), config)
|
||||
require.NoError(t, err)
|
||||
defer client.Close()
|
||||
|
||||
// wait past the handshake deadline; the connection must still be usable
|
||||
time.Sleep(3 * sshHandshakeTimeout)
|
||||
session, err := client.NewSession()
|
||||
require.NoError(t, err, "connection should outlive the handshake deadline")
|
||||
session.Close()
|
||||
}
|
||||
|
||||
@@ -992,6 +992,12 @@ func (s *System) createSSHClient() error {
|
||||
// per-operation timeout in runSSHOperation instead (see issue #2041).
|
||||
const sshKeepAliveInterval = 30 * time.Second
|
||||
|
||||
// sshHandshakeTimeout bounds the SSH handshake after the TCP connection is
|
||||
// established. ssh.ClientConfig.Timeout only covers the TCP connect, so a peer
|
||||
// that accepts the connection but never sends an SSH banner would otherwise
|
||||
// block the updater forever.
|
||||
var sshHandshakeTimeout = 10 * time.Second
|
||||
|
||||
// dialSSHWithKeepAlive dials an SSH connection like ssh.Dial, but enables TCP
|
||||
// keep-alive on the underlying connection so half-open connections are
|
||||
// eventually detected by the operating system.
|
||||
@@ -1004,11 +1010,14 @@ func dialSSHWithKeepAlive(network, addr string, config *ssh.ClientConfig) (*ssh.
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
_ = conn.SetDeadline(time.Now().Add(sshHandshakeTimeout))
|
||||
sshConn, chans, reqs, err := ssh.NewClientConn(conn, addr, config)
|
||||
if err != nil {
|
||||
_ = conn.Close()
|
||||
return nil, err
|
||||
}
|
||||
// clear the handshake deadline so it doesn't apply to the long-lived connection
|
||||
_ = conn.SetDeadline(time.Time{})
|
||||
return ssh.NewClient(sshConn, chans, reqs), nil
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user