Authenticate the daemon and its VNC agent to each other without sending the token

client/vnc/server/agent_darwin.go

@@ -208,9 +208,26 @@ func ensureAgentSocketParent(parent string) error {
 	if st, ok := info.Sys().(*syscall.Stat_t); ok && st.Uid != 0 {
 		return fmt.Errorf("%s not owned by root (uid=%d)", parent, st.Uid)
 	}
+	if writableByOthers(info.Mode()) {
+		return fmt.Errorf("%s is writable beyond its owner (mode %#o) and not sticky", parent, info.Mode().Perm())
+	}
 	return nil
 }
 
+// writableByOthers reports whether mode lets anyone but the owner create
+// entries in a directory.
+//
+// Root ownership alone is not enough for the agent's runtime parent: a
+// root-owned directory that group or other may write to lets an unprivileged
+// process create vnc-<uid> in the window between the stale-subdir check and the
+// Mkdir that follows, and the daemon would then hand its per-spawn token to
+// whatever is listening there. The sticky bit closes that, since only an
+// entry's owner may replace it, so a /tmp-style parent stays acceptable.
+func writableByOthers(mode os.FileMode) bool {
+	const groupOtherWrite = 0o022
+	return mode.Perm()&groupOtherWrite != 0 && mode&os.ModeSticky == 0
+}
+
 // purgeStaleAgentSubdir removes a leftover subdir unless it is a real dir
 // owned by uid with mode 0700. Lstat (not Stat) so a symlink is detected.
 func purgeStaleAgentSubdir(subdir string, uid uint32) error {

client/vnc/server/agent_darwin_test.go

@@ -0,0 +1,37 @@
+//go:build darwin && !ios
+
+package server
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// The agent's runtime parent has to be writable by its owner alone, or sticky.
+// Anything else lets an unprivileged process win the race to create vnc-<uid>
+// and receive the daemon's per-spawn token.
+func TestWritableByOthers(t *testing.T) {
+	tests := []struct {
+		name string
+		mode os.FileMode
+		want bool
+	}{
+		{name: "owner only", mode: 0o700},
+		{name: "owner writes, others read and traverse", mode: 0o755},
+		{name: "group writable", mode: 0o770, want: true},
+		{name: "world writable", mode: 0o707, want: true},
+		{name: "world writable and sticky, as /tmp", mode: 0o777 | os.ModeSticky},
+		{name: "group writable and sticky", mode: 0o770 | os.ModeSticky},
+		// Set-uid and set-gid share the bit range with sticky in FileMode, so
+		// they must not be mistaken for it.
+		{name: "world writable and setgid", mode: 0o777 | os.ModeSetgid, want: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, writableByOthers(tt.mode))
+		})
+	}
+}

client/vnc/server/agent_handshake.go

@@ -0,0 +1,154 @@
+//go:build !js && !ios && !android
+
+package server
+
+import (
+	"crypto/hmac"
+	"crypto/rand"
+	"crypto/sha256"
+	"crypto/subtle"
+	"fmt"
+	"io"
+	"net"
+	"time"
+
+	log "github.com/sirupsen/logrus"
+)
+
+// The daemon and its per-session agent authenticate each other with the
+// per-spawn token, over a challenge-response rather than by sending the token
+// itself.
+//
+// Sending it was enough to prove the daemon's side, but it also handed the
+// secret to whatever was listening. The socket lives in a directory only the
+// console user and root may write to, so an impostor has to already be running
+// as that user — but such a process could then take the token, answer as the
+// agent, and sit between an authorized remote peer and the desktop, watching
+// what they see and type. Neither end reveals the token now, and each refuses to
+// continue until the other has proved it holds the same one.
+//
+// Both ends are the same binary: the daemon spawns the agent from its own
+// executable, so there is no version skew between them to keep compatible.
+const (
+	// agentTokenLen is the size of the random per-spawn token in bytes. It is
+	// the HMAC key both halves below are keyed on.
+	agentTokenLen = 32
+
+	// agentNonceLen is the size of each side's challenge.
+	agentNonceLen = 32
+	// agentMACLen is the size of an HMAC-SHA256 tag.
+	agentMACLen = sha256.Size
+	// agentHandshakeTimeout bounds the whole exchange. Both ends are local
+	// processes, so this only has to cover scheduling, never a network.
+	agentHandshakeTimeout = 5 * time.Second
+)
+
+// Domain separation, so a tag one side produces can never be replayed as the
+// other side's answer.
+var (
+	agentDaemonLabel = []byte("netbird-vnc-daemon")
+	agentAgentLabel  = []byte("netbird-vnc-agent")
+)
+
+// agentMAC tags the label and the parts under a token.
+func agentMAC(token, label []byte, parts ...[]byte) []byte {
+	mac := hmac.New(sha256.New, token)
+	mac.Write(label)
+	for _, p := range parts {
+		mac.Write(p)
+	}
+	return mac.Sum(nil)
+}
+
+// agentClientHandshake runs the daemon's half against a freshly dialled agent
+// connection: read the agent's challenge, answer it, then challenge the agent
+// back and check its answer before any session bytes are proxied.
+//
+// viewOnly travels inside the daemon's tag, so an impostor cannot flip a
+// read-only session into a controlling one by rewriting the byte in flight.
+func agentClientHandshake(conn net.Conn, token []byte, viewOnly bool) error {
+	if err := conn.SetDeadline(time.Now().Add(agentHandshakeTimeout)); err != nil {
+		return fmt.Errorf("set handshake deadline: %w", err)
+	}
+	defer func() {
+		if err := conn.SetDeadline(time.Time{}); err != nil {
+			log.Debugf("clear agent handshake deadline: %v", err)
+		}
+	}()
+
+	agentNonce := make([]byte, agentNonceLen)
+	if _, err := io.ReadFull(conn, agentNonce); err != nil {
+		return fmt.Errorf("read agent challenge: %w", err)
+	}
+
+	daemonNonce := make([]byte, agentNonceLen)
+	if _, err := rand.Read(daemonNonce); err != nil {
+		return fmt.Errorf("read random: %w", err)
+	}
+
+	flag := viewOnlyByte(viewOnly)
+	reply := make([]byte, 0, agentMACLen+agentNonceLen+1)
+	reply = append(reply, agentMAC(token, agentDaemonLabel, agentNonce, flag)...)
+	reply = append(reply, daemonNonce...)
+	reply = append(reply, flag...)
+	if _, err := conn.Write(reply); err != nil {
+		return fmt.Errorf("send handshake response: %w", err)
+	}
+
+	agentTag := make([]byte, agentMACLen)
+	if _, err := io.ReadFull(conn, agentTag); err != nil {
+		return fmt.Errorf("read agent response: %w", err)
+	}
+	want := agentMAC(token, agentAgentLabel, daemonNonce)
+	if subtle.ConstantTimeCompare(agentTag, want) != 1 {
+		return fmt.Errorf("agent did not prove it holds the session token")
+	}
+	return nil
+}
+
+// agentServerHandshake runs the agent's half against an accepted connection,
+// returning the view-only flag the daemon authenticated.
+func agentServerHandshake(conn net.Conn, token []byte) (bool, error) {
+	if err := conn.SetDeadline(time.Now().Add(agentHandshakeTimeout)); err != nil {
+		return false, fmt.Errorf("set handshake deadline: %w", err)
+	}
+	defer func() {
+		if err := conn.SetDeadline(time.Time{}); err != nil {
+			log.Debugf("clear agent handshake deadline: %v", err)
+		}
+	}()
+
+	agentNonce := make([]byte, agentNonceLen)
+	if _, err := rand.Read(agentNonce); err != nil {
+		return false, fmt.Errorf("read random: %w", err)
+	}
+	if _, err := conn.Write(agentNonce); err != nil {
+		return false, fmt.Errorf("send challenge: %w", err)
+	}
+
+	buf := make([]byte, agentMACLen+agentNonceLen+1)
+	if _, err := io.ReadFull(conn, buf); err != nil {
+		return false, fmt.Errorf("read daemon response: %w", err)
+	}
+	daemonTag := buf[:agentMACLen]
+	daemonNonce := buf[agentMACLen : agentMACLen+agentNonceLen]
+	flag := buf[agentMACLen+agentNonceLen:]
+
+	want := agentMAC(token, agentDaemonLabel, agentNonce, flag)
+	if subtle.ConstantTimeCompare(daemonTag, want) != 1 {
+		return false, fmt.Errorf("caller did not prove it holds the session token")
+	}
+
+	if _, err := conn.Write(agentMAC(token, agentAgentLabel, daemonNonce)); err != nil {
+		return false, fmt.Errorf("send response: %w", err)
+	}
+	return flag[0] != 0, nil
+}
+
+// viewOnlyByte renders the flag as the single byte both tags cover.
+func viewOnlyByte(viewOnly bool) []byte {
+	if viewOnly {
+		return []byte{1}
+	}
+	return []byte{0}
+}

client/vnc/server/agent_handshake_test.go

@@ -0,0 +1,127 @@
+//go:build !js && !ios && !android
+
+package server
+
+import (
+	"bytes"
+	"net"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// runHandshake drives both halves over an in-memory pipe and returns what each
+// side concluded.
+func runHandshake(t *testing.T, daemonToken, agentToken []byte, viewOnly bool) (daemonErr error, gotViewOnly bool, agentErr error) {
+	t.Helper()
+
+	daemonSide, agentSide := net.Pipe()
+	t.Cleanup(func() {
+		_ = daemonSide.Close()
+		_ = agentSide.Close()
+	})
+
+	type agentResult struct {
+		viewOnly bool
+		err      error
+	}
+	agentDone := make(chan agentResult, 1)
+	go func() {
+		v, err := agentServerHandshake(agentSide, agentToken)
+		if err != nil {
+			// What the agent's caller does on rejection, so the daemon sees the
+			// close rather than waiting out its own deadline.
+			_ = agentSide.Close()
+		}
+		agentDone <- agentResult{v, err}
+	}()
+
+	daemonErr = agentClientHandshake(daemonSide, daemonToken, viewOnly)
+	res := <-agentDone
+	return daemonErr, res.viewOnly, res.err
+}
+
+func TestAgentHandshake_MatchingTokens(t *testing.T) {
+	token := bytes.Repeat([]byte{0xA5}, agentTokenLen)
+
+	for _, viewOnly := range []bool{false, true} {
+		dErr, gotViewOnly, aErr := runHandshake(t, token, token, viewOnly)
+		require.NoError(t, dErr)
+		require.NoError(t, aErr)
+		assert.Equal(t, viewOnly, gotViewOnly, "the agent must see the flag the daemon authenticated")
+	}
+}
+
+// The point of the exchange: an impostor listening on the socket without the
+// token cannot complete it, and the daemon refuses before proxying anything.
+func TestAgentHandshake_ImpostorAgentIsRefused(t *testing.T) {
+	daemonToken := bytes.Repeat([]byte{0x01}, agentTokenLen)
+	impostorToken := bytes.Repeat([]byte{0x02}, agentTokenLen)
+
+	dErr, _, aErr := runHandshake(t, daemonToken, impostorToken, false)
+	require.Error(t, aErr, "the impostor cannot verify the daemon's tag")
+	require.Error(t, dErr, "the daemon must not proceed against an unproven peer")
+}
+
+// And the other direction: something dialling the agent without the token gets
+// nowhere either.
+func TestAgentHandshake_ImpostorDaemonIsRefused(t *testing.T) {
+	agentToken := bytes.Repeat([]byte{0x03}, agentTokenLen)
+	impostorToken := bytes.Repeat([]byte{0x04}, agentTokenLen)
+
+	_, _, aErr := runHandshake(t, impostorToken, agentToken, false)
+	require.Error(t, aErr)
+	assert.Contains(t, aErr.Error(), "did not prove it holds the session token")
+}
+
+// The token itself must never appear on the wire; that was the whole reason for
+// replacing the plain preamble.
+func TestAgentHandshake_TokenNeverSent(t *testing.T) {
+	token := bytes.Repeat([]byte{0x7E}, agentTokenLen)
+
+	daemonSide, agentSide := net.Pipe()
+	defer daemonSide.Close()
+	defer agentSide.Close()
+
+	// Tee everything the daemon writes so it can be searched afterwards.
+	var sent bytes.Buffer
+	go func() {
+		_, _ = agentServerHandshake(&teeConn{Conn: agentSide, read: &sent}, token)
+	}()
+
+	require.NoError(t, agentClientHandshake(daemonSide, token, false))
+	assert.NotContains(t, sent.Bytes(), token, "the token must not cross the socket")
+}
+
+// A tag is bound to the nonce it answered, so replaying one against a fresh
+// challenge fails.
+func TestAgentMAC_IsBoundToNonceAndLabel(t *testing.T) {
+	token := bytes.Repeat([]byte{0x11}, agentTokenLen)
+	nonceA := bytes.Repeat([]byte{0x22}, agentNonceLen)
+	nonceB := bytes.Repeat([]byte{0x33}, agentNonceLen)
+
+	assert.NotEqual(t,
+		agentMAC(token, agentDaemonLabel, nonceA),
+		agentMAC(token, agentDaemonLabel, nonceB),
+		"a different challenge must produce a different tag")
+
+	assert.NotEqual(t,
+		agentMAC(token, agentDaemonLabel, nonceA),
+		agentMAC(token, agentAgentLabel, nonceA),
+		"the two directions must not share a tag, or one could be replayed as the other")
+}
+
+// teeConn records everything read from the wrapped connection.
+type teeConn struct {
+	net.Conn
+	read *bytes.Buffer
+}
+
+func (c *teeConn) Read(b []byte) (int, error) {
+	n, err := c.Conn.Read(b)
+	if n > 0 {
+		c.read.Write(b[:n])
+	}
+	return n, err
+}

client/vnc/server/agent_ipc.go

@@ -118,9 +118,6 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) {
 }
 
 const (
-	// agentTokenLen is the size of the random per-spawn token in bytes.
-	agentTokenLen = 32
-
 	// agentTokenEnvVar names the environment variable the daemon uses to
 	// hand the per-spawn token to the agent child. Out-of-band channels
 	// like this keep the secret out of the command line, where listings
@@ -172,14 +169,9 @@ func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken st
 		return fmt.Errorf("agent peer validation failed: %w", err)
 	}
 
-	preamble := make([]byte, len(tokenBytes)+1)
-	copy(preamble, tokenBytes)
-	if viewOnly {
-		preamble[len(tokenBytes)] = 1
-	}
-	if _, err := agentConn.Write(preamble); err != nil {
+	if err := agentClientHandshake(agentConn, tokenBytes, viewOnly); err != nil {
 		_ = agentConn.Close()
-		return fmt.Errorf("send auth preamble to agent: %w", err)
+		return fmt.Errorf("agent handshake: %w", err)
 	}
 
 	// Audit: one line per successfully-dispatched daemon→agent preamble.

client/vnc/server/handshake.go

@@ -4,7 +4,6 @@ package server
 
 import (
 	"bytes"
-	"crypto/subtle"
 	"encoding/binary"
 	"encoding/hex"
 	"errors"
@@ -231,33 +230,20 @@ func (s *Server) verifyAgentToken(conn net.Conn, connLog *log.Entry) (bool, bool
 	if len(s.agentToken) == 0 {
 		return true, false
 	}
-	buf := make([]byte, len(s.agentToken)+1)
-	if err := conn.SetReadDeadline(time.Now().Add(5 * time.Second)); err != nil {
-		connLog.Debugf("set agent token deadline: %v", err)
-		conn.Close()
-		return false, false
-	}
-	if _, err := io.ReadFull(conn, buf); err != nil {
+	viewOnly, err := agentServerHandshake(conn, s.agentToken)
+	if err != nil {
 		if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
-			// Connect-then-close probes (port liveness checks) hit this
-			// path on every dial; logging them would just flood the
-			// daemon log without surfacing a real failure.
-			connLog.Tracef("agent auth: read preamble: %v", err)
+			// Connect-then-close probes (the daemon's own readiness check
+			// among them) hit this path on every dial; logging them would
+			// just flood the daemon log without surfacing a real failure.
+			connLog.Tracef("agent auth: %v", err)
 		} else {
-			connLog.Warnf("agent auth: read preamble: %v", err)
+			connLog.Warnf("agent auth: %v", err)
 		}
 		conn.Close()
 		return false, false
 	}
-	if err := conn.SetReadDeadline(time.Time{}); err != nil {
-		connLog.Debugf("clear agent token deadline: %v", err)
-	}
-	if subtle.ConstantTimeCompare(buf[:len(s.agentToken)], s.agentToken) != 1 {
-		connLog.Warn("agent auth: invalid token, rejecting")
-		conn.Close()
-		return false, false
-	}
-	return true, buf[len(s.agentToken)] != 0
+	return true, viewOnly
 }
 
 // authorizeSession runs the Noise_IK handshake when auth is enabled.

client/vnc/server/server_test.go

@@ -3,6 +3,7 @@
 package server
 
 import (
+	"bytes"
 	"context"
 	"encoding/binary"
 	"encoding/hex"
@@ -290,9 +291,10 @@ func TestAgentToken_MismatchClosesConnection(t *testing.T) {
 	defer conn.Close()
 	require.NoError(t, conn.SetDeadline(time.Now().Add(10*time.Second)))
 
-	// Send a wrong token of the right length (8 bytes hex-decoded).
-	if _, err := conn.Write([]byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}); err != nil {
-		// Server may already have closed; either way the read below must EOF.
+	// Answer the agent's challenge with a tag derived from the wrong token.
+	if err := agentClientHandshake(conn, bytes.Repeat([]byte{0xff}, agentTokenLen), false); err != nil {
+		// Expected: the server rejects and closes. The read below confirms it
+		// never reached the greeting.
 		_ = err
 	}
 
@@ -326,8 +328,7 @@ func TestAgentToken_MatchAllowsHandshake(t *testing.T) {
 	defer conn.Close()
 	require.NoError(t, conn.SetDeadline(time.Now().Add(10*time.Second)))
 
-	_, err = conn.Write(token)
-	require.NoError(t, err)
+	require.NoError(t, agentClientHandshake(conn, token, false))
 
 	// Send session header so handleConnection can proceed past readConnectionHeader.
 	header := make([]byte, 11) // ModeAttach + usernameLen=0 + sessionID=0 + width=0 + height=0