[PATCH] bug in check_parent_exists() in ssh-agent.c

[Alan Barrett]

The check_parent_exists() function in ssh-agent.c does this:

        if (parent_pid != -1 && kill(parent_pid, 0) < 0)

however, the kill can fail with EPERM even if the parent_pid
exists.  For example, consider this command:

        ssh-agent sh -c 'ssh-add ; exec sudo sh -i'

The original ssh-agent process sets things up so that the "sh -c '...'"
process is the parent of a child ssh-agent process; then the "sh -c
'...' process execs sudo (which is setuid root), and sudo execs "sh -i"
(which is now running as root); so now the "sh -i" process (running as
root) is the parent of the "ssh-agent" process (running as the original
user).  When the ssh-agent child process calls check_parent_exists()
after 10 seconds, the kill will fail with EPERM, and the ssh-agent
process will exit.  The obvious symptom is that "ssh-add -l" executed
in the child shell works if you are quick enough, but doesn't work 10
seconds later.

Two potential fixes come to mind:

A: if (parent_pid != -1 && kill(parent_pid, 0) < 0 && errno == ESRCH)

B: if (parent_pid != -1 && getppid() != parent_pid)

Fix A assumes that errno == ESRCH means that the process really doesn't
exist, and that other errno values mean other things.  This assumption
would fail in a unix-like OS that, for security reasons, decides not to
let processes learn anything about the existence of processes owned by
other users.

Fix B assumes that, when your parent exits, you get re-parented to pid 1
and the result from getppid() changes.  I have tested this and it works
for me.  I append a patch.

--apb (Alan Barrett)

Index: ssh-agent.c
--- ssh-agent.c
+++ ssh-agent.c
@@ -964,7 +964,13 @@
 {
 	int save_errno = errno;
 
-	if (parent_pid != -1 && kill(parent_pid, 0) < 0) {
+	/*
+	 * Don't just test whether kill(parent_pid,0) is successful,
+	 * because it may fail with EPERM even if the process exists.
+	 * If our parent has exited then getppid() will return (pid_t)1,
+	 * so testing for that should be safe.
+	 */
+	if (parent_pid != -1 && getppid() != parent_pid) {
 		/* printf("Parent has died - Authentication agent exiting.\n"); */
 		cleanup_handler(sig); /* safe */
 	}