Re: [Patch] Fixing the PROCESS_DUP_HANDLE security hole.

["Pierre A. Humblet" <pierre at phumblet dot no-ip dot org>]

At 12:54 AM 12/24/2004 -0500, Pierre A. Humblet wrote:
>At 12:25 AM 12/24/2004 -0500, Christopher Faylor wrote:
>>On Thu, Dec 23, 2004 at 11:59:59PM -0500, Pierre A. Humblet wrote:
>>>At 11:35 PM 12/23/2004 -0500, Christopher Faylor wrote:
>>>>I don't think you need it.  You just need to tell a process which is
>>>>about to exec after having been execed to make sure that its
>>>>wr_proc_pipe is valid.
>>>
>>>Yes, that's the key. So the question is only about method. Either the
parent
>>>guarantees that the child has a valid handle, or the child must check
>>>that it already has a valid handle or wait until it does. 
>>
>>I have just implemented code which causes an execed child to wait for the
>>parent to fill in its wr_proc_pipe if it is going to exec again.  It uses
>>a busy loop but I think it's unlikely that the loop will be exercised too
>>often.
>
>It's late, but I am trying to go through all permutations.
>Here is a strange one. 
>Cygwin process A started from Windows execs a Windows process B.
>We are in the case where A
>      if (!myself->wr_proc_pipe)
>       {
>         myself.remember (true);
>         wait_for_myself = true;
>
>The problem is that later there is
>if (wait_for_myself)
>  waitpid (myself->pid, &res, 0);
>else
>  ciresrv.sync (myself, INFINITE);
>
>Process A takes the first branch (waitpid), although it's the
>second branch that will call GetExitCodeProcess.
>So A will see its logical self terminate, but it won't get the
>exit status of B. 
>Right? Going to sleep on this.

I think the way out is as follows:
Toward the end of spawn_guts:

ciresrv.sync (myself, INFINITE);   [always]

if (wait_for_myself)
   waitpid (myself->pid, &dummy, 0);
 [For clarity, these two lines should be brought down
  inside the case _P_OVERLAY: ]

and in pinfo::exit, change ExitProcess (n) to
ExitProcess (exitcode)

There is NO NEED for a Cygwin process started from Windows to
start a new exec'ed process in suspended state.

The ciressrv.sync will collect the exit status of any Windows
process.
The purpose of the waitpid is to wait for the process chain to be
finished. But waitpid will fail if the child had terminated before
the pipe could be duplicated. That't why waitpid uses "dummy".
At any rate the final return value of the chain is safely set in
the exitstatus.

The change in pinfo::exit is to handle the "norecord" case. In that
case the value of n is meaningless, the correct exit code is already
set in exitcode.

Pierre