bugzilla-daemon at bugzilla.mindrot.org
2011-May-12 19:32 UTC
[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.
bugzilla-daemon at bugzilla.mindrot.org
2011-May-12 19:33 UTC
[Bug 1905] check_parent_exists() logic does not cover all cases
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.
bugzilla-daemon at bugzilla.mindrot.org
2011-Jun-03 00:56 UTC
[Bug 1905] check_parent_exists() logic does not cover all cases
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.
bugzilla-daemon at bugzilla.mindrot.org
2011-Jun-03 00:56 UTC
[Bug 1905] check_parent_exists() logic does not cover all cases
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.
bugzilla-daemon at bugzilla.mindrot.org
2011-Jun-03 01:28 UTC
[Bug 1905] check_parent_exists() logic does not cover all cases
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.
bugzilla-daemon at bugzilla.mindrot.org
2011-Jun-03 01:38 UTC
[Bug 1905] check_parent_exists() logic does not cover all cases
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.
bugzilla-daemon at bugzilla.mindrot.org
2011-Sep-06 05:32 UTC
[Bug 1905] check_parent_exists() logic does not cover all cases
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.
Maybe Matching Threads
- [PATCH] bug in check_parent_exists() in ssh-agent.c
- ssh-agent subprocess parentage
- [Bug 3181] New: ssh-agent doesn't exit automatically after child program exits
- [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).
- Re: [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).