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]

Fixing the PROCESS_DUP_HANDLE security hole.


Continuing plugging security holes in the core of Cygwin...

The PROCESS_DUP_HANDLE privilege is currently used in 3 different fashions
in Cygwin:

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.

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.
 
A) To solve 4), the child should not try to open the parent process
and duplicate the sig pipe handle. It must be given by its parent
a duplicate of the parent's sig pipe handle and use it in sig_send.
In case of an exec, that copy must be duplicated and passed to
the grandchild during spawn. We have to be careful because the grand
child should not use that pipe to signal its (grand)parent before
reparenting as occurred.

B) To solve 2), the grandparent must obtain a handle to the grandchild
(in the space of the child), and duplicate it himself.
Fortunately, this can easily be done by reusing the sig pipe to the parent
from the child, as per paragraph A, to give the parent a handle to the
grandchild (in the child process).
After passing the handle, the child must remain alive long enough to
allow the parent to duplicate the handle. So we either need an ACK from the
parent to the child, or more simply the parent could TerminateProcess the 
child once reparenting is complete (the reparenting code would be moved 
near the very end of the child). 

C) One way to solve 1) is as follows. In practice processes want to open
/dev/tty, which gets remapped to some /dev/ttyN, depending on the ctty.
Opening /dev/ttyN can fail for security reasons, but in practice we can
arrange for processes to have inherited handles to their ctty, that are
created just after the master tty has been opened, before seteuid. 
So as in A), no duplication from the master pid is necessary to open 
/dev/tty.

I am attaching some "proof of principle" code that does C). It works
but I am not a tty expert and I expect bugs. I tested it (strace) with rxvt 
and telnetd. It also works with sshd, but sshd crashes under strace and gdb
on my WinME so I couldn't check all the details.
 
I won't proceed with more changes in this area until I hear from Chris.


Pierre


P.S. Is this normal? The 34064 appears to be garbage in the child.
  161 5543587 [main] in.telnetd 265749139 fhandler_tty_slave::dup: incremented open_fhs 3
  168 5543755 [main] in.telnetd 265749139 tty_list::connect_tty: ttynum (34064) out of range


P.S. for Corinna:

At 04:39 PM 9/26/2003 +0200, you wrote:
>On Fri, Sep 26, 2003 at 09:41:17AM -0400, Pierre A. Humblet wrote:
>> Corinna Vinschen wrote:
>> > Somehow I'm missing a description why that's necessary and the
>> > implications.
>> > 
>> I am getting paranoid. Most often we duplicate DUPLICATE_SAME_ACCESS
>> without thinking about what access is really needed. It would be a good
>> discipline to ask ourselves what is needed and give just that. Here nothing
>> is needed at all. 
>
>This handle is only used to have a handle.  It's never accessed, just
>kept so that we not get the same Winpid again, right?  If so, the
>patch is ok, of course.  I was just missing a description.

Now I can explain more fully: this inheritable handle was giving full access
to the parent, even though its use appears to be completely benign.

Attachment: tty.diff
Description: Text document


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