Fixing the PROCESS_DUP_HANDLE security hole.

Pierre A. Humblet
Sat Sep 27 23:18:00 GMT 2003

At 06:08 PM 9/27/2003 -0400, Christopher Faylor wrote:
>On Sat, Sep 27, 2003 at 12:43:03PM -0400, Pierre A. Humblet wrote:
>>Continuing plugging security holes in the core of Cygwin...
>>The PROCESS_DUP_HANDLE privilege is currently used in 3 different fashions
>>in Cygwin:
>Can I reiterate my suggestion that you tackle one thing at a time?
>Rather than send long email talking about four different problems, four
>separate emails (and eventually four patches) are easier to digest.

Those 4 problems are very much related, particularly 2, 3 and 4 and they 
have to be thought about together. Note that the patch is about one of them.

>However, in particular, suggestions for how to deal with the pipe/signal
>code are premature at this point.  As I've mentioned, I checked in what
>I had because my changes were getting out of hand but the code is not
>done yet.

OK. As I wrote I won't produce any code for that, for the moment.
Personally, I prefer to receive suggestions before a job is finished.

>>1) It is necessary to connect to a master TTY. Processes creating master ttys 
>>  currently give total access to themselves from everybody.
>>2) It is necessary to do reparenting. A child is given a parent handle that
>>  has PROCESS_DUP_HANDLE, and it dups the grandchild handle into the grandparent.
>>3) It is needed to send a signal to a process (and for various interprocess
>>  communication with pipes). AFAICS such code takes no action to allow 
>>  access to the process, thus I would expect:
>>4) A setuid'ed child of a process that has not created a master tty cannot
>>  signal its parent (e.g. for SIGCHLD).
>>Having PROCESS_DUP_HANDLE to a process effectively grants full access, 
>>including the right to modify memory. Thus we must be extremely careful
>>with it. See the remark at then bottom of
>>Specifically, the statements:
>>a) tty::common_init:
>>    	  !SetKernelObjectSecurity (hMainProc, DACL_SECURITY_INFORMATION, get_null_sd ()))
>>must be eliminated.
>This code predates me so I have to ask why it was there in the first place if
>it is unneeded.

There is an accurate comment: /* Allow the others to open us (for handle duplication) */
The proposed patch is avoiding the need for that.
>>b) proc_subproc:
>>      if (!DuplicateHandle (hMainProc, hMainProc, vchild->hProcess, &vchild->ppid_handle,
>>should be changed to give no access rights to the duplicated handle.
>>This will still allow to check if the parent is alive, but not to signal it nor
>>to reparent.
>Have you verified this on all platforms?  I don't think you can assume
>that ppid_handle will work correctly in a Wait* function if it is duplicated
>with no special access.  

Absolutely correct, SYNCHRONIZE is needed.


More information about the Cygwin-developers mailing list