This is the mail archive of the
cygwin-developers@cygwin.com
mailing list for the Cygwin project.
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