Re: More security issues

[Corinna Vinschen <cygwin@cygwin.com>]

On Sun, Feb 10, 2002 at 02:34:55PM -0500, Pierre A. Humblet wrote:
>  I wonder what the sa in CreateProcess
> really does... The only thing that has an effect is the Inherit flag.

MSDN documents the SD in the lpProcessAttributes/lpThreadAttributes
argument being used as the SD of the called process/main thread.
The SD of the process seems not to correspond with the default DACL
in the token.  However, the sec_user() isn't w/o effect.  You
can easily check that by changing the function to create a wrong
DACL.

> In the course of debugging I also noticed that the sid2 passed
> to sec_user() from just before CreateProcessAsUser() is useless.
> It is actually equal to the sid that sec_user() gets from 
> cygheap->user.sid ()  [cygheap->user is set in seteuid()]

Does the following patch help?

Index: spawn.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/spawn.cc,v
retrieving revision 1.97
diff -u -p -r1.97 spawn.cc
--- spawn.cc	2002/02/10 13:38:49	1.97
+++ spawn.cc	2002/02/12 15:54:53
@@ -647,6 +647,11 @@ spawn_guts (HANDLE hToken, const char * 
     }
   else
     {
+      /* Remove impersonation */
+      if (cygheap->user.impersonated
+	  && cygheap->user.token != INVALID_HANDLE_VALUE)
+	RevertToSelf ();
+
       cygsid sid;
       DWORD ret_len;
       if (!GetTokenInformation (hToken, TokenUser, &sid, sizeof sid, &ret_len))
@@ -659,11 +664,6 @@ spawn_guts (HANDLE hToken, const char * 
       PSECURITY_ATTRIBUTES sec_attribs = allow_ntsec && sid
 					 ? sec_user (sa_buf, sid)
 					 : &sec_all_nih;
-
-      /* Remove impersonation */
-      if (cygheap->user.impersonated
-	  && cygheap->user.token != INVALID_HANDLE_VALUE)
-	RevertToSelf ();
 
       static BOOL first_time = TRUE;
       if (first_time)

> All of this effort was motivated by weird access issues to the 
> impersonation token. I can fix that by opening the thread token
> security descriptor after ImpersonateLoggedOnUser() in seteuid()
> and changing the ACL (using the ACL from sec_user(), that works!). 
> Unfortunately the work must be redone each time the sequence 
> RevertToSelf(), ..., ImpersonateLoggedOnUser() occurs. 

That can't be the way to go.  Somehow we should try to figure out to do
it correctly.

> Back to setegid(), another safe way would be to 
> RevertToSelf(),..,Impersonate..() if currently impersonated.
> That's because there is also a RevertToSelf() before CreateProcessAsUser()
> Why is there one, by the way? Microsoft seems to suggest working in the
> security context of the new user. It says it's useful if the executable 
> is only executable by the new user.

Did you try if that works reliable?  Nobody keeps you from patching it ;-)

Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Developer                                mailto:cygwin@cygwin.com
Red Hat, Inc.

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/