Randomize WireGuard TCP reconnect ports

client/cli/connect_spinner.go

@@ -25,12 +25,12 @@ type connectResult struct {
 }
 
 func connectWithSpinner(out io.Writer, target string, connect func(transport.ConnectStatusFn) (rpcpb.SliverRPCClient, *grpc.ClientConn, error)) (rpcpb.SliverRPCClient, *grpc.ClientConn, error) {
-	statusCh := make(chan string, 1)
+	statusCh := make(chan string, 8)
 	resultCh := make(chan connectResult, 1)
 
 	go func() {
 		rpc, conn, err := connect(func(status string) {
-			pushLatestStatus(statusCh, status)
+			sendStatus(statusCh, status)
 		})
 		resultCh <- connectResult{rpc: rpc, conn: conn, err: err}
 	}()
@@ -132,21 +132,12 @@ func connectWithSpinner(out io.Writer, target string, connect func(transport.Con
 	}
 }
 
-func pushLatestStatus(statusCh chan string, status string) {
+func sendStatus(statusCh chan string, status string) {
 	status = strings.TrimSpace(status)
 	if status == "" {
 		return
 	}
-
-	select {
-	case statusCh <- status:
-	default:
-		select {
-		case <-statusCh:
-		default:
-		}
-		statusCh <- status
-	}
+	statusCh <- status
 }
 
 func formatConnectSpinnerMessage(target string, status string) string {

client/transport/connect_mode.go

@@ -64,9 +64,9 @@ func CloseGRPCConnection(conn *grpc.ClientConn) error {
 	}
 
 	var errs []error
+	errs = append(errs, conn.Close())
 	if closer := unregisterConnCloser(conn); closer != nil {
 		errs = append(errs, closer.Close())
 	}
-	errs = append(errs, conn.Close())
 	return errors.Join(errs...)
 }

client/transport/mtls_test.go

@@ -5,6 +5,9 @@ import (
 	"testing"
 
 	"github.com/bishopfox/sliver/client/assets"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/connectivity"
+	"google.golang.org/grpc/credentials/insecure"
 )
 
 func TestSelectMultiplayerDialStrategyLegacyConfigUsesDirectMTLS(t *testing.T) {
@@ -78,3 +81,29 @@ func setTestMultiplayerConnectMode(t *testing.T, mode MultiplayerConnectMode) {
 		SetMultiplayerConnectMode(previous)
 	})
 }
+
+func TestCloseGRPCConnectionClosesConnBeforeTransportCloser(t *testing.T) {
+	conn, err := grpc.NewClient("passthrough:///sliver-test", grpc.WithTransportCredentials(insecure.NewCredentials()))
+	if err != nil {
+		t.Fatalf("create grpc client: %v", err)
+	}
+
+	var stateSeen connectivity.State
+	registerConnCloser(conn, testConnectionCloser(func() error {
+		stateSeen = conn.GetState()
+		return nil
+	}))
+
+	if err := CloseGRPCConnection(conn); err != nil {
+		t.Fatalf("close grpc connection: %v", err)
+	}
+	if stateSeen != connectivity.Shutdown {
+		t.Fatalf("expected transport closer to run after grpc shutdown, got state %v", stateSeen)
+	}
+}
+
+type testConnectionCloser func() error
+
+func (fn testConnectionCloser) Close() error {
+	return fn()
+}

client/transport/wireguard.go

@@ -47,6 +47,7 @@ func (t *wireGuardTunnel) Close() error {
 	t.closeOnce.Do(func() {
 		if t.dev != nil {
 			t.dev.Close()
+			<-t.dev.Wait()
 		}
 	})
 	return nil
@@ -60,31 +61,6 @@ func (t *wireGuardTunnel) DialContext(ctx context.Context, address string) (net.
 }
 
 func wireGuardMTLSConnect(config *assets.ClientConfig, statusFn ConnectStatusFn) (rpcpb.SliverRPCClient, *grpc.ClientConn, error) {
-	notifyConnectStatus(statusFn, connectStatusWireGuard)
-	tunnel, target, err := newWireGuardTunnel(config)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	options, err := newMTLSDialOptions(config)
-	if err != nil {
-		_ = tunnel.Close()
-		return nil, nil, err
-	}
-	options = append(options, grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
-		return tunnel.DialContext(ctx, addr)
-	}))
-
-	notifyConnectStatus(statusFn, connectStatusGRPCMTLSOverWireGuard)
-	rpcClient, conn, err := dialWireGuardRPCClient(target, options, tunnel)
-	if err != nil {
-		_ = tunnel.Close()
-		return nil, nil, err
-	}
-	return rpcClient, conn, nil
-}
-
-func dialWireGuardRPCClient(target string, options []grpc.DialOption, tunnel connectionCloser) (rpcpb.SliverRPCClient, *grpc.ClientConn, error) {
 	deadline := time.Now().Add(multiplayerWireGuardDialTimeout)
 	var lastErr error
 	attempts := 0
@@ -92,12 +68,29 @@ func dialWireGuardRPCClient(target string, options []grpc.DialOption, tunnel con
 	for {
 		attempts++
 
+		notifyConnectStatus(statusFn, connectStatusWireGuard)
+		tunnel, target, err := newWireGuardTunnel(config)
+		if err != nil {
+			return nil, nil, err
+		}
+
+		options, err := newMTLSDialOptions(config)
+		if err != nil {
+			_ = tunnel.Close()
+			return nil, nil, err
+		}
+		options = append(options, grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
+			return tunnel.DialContext(ctx, addr)
+		}))
+
+		notifyConnectStatus(statusFn, connectStatusGRPCMTLSOverWireGuard)
 		rpcClient, conn, err := dialRPCClient(target, options, nil)
 		if err == nil {
 			registerConnCloser(conn, tunnel)
 			return rpcClient, conn, nil
 		}
 
+		_ = tunnel.Close()
 		lastErr = err
 		if !errors.Is(err, context.DeadlineExceeded) || !time.Now().Before(deadline) {
 			break

client/transport/wireguard_netstack.go

@@ -3,6 +3,7 @@ package transport
 import (
 	"context"
 	"crypto/rand"
+	"errors"
 	"fmt"
 	"math/big"
 	insecurerand "math/rand"
@@ -32,6 +33,7 @@ type transportTun struct {
 	notifyHandle   *channel.NotificationHandle
 	incomingPacket chan *buffer.View
 	mtu            int
+	primaryAddr    netip.Addr
 	hasV4          bool
 	hasV6          bool
 }
@@ -41,6 +43,8 @@ type transportNet transportTun
 const (
 	transportTCPReceiveBufferMax = 8 << 20
 	transportTCPSendBufferMax    = 6 << 20
+	transportTCPBindPortMin      = 49152
+	transportTCPBindPortSpan     = 65535 - transportTCPBindPortMin + 1
 )
 
 func createTransportNetTUN(localAddresses []netip.Addr, mtu int) (tun.Device, *transportNet, error) {
@@ -88,6 +92,9 @@ func createTransportNetTUN(localAddresses []netip.Addr, mtu int) (tun.Device, *t
 		if tcpipErr := dev.stack.AddProtocolAddress(1, protoAddr, stack.AddressProperties{}); tcpipErr != nil {
 			return nil, nil, fmt.Errorf("AddProtocolAddress(%v): %v", ip, tcpipErr)
 		}
+		if !dev.primaryAddr.IsValid() {
+			dev.primaryAddr = ip
+		}
 		if ip.Is4() {
 			dev.hasV4 = true
 		} else if ip.Is6() {
@@ -240,8 +247,12 @@ func convertTransportFullAddr(endpoint netip.AddrPort) (tcpip.FullAddress, tcpip
 }
 
 func (netstack *transportNet) DialContextTCPAddrPort(ctx context.Context, addr netip.AddrPort) (*gonet.TCPConn, error) {
-	fa, pn := convertTransportFullAddr(addr)
-	return gonet.DialContextTCP(ctx, netstack.stack, fa, pn)
+	remoteAddr, pn := convertTransportFullAddr(addr)
+	localAddr, err := netstack.randomTCPBindAddr()
+	if err != nil {
+		return nil, err
+	}
+	return gonet.DialTCPWithBind(ctx, netstack.stack, localAddr, remoteAddr, pn)
 }
 
 func (netstack *transportNet) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
@@ -265,3 +276,28 @@ func (netstack *transportNet) DialContext(ctx context.Context, network, address
 	}
 	return netstack.DialContextTCPAddrPort(ctx, netip.AddrPortFrom(addr, uint16(port)))
 }
+
+func (netstack *transportNet) randomTCPBindAddr() (tcpip.FullAddress, error) {
+	if netstack == nil || !netstack.primaryAddr.IsValid() {
+		return tcpip.FullAddress{}, errors.New("wireguard transport has no local address")
+	}
+
+	port, err := randomTransportTCPBindPort()
+	if err != nil {
+		return tcpip.FullAddress{}, err
+	}
+
+	return tcpip.FullAddress{
+		NIC:  1,
+		Addr: tcpip.AddrFromSlice(netstack.primaryAddr.AsSlice()),
+		Port: port,
+	}, nil
+}
+
+func randomTransportTCPBindPort() (uint16, error) {
+	n, err := rand.Int(rand.Reader, big.NewInt(transportTCPBindPortSpan))
+	if err != nil {
+		return 0, fmt.Errorf("failed to choose random wireguard TCP bind port: %w", err)
+	}
+	return uint16(transportTCPBindPortMin + n.Int64()), nil
+}

client/transport/wireguard_netstack_test.go

@@ -0,0 +1,45 @@
+package transport
+
+import (
+	"net/netip"
+	"testing"
+)
+
+func TestRandomTransportTCPBindPortUsesEphemeralRange(t *testing.T) {
+	for attempt := 0; attempt < 16; attempt++ {
+		port, err := randomTransportTCPBindPort()
+		if err != nil {
+			t.Fatalf("choose random tcp bind port: %v", err)
+		}
+		if port < transportTCPBindPortMin {
+			t.Fatalf("expected bind port >= %d, got %d", transportTCPBindPortMin, port)
+		}
+		if 65535 < port {
+			t.Fatalf("expected bind port <= 65535, got %d", port)
+		}
+	}
+}
+
+func TestRandomTCPBindAddrUsesPrimaryTransportAddress(t *testing.T) {
+	netstack := &transportNet{
+		primaryAddr: netip.MustParseAddr("100.65.0.2"),
+	}
+
+	localAddr, err := netstack.randomTCPBindAddr()
+	if err != nil {
+		t.Fatalf("random tcp bind addr: %v", err)
+	}
+	if localAddr.NIC != 1 {
+		t.Fatalf("expected local bind NIC 1, got %d", localAddr.NIC)
+	}
+	got, ok := netip.AddrFromSlice(localAddr.Addr.AsSlice())
+	if !ok {
+		t.Fatal("expected local bind address to be parseable")
+	}
+	if got != netstack.primaryAddr {
+		t.Fatalf("expected local bind IP %s, got %s", netstack.primaryAddr, got)
+	}
+	if localAddr.Port < transportTCPBindPortMin {
+		t.Fatalf("expected local bind port >= %d, got %d", transportTCPBindPortMin, localAddr.Port)
+	}
+}

server/certs/certs.go

@@ -64,18 +64,30 @@ func saveCertificate(caType string, keyType string, commonName string, cert []by
 
 	certsLog.Infof("Saving certificate for cn = '%s'", commonName)
 
-	certModel := &models.Certificate{
-		CommonName:     commonName,
-		CAType:         caType,
-		KeyType:        keyType,
-		CertificatePEM: string(cert),
-		PrivateKeyPEM:  string(key),
+	dbSession := db.Session()
+	certModel := &models.Certificate{}
+	result := dbSession.Where(&models.Certificate{
+		CommonName: commonName,
+		CAType:     caType,
+		KeyType:    keyType,
+	}).First(certModel)
+	if errors.Is(result.Error, db.ErrRecordNotFound) {
+		certModel = &models.Certificate{
+			CommonName:     commonName,
+			CAType:         caType,
+			KeyType:        keyType,
+			CertificatePEM: string(cert),
+			PrivateKeyPEM:  string(key),
+		}
+		return dbSession.Create(certModel).Error
+	}
+	if result.Error != nil {
+		return result.Error
 	}
 
-	dbSession := db.Session()
-	result := dbSession.Create(&certModel)
-
-	return result.Error
+	certModel.CertificatePEM = string(cert)
+	certModel.PrivateKeyPEM = string(key)
+	return dbSession.Save(certModel).Error
 }
 
 // GetECCCertificate - Get an ECC certificate

server/certs/certs_test.go

@@ -42,3 +42,28 @@ func TestOperatorGenerateCertificate(t *testing.T) {
 		return
 	}
 }
+
+func TestOperatorServerGenerateCertificateReplacesExistingCertificate(t *testing.T) {
+	GenerateCertificateAuthority(OperatorCA, "")
+
+	cert1, key1, err := OperatorServerGenerateCertificate("multiplayer")
+	if err != nil {
+		t.Fatalf("generate first operator server certificate: %v", err)
+	}
+
+	cert2, key2, err := OperatorServerGenerateCertificate("multiplayer")
+	if err != nil {
+		t.Fatalf("generate replacement operator server certificate: %v", err)
+	}
+
+	storedCert, storedKey, err := OperatorServerGetCertificate("multiplayer")
+	if err != nil {
+		t.Fatalf("get operator server certificate: %v", err)
+	}
+	if !bytes.Equal(storedCert, cert2) || !bytes.Equal(storedKey, key2) {
+		t.Fatalf("expected latest operator server certificate to be stored")
+	}
+	if bytes.Equal(cert1, cert2) && bytes.Equal(key1, key2) {
+		t.Fatalf("expected regenerated operator server certificate to differ from original")
+	}
+}

server/console/console-admin_wireguard_test.go

@@ -177,6 +177,64 @@ func TestNewOperatorConfigWithWireGuardConnectsToWrappedMultiplayerRepeatedly(t
 	}
 }
 
+func TestWrappedMultiplayerWireGuardListenerRestartsCleanly(t *testing.T) {
+	certs.SetupCAs()
+	certs.SetupWGKeys()
+	certs.SetupMultiplayerWGKeys()
+	clienttransport.SetMultiplayerConnectMode(clienttransport.MultiplayerConnectAuto)
+
+	operatorName := uniqueKickOperatorName(t)
+	t.Cleanup(func() {
+		_ = removeOperator(operatorName)
+		_ = revokeOperatorClientCertificate(operatorName)
+		closeOperatorStreams(operatorName)
+	})
+
+	port := freeUDPPort(t)
+	configJSON, err := NewOperatorConfig(operatorName, "127.0.0.1", uint16(port), []string{"all"}, true)
+	if err != nil {
+		t.Fatalf("generate wireguard operator config: %v", err)
+	}
+
+	config := &clientassets.ClientConfig{}
+	if err := json.Unmarshal(configJSON, config); err != nil {
+		t.Fatalf("parse operator config: %v", err)
+	}
+
+	for attempt := 1; attempt <= 3; attempt++ {
+		grpcServer, ln, err := servertransport.StartWGWrappedMtlsClientListener("127.0.0.1", uint16(port))
+		if err != nil {
+			if strings.Contains(err.Error(), "operation not permitted") {
+				t.Skipf("wireguard listener bind not permitted in this environment: %v", err)
+			}
+			t.Fatalf("attempt %d: start wrapped multiplayer listener: %v", attempt, err)
+		}
+
+		rpcClient, conn, err := clienttransport.MTLSConnect(config)
+		if err != nil {
+			grpcServer.Stop()
+			_ = ln.Close()
+			t.Fatalf("attempt %d: connect operator through restarted wireguard wrapper: %v", attempt, err)
+		}
+		if _, err := rpcClient.GetVersion(context.Background(), &commonpb.Empty{}); err != nil {
+			_ = clienttransport.CloseGRPCConnection(conn)
+			grpcServer.Stop()
+			_ = ln.Close()
+			t.Fatalf("attempt %d: GetVersion over restarted wrapped multiplayer failed: %v", attempt, err)
+		}
+		if err := clienttransport.CloseGRPCConnection(conn); err != nil {
+			grpcServer.Stop()
+			_ = ln.Close()
+			t.Fatalf("attempt %d: close wrapped multiplayer connection: %v", attempt, err)
+		}
+
+		grpcServer.Stop()
+		if err := ln.Close(); err != nil {
+			t.Fatalf("attempt %d: close wrapped multiplayer listener: %v", attempt, err)
+		}
+	}
+}
+
 func TestWrappedMultiplayerWireGuardSupportsUnaryRPCsWithDedicatedCommandConnection(t *testing.T) {
 	t.Run("events-only", func(t *testing.T) {
 		runWrappedMultiplayerWireGuardUnaryWithBackgroundStreams(t, true, false)

server/transport/mtls.go

@@ -21,10 +21,12 @@ package transport
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"net"
 	"runtime/debug"
+	"strings"
 
 	"github.com/bishopfox/sliver/protobuf/rpcpb"
 	"github.com/bishopfox/sliver/server/certs"
@@ -89,21 +91,33 @@ func StartMtlsClientServer(ln net.Listener) (*grpc.Server, error) {
 	grpcServer := grpc.NewServer(options...)
 	rpcpb.RegisterSliverRPCServer(grpcServer, rpc.NewServer())
 	go func() {
-		panicked := true
 		defer func() {
-			if panicked {
-				mtlsLog.Errorf("stacktrace from panic: %s", string(debug.Stack()))
+			if r := recover(); r != nil {
+				mtlsLog.Errorf("gRPC server panic: %v\n%s", r, string(debug.Stack()))
 			}
 		}()
-		if err := grpcServer.Serve(ln); err != nil {
+		if err := grpcServer.Serve(ln); err != nil && !isExpectedGRPCServerExit(err) {
 			mtlsLog.Warnf("gRPC server exited with error: %v", err)
-		} else {
-			panicked = false
 		}
 	}()
 	return grpcServer, nil
 }
 
+func isExpectedGRPCServerExit(err error) bool {
+	if err == nil {
+		return true
+	}
+	if errors.Is(err, grpc.ErrServerStopped) || errors.Is(err, net.ErrClosed) {
+		return true
+	}
+
+	// The gVisor-backed listener used by the WireGuard multiplayer transport can
+	// surface this accept error during normal shutdown on Windows.
+	errString := err.Error()
+	return strings.Contains(errString, "use of closed network connection") ||
+		strings.Contains(errString, "endpoint is in invalid state")
+}
+
 // getOperatorServerTLSConfig - Generate the TLS configuration, we do now allow the end user
 // to specify any TLS parameters, we choose sensible defaults instead
 func getOperatorServerTLSConfig(host string) *tls.Config {
@@ -114,12 +128,7 @@ func getOperatorServerTLSConfig(host string) *tls.Config {
 	caCertPool := x509.NewCertPool()
 	caCertPool.AddCert(caCertPtr)
 
-	_, _, err = certs.OperatorServerGetCertificate(host)
-	if err == certs.ErrCertDoesNotExist {
-		certs.OperatorServerGenerateCertificate(host)
-	}
-
-	certPEM, keyPEM, err := certs.OperatorServerGetCertificate(host)
+	certPEM, keyPEM, err := ensureCurrentOperatorServerCertificate(host, caCertPtr)
 	if err != nil {
 		mtlsLog.Errorf("Failed to generate or fetch certificate %s", err)
 		return nil
@@ -142,3 +151,35 @@ func getOperatorServerTLSConfig(host string) *tls.Config {
 
 	return tlsConfig
 }
+
+func ensureCurrentOperatorServerCertificate(host string, caCert *x509.Certificate) ([]byte, []byte, error) {
+	certPEM, keyPEM, err := certs.OperatorServerGetCertificate(host)
+	if err != nil && !errors.Is(err, certs.ErrCertDoesNotExist) {
+		return nil, nil, err
+	}
+	if errors.Is(err, certs.ErrCertDoesNotExist) || !operatorServerCertMatchesCA(certPEM, caCert) {
+		if _, _, genErr := certs.OperatorServerGenerateCertificate(host); genErr != nil {
+			return nil, nil, genErr
+		}
+		return certs.OperatorServerGetCertificate(host)
+	}
+	return certPEM, keyPEM, nil
+}
+
+func operatorServerCertMatchesCA(certPEM []byte, caCert *x509.Certificate) bool {
+	if len(certPEM) == 0 || caCert == nil {
+		return false
+	}
+	block, _ := pem.Decode(certPEM)
+	if block == nil {
+		return false
+	}
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		return false
+	}
+	roots := x509.NewCertPool()
+	roots.AddCert(caCert)
+	_, err = cert.Verify(x509.VerifyOptions{Roots: roots})
+	return err == nil
+}

server/transport/multiplayer_wireguard.go

@@ -48,6 +48,7 @@ func (l *wireGuardWrappedClientListener) Close() error {
 		}
 		if l.dev != nil {
 			l.dev.Close()
+			<-l.dev.Wait()
 		}
 	})
 	return err