Bug 1905 - check_parent_exists() logic does not cover all cases
Summary: check_parent_exists() logic does not cover all cases
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh-agent (show other bugs)
Version: 5.8p2
Hardware: All All
: P2 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks: V_5_9
  Show dependency treegraph
 
Reported: 2011-05-13 05:32 AEST by Daniel Kahn Gillmor
Modified: 2011-09-06 15:32 AEST (History)
1 user (show)

See Also:


Attachments
patch for check_parent_exists which assumes orphaned processes get reparented to PID 1 (565 bytes, patch)
2011-05-13 05:33 AEST, Daniel Kahn Gillmor
djm: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Kahn Gillmor 2011-05-13 05:32:09 AEST
As initially reported on the mailing list:

http://lists.mindrot.org/pipermail/openssh-unix-dev/2006-April/024144.html

Alan P. Barrett wrote:

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.
Comment 1 Daniel Kahn Gillmor 2011-05-13 05:33:09 AEST
Created attachment 2045 [details]
patch for check_parent_exists which assumes orphaned processes get reparented to PID 1
Comment 2 Darren Tucker 2011-06-03 10:56:20 AEST
Looks reasonable to me.
Comment 3 Damien Miller 2011-06-03 11:28:42 AEST
Comment on attachment 2045 [details]
patch for check_parent_exists which assumes orphaned processes get reparented to PID 1

>+	 * Don't just test whether kill(parent_pid,0) is successful,
>+	 * because it may fail with EPERM even if the process exists.

There is no point mentioning kill() if it is no longer used in the source. Otherwise ok djm@
Comment 4 Darren Tucker 2011-06-03 11:38:05 AEST
Thanks, applied, it will be in 5.9.
Comment 5 Damien Miller 2011-09-06 15:32:54 AEST
close resolved bugs now that openssh-5.9 has been released