diff --git a/client/vnc/server/agent_darwin.go b/client/vnc/server/agent_darwin.go index 13e06e66d..2c7abfd3e 100644 --- a/client/vnc/server/agent_darwin.go +++ b/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- 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 { diff --git a/client/vnc/server/agent_darwin_test.go b/client/vnc/server/agent_darwin_test.go new file mode 100644 index 000000000..14fa2287d --- /dev/null +++ b/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- +// 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)) + }) + } +} diff --git a/client/vnc/server/agent_handshake.go b/client/vnc/server/agent_handshake.go new file mode 100644 index 000000000..4188f93e9 --- /dev/null +++ b/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} +} diff --git a/client/vnc/server/agent_handshake_test.go b/client/vnc/server/agent_handshake_test.go new file mode 100644 index 000000000..33677e2cc --- /dev/null +++ b/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 +} diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index 715fdd4db..594c8ca96 100644 --- a/client/vnc/server/agent_ipc.go +++ b/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. diff --git a/client/vnc/server/handshake.go b/client/vnc/server/handshake.go index 1f23688e1..f7a82061d 100644 --- a/client/vnc/server/handshake.go +++ b/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. diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index e9229e20d..e64166fc9 100644 --- a/client/vnc/server/server_test.go +++ b/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