openssh bugs - May 2011 - [Bug 1905] New: check_parent_exists() logic does not cover all cases

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

           Summary: check_parent_exists() logic does not cover all cases
           Product: Portable OpenSSH
           Version: 5.8p2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: ssh-agent
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: dkg at fifthhorseman.net


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.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

--- Comment #1 from Daniel Kahn Gillmor <dkg at fifthhorseman.net>
2011-05-13 05:33:09 EST ---
Created attachment 2045
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2045
patch for check_parent_exists which assumes orphaned processes get
reparented to PID 1

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
             Blocks|                            |1845

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> 2011-06-03
10:56:20 EST ---
Looks reasonable to me.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2045|                            |ok?(djm at mindrot.org)
              Flags|                            |

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2045|ok?(djm at mindrot.org)        |ok+
              Flags|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> 2011-06-03 11:28:42
EST ---
Comment on attachment 2045
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2045
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@

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #4 from Darren Tucker <dtucker at zip.com.au> 2011-06-03
11:38:05 EST ---
Thanks, applied, it will be in 5.9.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1905

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Damien Miller <djm at mindrot.org> 2011-09-06 15:32:54
EST ---
close resolved bugs now that openssh-5.9 has been released

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.