diff --git a/DESIGN.md b/DESIGN.md index 44da633..270351e 100644 --- a/DESIGN.md +++ b/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 diff --git a/THREAT-MODEL.md b/THREAT-MODEL.md index fb1fd6a..78a05c3 100644 --- a/THREAT-MODEL.md +++ b/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. diff --git a/internal/client/sshagent.go b/internal/client/sshagent.go index 87ab97b..dbd8e0f 100644 --- a/internal/client/sshagent.go +++ b/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", diff --git a/internal/client/sshagent_linux.go b/internal/client/sshagent_linux.go index fae572c..0105eb1 100644 --- a/internal/client/sshagent_linux.go +++ b/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 +} diff --git a/internal/client/sshagent_other.go b/internal/client/sshagent_other.go index 3b14526..800b6b1 100644 --- a/internal/client/sshagent_other.go +++ b/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 } diff --git a/internal/client/sshagent_pidfd_linux_test.go b/internal/client/sshagent_pidfd_linux_test.go new file mode 100644 index 0000000..c31e2e8 --- /dev/null +++ b/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()) + } + }) + } +} diff --git a/internal/client/sshagent_proc.go b/internal/client/sshagent_proc.go index 881acf5..e49ff73 100644 --- a/internal/client/sshagent_proc.go +++ b/internal/client/sshagent_proc.go @@ -45,6 +45,9 @@ const ( // give it a tree of their own. type procFS struct { root string + // opened, when not nil, is called after the directory of a process + // is open. Only tests set it. + opened func(pid int) } // claimedContext returns the claimed context of a sign request from the @@ -55,6 +58,15 @@ type procFS struct { // stops at the first one that cannot be read, has no command line, such // as a kernel thread, or is there twice. // +// A process ID can be used again by a new process once the process ended +// (issue #61). So when running is not nil, the context counts only if +// running reports true after the directory of pid is open: then the +// directory is of the process that connected, and the reads through it +// stay on it. running is nil when the kernel cannot tell (before Linux +// 6.5); then the open may land on a new process. A parent counts only if +// its child, after the open of the parent's directory, still names it as +// its parent: a process whose parent ended gets another parent. +// // Each command line and the reason are as nodkey run sends them: a field // that is too long becomes "[hidden: too long]" (claimedCommand, // claimedText). Nothing else of the environment is kept or sent. @@ -62,21 +74,23 @@ type procFS struct { // A read of /proc can block, such as on a mapping of a stuck FUSE or NFS // file system; sshAgent.claimed waits for claimedContext only for a short // time. -func (p procFS) claimedContext(pid int32) (commands [][]string, reason string) { +func (p procFS) claimedContext(pid int32, running func() bool) (commands [][]string, reason string) { commands = [][]string{} - pr, err := p.openProcess(int(pid)) + child, err := p.openProcess(int(pid)) if err != nil { return commands, "" } - argv, ok := pr.commandLine() + defer func() { child.close() }() + if running != nil && !running() { + return commands, "" + } + argv, ok := child.commandLine() if !ok { - pr.close() return commands, "" } commands = append(commands, argv) - reason = pr.reason() - parent, hasParent := pr.parent() - pr.close() + reason = child.reason() + parent, hasParent := child.parent() seen := map[int]bool{int(pid): true} for range maxParents { if !hasParent || seen[parent] { @@ -87,13 +101,18 @@ func (p procFS) claimedContext(pid int32) (commands [][]string, reason string) { if err != nil { break } - argv, ok := pr.commandLine() - parent, hasParent = pr.parent() - pr.close() + if again, ok := child.parent(); !ok || again != parent { + pr.close() + break + } + child.close() + child = pr + argv, ok := child.commandLine() if !ok { break } commands = append(commands, argv) + parent, hasParent = child.parent() } return commands, reason } @@ -115,6 +134,9 @@ func (p procFS) openProcess(pid int) (*process, error) { if err != nil { return nil, err } + if p.opened != nil { + p.opened(pid) + } return &process{dir: dir}, nil } diff --git a/internal/client/sshagent_proc_linux_test.go b/internal/client/sshagent_proc_linux_test.go index 7168074..9428621 100644 --- a/internal/client/sshagent_proc_linux_test.go +++ b/internal/client/sshagent_proc_linux_test.go @@ -48,7 +48,7 @@ func TestClaimedContextRealProcess(t *testing.T) { return len(argv) > 0 && argv[0] == sleep }) proc := procFS{root: procDir} - commands, reason := proc.claimedContext(int32(pid)) //nolint:gosec // G115: a process ID fits in an int32. + commands, reason := proc.claimedContext(int32(pid), nil) //nolint:gosec // G115: a process ID fits in an int32. want := [][]string{{sleep, "60"}, {sh, "-c", script, sleep, canary}, os.Args} if parent := realCommandLine(t, os.Getppid()); parent != nil { want = append(want, parent) diff --git a/internal/client/sshagent_proc_test.go b/internal/client/sshagent_proc_test.go index 990050c..f358bd8 100644 --- a/internal/client/sshagent_proc_test.go +++ b/internal/client/sshagent_proc_test.go @@ -89,20 +89,61 @@ func TestClaimedContextParents(t *testing.T) { p.add(t, 97, 96, []string{"node", "agent.js"}, nil) p.add(t, 96, 1, []string{"sshd: dev@pts/0"}, nil) p.add(t, 1, 0, []string{"/sbin/init"}, nil) - commands, reason := p.fs().claimedContext(100) + commands, reason := p.fs().claimedContext(100, nil) want := [][]string{{"ssh", "deploy@web-1"}, {"ansible-playbook", "site.yml"}, {"/bin/bash", "-c", "make deploy"}, {"node", "agent.js"}} if !slices.EqualFunc(commands, want, slices.Equal) || reason != "apply the nginx fix" { t.Errorf("claimedContext = %q, %q; want %q, the reason of process 100", commands, reason, want) } - if got, _ := p.fs().claimedContext(98); len(got) != 4 || got[3][0] != "/sbin/init" { + if got, _ := p.fs().claimedContext(98, nil); len(got) != 4 || got[3][0] != "/sbin/init" { t.Errorf("from process 98: %q; want 98, 97, 96 and 1", got) } - if got, reason := p.fs().claimedContext(1); len(got) != 1 || reason != "" { + if got, reason := p.fs().claimedContext(1, nil); len(got) != 1 || reason != "" { t.Errorf("from process 1: %q, %q; it has no parent", got, reason) } } +// TestClaimedContextNotRunning: when the process that connected has ended +// by the time its directory in /proc is open, the directory may be a new +// process with the same ID, so there is no claimed context at all (issue +// #61). +func TestClaimedContextNotRunning(t *testing.T) { + t.Parallel() + p := newFakeProc(t) + p.add(t, 100, 99, []string{"new-process"}, []string{"NODKEY_REASON=from the new process"}) + p.add(t, 99, 1, []string{"bash"}, nil) + asked := 0 + commands, reason := p.fs().claimedContext(100, func() bool { asked++; return false }) + if len(commands) != 0 || reason != "" || asked != 1 { + t.Errorf("claimedContext = %q, %q, asked %d times; want nothing, asked once", commands, reason, asked) + } + commands, _ = p.fs().claimedContext(100, func() bool { return true }) + if len(commands) != 2 { + t.Errorf("a process that runs: %q; want it and its parent", commands) + } +} + +// TestClaimedContextParentChanged: a parent counts only if its child still +// names it as its parent after the parent's directory is open. Else the +// parent may have ended, and its ID may be a new process (issue #61). +func TestClaimedContextParentChanged(t *testing.T) { + t.Parallel() + p := newFakeProc(t) + p.add(t, 100, 99, []string{"ssh", "web-1"}, nil) + p.add(t, 99, 98, []string{"new-process"}, nil) + p.add(t, 98, 1, []string{"bash"}, nil) + fs := p.fs() + fs.opened = func(pid int) { + if pid == 99 { // the parent ended; process 100 has a new parent now + p.add(t, 100, 1, []string{"ssh", "web-1"}, nil) + } + } + commands, _ := fs.claimedContext(100, nil) + if want := [][]string{{"ssh", "web-1"}}; !slices.EqualFunc(commands, want, slices.Equal) { + t.Errorf("claimedContext = %q; want %q, without the new process", commands, want) + } +} + // TestClaimedContextStops: the chain of parents stops at a parent that // cannot be read, has no command line (a kernel thread), or is there // twice. Without the command line of the process that connected, there @@ -138,7 +179,7 @@ func TestClaimedContextStops(t *testing.T) { {-1, [][]string{}, ""}, } for _, tt := range tests { - commands, reason := p.fs().claimedContext(tt.pid) + commands, reason := p.fs().claimedContext(tt.pid, nil) if !slices.EqualFunc(commands, tt.want, slices.Equal) || commands == nil || reason != tt.reason { t.Errorf("process %d: %q, %q; want %q, %q", tt.pid, commands, reason, tt.want, tt.reason) } diff --git a/internal/client/sshagent_test.go b/internal/client/sshagent_test.go index 9f9bc82..6de7df7 100644 --- a/internal/client/sshagent_test.go +++ b/internal/client/sshagent_test.go @@ -986,6 +986,9 @@ func TestPeerCredentials(t *testing.T) { fb := newFakeSSHBroker(t, nil, nil) au := startAgent(t, fb.b, agentOptions{peer: func(c *net.UnixConn) (peer, error) { p, err := peerCredentials(c) + if p.gone || (p.pidfd != nil && !processRuns(p.pidfd)) { + t.Errorf("SO_PEERPIDFD: gone %v; want a pidfd of a process that runs, or none on an old kernel", p.gone) + } got <- p return p, err }})