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.


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.

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.

>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.

>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.  MSDN suggests that specifying SYNCHRONIZE is
necessary, at least on some platforms, and I seem to recall that 0
didn't work right everywhere, which is the reason that I just defaulted
to DUPLICATE_SAME_ACCESS.

cgf


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