feat(ssh-agent): pin the peer process with a pidfd before reading /proc

DESIGN.md

@@ -452,6 +452,14 @@ NODKEY_REASON="apply the nginx fix" ansible-playbook site.yml
      (such as a kernel thread), or is there twice. The client opens the
      directory `/proc/PID` of a process once, and reads its files through
      it, so that they all come from that one process.
+   - A process ID can be used again by a new process. On Linux 6.5 and
+     newer the client gets a pidfd of `ssh` too (`SO_PEERPIDFD`). After it
+     opens `/proc/PID`, it checks with the pidfd that `ssh` still runs. If
+     `ssh` has ended, or the kernel has pidfds but gives none, the request
+     goes without the command lines and the reason, and a line says so.
+     A parent counts only
+     if, after the client opened the parent's directory, its child still
+     names it as its parent. On an older kernel there is no pidfd check.
    - A read of `/proc` can block, such as on a stuck FUSE or NFS file
      system. The client waits for it at most 2 seconds, and runs at most
      4 such reads at the same time. Otherwise the request goes without

THREAT-MODEL.md

@@ -147,8 +147,21 @@ root). This helps against honest mistakes and tricks, not against a
 hostile agent: the client runs on the agent machine.
 For an SSH login, `nodkey ssh-agent` reads the command lines of `ssh` and
 its parents, and the reason, from `/proc` (DESIGN.md section 6). A
-process can change its own command line and environment, and a process ID
-can be used again by a new process. So these are claims too.
+process can change its own command line and environment. So these are
+claims too. A process ID can also be used again by a new process: if `ssh`
+ends before the agent opens `/proc/PID`, the open could land on another
+process (issue #61). So on Linux 6.5 and newer the agent gets a pidfd of
+`ssh` (`SO_PEERPIDFD`), opens `/proc/PID`, and then checks with the pidfd
+that `ssh` still runs; its files are read through that open directory. If
+`ssh` has ended, or the kernel has pidfds but gives none, the request goes without
+the command lines and the reason. A parent counts only if its child still
+names it as its parent after the open of the parent's directory, as a
+process whose parent ends gets another parent. Tests check both: no text
+of a new process with the same ID is sent. On an older kernel the open of
+`/proc/PID` of `ssh` can still land on a new process, also of another
+user. Then its command line and those of its parents can be sent as
+claimed text; its reason only if it runs as the same user, as Linux shows
+the environment only to that user.
 Risk that remains: the person can still be fooled. Deny any request that
 looks strange.
 

internal/client/sshagent.go

@@ -310,6 +310,19 @@ func currentUID() (uint32, error) {
 type peer struct {
 	uid uint32
 	pid int32 // 0 when it is not known
+	// pidfd refers to the process itself, not to its ID (SO_PEERPIDFD,
+	// Linux 6.5 and newer). It is nil on an older kernel.
+	pidfd *os.File
+	// gone is true when the kernel has SO_PEERPIDFD but gave no pidfd:
+	// the process may have ended, and its ID may be a new process now.
+	gone bool
+}
+
+// close closes the pidfd of p, if it has one.
+func (p peer) close() {
+	if p.pidfd != nil {
+		_ = p.pidfd.Close()
+	}
 }
 
 // checkPeer allows only the user that runs nodkey ssh-agent (DESIGN.md
@@ -490,11 +503,12 @@ func (a *sshAgent) serveConn(ctx context.Context, c *net.UnixConn) {
 		a.log.note("refused a connection: its user cannot be checked")
 		return
 	}
+	defer p.close()
 	if err := checkPeer(p, a.self); err != nil {
 		a.log.note("%s", err.Error())
 		return
 	}
-	conn := &agentConn{a: a, conn: c, pid: p.pid}
+	conn := &agentConn{a: a, conn: c, peer: p}
 	for {
 		msg, err := readAgentMessage(c)
 		if err != nil {
@@ -519,7 +533,7 @@ func (a *sshAgent) serveConn(ctx context.Context, c *net.UnixConn) {
 type agentConn struct {
 	a    *sshAgent
 	conn *net.UnixConn
-	pid  int32 // the process that connected, from SO_PEERCRED
+	peer peer // the process that connected, from SO_PEERCRED
 	// binds are the contents of the session-bind@openssh.com messages of
 	// this connection, in order: at most api.MaxBinds.
 	binds [][]byte
@@ -606,7 +620,7 @@ func (c *agentConn) sign(ctx context.Context, key, data []byte, flags uint32) []
 		a.log.note("no signature: the key or the data to sign is longer than the broker reads")
 		return failureMessage
 	}
-	commands, reason := a.claimed(ctx, c.pid)
+	commands, reason := a.claimed(ctx, c.peer)
 	body, err := encodeSignRequest(newSignRequest(key, data, flags, c.binds, commands, reason))
 	if err != nil {
 		a.log.note("no signature: %s", err.Error())
@@ -647,13 +661,31 @@ func (c *agentConn) sign(ctx context.Context, key, data []byte, flags uint32) []
 	return signAnswerMessage(sig)
 }
 
-// claimed returns the claimed context of a sign request from process pid
-// (procFS.claimedContext). It waits for the reads of /proc at most
+// claimed returns the claimed context of a sign request from the process
+// p (procFS.claimedContext). When the kernel has a pidfd of p, the context
+// counts only if p still runs after its directory in /proc is open: else
+// the directory may be a new process with the same ID. When the kernel
+// has pidfds but gave none for p, p has ended, and the request goes
+// without the context; a line says so. It waits for the reads of /proc at most
 // a.procTimeout, and at most maxProcReads such reads run at the same
 // time: a read can block, such as on a stuck FUSE or NFS file system.
 // Otherwise the request goes without the command lines and the reason,
 // and a line says why.
-func (a *sshAgent) claimed(ctx context.Context, pid int32) ([][]string, string) {
+func (a *sshAgent) claimed(ctx context.Context, p peer) ([][]string, string) {
+	const ended = "sent a sign request without the command lines and the reason: ssh has ended"
+	if p.gone {
+		a.log.note(ended)
+		return [][]string{}, ""
+	}
+	var running func() bool
+	stopped := false // read only after claimedContext returned
+	if p.pidfd != nil {
+		running = func() bool {
+			r := processRuns(p.pidfd)
+			stopped = !r
+			return r
+		}
+	}
 	if a.procReads.Add(1) > maxProcReads {
 		a.procReads.Add(-1)
 		a.log.note("sent a sign request without the command lines and the reason: %d reads of /proc are stuck",
@@ -667,13 +699,16 @@ func (a *sshAgent) claimed(ctx context.Context, pid int32) ([][]string, string)
 	got := make(chan found, 1)
 	go func() {
 		defer a.procReads.Add(-1)
-		commands, reason := a.proc.claimedContext(pid)
+		commands, reason := a.proc.claimedContext(p.pid, running)
 		got <- found{commands, reason}
 	}()
 	t := time.NewTimer(a.procTimeout)
 	defer t.Stop()
 	select {
 	case c := <-got:
+		if stopped {
+			a.log.note(ended)
+		}
 		return c.commands, c.reason
 	case <-t.C:
 		a.log.note("sent a sign request without the command lines and the reason: /proc did not answer within %s",

internal/client/sshagent_linux.go

@@ -12,23 +12,31 @@ import (
 // peerCredentials returns the user ID and the process ID of the process
 // that connected to c, as the kernel recorded them at connect(2)
 // (SO_PEERCRED). The process cannot change them. The process ID is 0 when
-// the process is in a PID namespace that nodkey cannot see.
+// the process is in a PID namespace that nodkey cannot see. On Linux 6.5
+// and newer it also returns a pidfd of the process; the caller closes it
+// (peer.close).
 func peerCredentials(c *net.UnixConn) (peer, error) {
 	raw, err := c.SyscallConn()
 	if err != nil {
 		return peer{}, err
 	}
-	var cred *syscall.Ucred
+	var p peer
 	var credErr error
 	if err := raw.Control(func(fd uintptr) {
+		var cred *syscall.Ucred
 		cred, credErr = syscall.GetsockoptUcred(int(fd), syscall.SOL_SOCKET, syscall.SO_PEERCRED)
+		if credErr == nil {
+			p = peer{uid: cred.Uid, pid: cred.Pid}
+			peerPidfd(int(fd), &p)
+		}
 	}); err != nil {
+		p.close()
 		return peer{}, err
 	}
 	if credErr != nil {
 		return peer{}, credErr
 	}
-	return peer{uid: cred.Uid, pid: cred.Pid}, nil
+	return p, nil
 }
 
 // ownerOf returns the user ID that owns the file of fi.
@@ -121,3 +129,43 @@ func waitHangUp(c *net.UnixConn) bool {
 	})
 	return err == nil && gone
 }
+
+// Linux numbers that the syscall package of Go does not have. They are the
+// same on amd64 and arm64, the systems that nodkey is released for.
+const (
+	soPeerPidfd        = 77  // SO_PEERPIDFD, Linux 6.5
+	sysPidfdSendSignal = 424 // pidfd_send_signal(2), Linux 5.1
+)
+
+// peerPidfd sets the pidfd of the process that connected to fd
+// (SO_PEERPIDFD) in p. On a kernel without SO_PEERPIDFD it leaves p as it
+// is. Any other error, such as when the process already ended,
+// marks p as gone: its claimed context is not sent.
+func peerPidfd(fd int, p *peer) {
+	pidfd, err := syscall.GetsockoptInt(fd, syscall.SOL_SOCKET, soPeerPidfd)
+	switch {
+	case errors.Is(err, syscall.ENOPROTOOPT):
+	case err != nil || pidfd < 0:
+		p.gone = true
+	default:
+		syscall.CloseOnExec(pidfd)
+		p.pidfd = os.NewFile(uintptr(pidfd), "pidfd")
+	}
+}
+
+// processRuns reports whether the process of pidfd has not ended, with
+// signal 0 (pidfd_send_signal), which sends nothing. It reports false on
+// any error, also after pidfd was closed.
+func processRuns(pidfd *os.File) bool {
+	raw, err := pidfd.SyscallConn()
+	if err != nil {
+		return false
+	}
+	var errno syscall.Errno
+	if err := raw.Control(func(fd uintptr) {
+		_, _, errno = syscall.Syscall6(sysPidfdSendSignal, fd, 0, 0, 0, 0, 0)
+	}); err != nil {
+		return false
+	}
+	return errno == 0
+}

internal/client/sshagent_other.go

@@ -28,3 +28,6 @@ func lockFile(*os.File) error {
 // waitHangUp cannot watch a connection here: it reports false at once, so
 // a request is not cancelled when ssh hangs up.
 func waitHangUp(*net.UnixConn) bool { return false }
+
+// processRuns cannot tell here; peerCredentials gives no pidfd anyway.
+func processRuns(*os.File) bool { return false }

internal/client/sshagent_pidfd_linux_test.go

@@ -0,0 +1,105 @@
+package client
+
+import (
+	"net"
+	"net/http"
+	"os"
+	"os/exec"
+	"strings"
+	"syscall"
+	"testing"
+
+	"github.com/code6x/nodkey/internal/api"
+	"github.com/code6x/nodkey/internal/transport"
+)
+
+// pidfdOf returns a pidfd of a process of the test (pidfd_open(2), Linux
+// 5.3).
+func pidfdOf(t *testing.T, pid int) *os.File {
+	t.Helper()
+	const sysPidfdOpen = 434 // the same on amd64 and arm64
+	fd, _, errno := syscall.Syscall(sysPidfdOpen, uintptr(pid), 0, 0)
+	if errno != 0 {
+		t.Skipf("pidfd_open: %v", errno)
+	}
+	f := os.NewFile(fd, "pidfd")
+	t.Cleanup(func() { _ = f.Close() })
+	return f
+}
+
+// TestPeerExitedBeforeRead: ssh connects and ends before the agent reads
+// /proc, and a new process gets its ID. The sign request goes without
+// claimed context: nothing of the new process is sent (issue #61). The
+// same holds when the kernel has pidfds but gave none.
+func TestPeerExitedBeforeRead(t *testing.T) {
+	t.Parallel()
+	sleep := needProgram(t, "sleep")
+	cmd := exec.CommandContext(t.Context(), sleep, "60") //nolint:gosec // G204: the test runs sleep.
+	if err := cmd.Start(); err != nil {
+		t.Fatal(err)
+	}
+	pid := cmd.Process.Pid
+	pidfd := pidfdOf(t, pid)
+	if !processRuns(pidfd) {
+		_ = cmd.Process.Kill()
+		_ = cmd.Wait()
+		t.Fatal("processRuns reports false for a process that runs")
+	}
+	_ = cmd.Process.Kill()
+	_ = cmd.Wait()
+	if processRuns(pidfd) {
+		t.Fatal("processRuns reports true for a process that ended")
+	}
+	// The new process with the same ID, in a /proc tree of the test.
+	canary := madeUp("canary")
+	p := newFakeProc(t)
+	p.add(t, pid, 1, []string{"new-process", canary}, []string{"NODKEY_REASON=" + canary})
+	self, err := currentUID()
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, tt := range []struct {
+		name string
+		peer peer
+	}{
+		{"the pidfd says it ended", peer{uid: self, pid: int32(pid), pidfd: pidfd}}, //nolint:gosec // G115: a process ID fits in an int32.
+		{"no pidfd", peer{uid: self, pid: int32(pid), gone: true}},                  //nolint:gosec // G115: a process ID fits in an int32.
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			bodies := make(chan []byte, 1)
+			fb := newFakeBroker(t, func(w http.ResponseWriter, r *http.Request) {
+				bodies <- readBody(t, r)
+				transport.WriteError(w, http.StatusForbidden)
+			})
+			au := startAgent(t, fb.b, agentOptions{proc: p.fs(), peer: func(*net.UnixConn) (peer, error) {
+				p := tt.peer
+				if p.pidfd != nil { // the agent closes its pidfd: it gets a copy
+					fd, err := syscall.Dup(int(p.pidfd.Fd()))
+					if err == nil {
+						syscall.CloseOnExec(fd)
+					}
+					if err != nil {
+						return peer{}, err
+					}
+					p.pidfd = os.NewFile(uintptr(fd), "pidfd")
+				}
+				return p, nil
+			}})
+			if signatureOf(t, rawCall(t, au.conn(t), signRequestMessage([]byte("key"), []byte("data"), 0))) != nil {
+				t.Fatal("the agent gave ssh a signature")
+			}
+			var req api.SignRequest
+			if err := req.UnmarshalJSON(<-bodies); err != nil {
+				t.Fatal(err)
+			}
+			if len(req.Commands) != 0 || req.Reason != "" {
+				t.Errorf("the request has %d command lines and a reason of %d bytes; want none", len(req.Commands),
+					len(req.Reason))
+			}
+			ended := "sent a sign request without the command lines and the reason: ssh has ended"
+			if countLines(au.log.String(), ended) != 1 || strings.Contains(au.log.String(), canary) {
+				t.Errorf("the agent wrote %q; want one line that ssh has ended", au.lines())
+			}
+		})
+	}
+}