This is the mail archive of the cygwin-developers@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fixing the PROCESS_DUP_HANDLE security hole.


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
>><http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/process_security_and_access_rights.asp>
>>
>>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,
>>		0, TRUE, DUPLICATE_SAME_ACCESS)
>>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.

Pierre


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]