This is the mail archive of the cygwin@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: More security issues


At 04:57 PM 2/12/2002 +0100, Corinna Vinschen wrote:
>On Sun, Feb 10, 2002 at 02:34:55PM -0500, Pierre A. Humblet wrote:

Corinna, I have changed the order of the items.

>> 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?
<snip>
What I had in mind is something like this (untested but see attached diff
with even more drastic changes)
@@ -647,17 +647,11 @@
     }
   else
     {
-      cygsid sid;
       DWORD ret_len;
-      if (!GetTokenInformation (hToken, TokenUser, &sid, sizeof sid,
&ret_len))
-       {
-         sid = NO_SID;
-         system_printf ("GetTokenInformation: %E");
-       }
       /* Retrieve security attributes before setting psid to NULL
         since it's value is needed by `sec_user'. */
-      PSECURITY_ATTRIBUTES sec_attribs = allow_ntsec && sid
-                                        ? sec_user (sa_buf, sid)
+      PSECURITY_ATTRIBUTES sec_attribs = allow_ntsec
+                                        ? sec_user (sa_buf)
                                         : &sec_all_nih;

>>  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.
I haven't put all the printf to prove it, but I believe that the SD's
passed to CreateToken determine the DACLs of the process and main thread
handles (so they ARE used), but not those of the process and thread 
themselves (so it isn't very useful, except to limit access to handles
if you pass them around). The technical writer didn't make the distinction.

Similarly the SD given to DuplicateTokenEx() (in create_token()) is for 
the primary token produced by that function. Later ImpersonateLoggedOnUser()
uses that token to produce an impersonated thread, but the impersonated 
thread doesn't get its SD from the access token..  (*)
When I open the thread, the SD of the handle token seems to come from the
Owner/Primary/Group/defDACL of the process token.

To kind of prove that, I have a new simplified (perhaps too much!) 
spawn.cc that doesn't call sec_user(). It runs fine and I see no change 
in the tokens openened by the exec'ed process. See the attached diff file.

>> All of this effort was motivated by weird access issues to the 
>> impersonation token. I can fix that by opening the thread token
<snip>
>That can't be the way to go.  Somehow we should try to figure out to do
>it correctly.
>
That's why I didn't do it, but if my theory is correct (see (*) above),
we can either do that, or modify the default DACL of the process token
(it seems to be used to build the sd of the impersonation token)

<reordered>
>> 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 ;-)
Yes, it seems to work (see lines with +// in diff). Do you think this 
breaks something? I didn't understand the comment 
      /* Restore impersonation. In case of _P_OVERLAY this isn't
	 allowed since it would overwrite child data. */

>> Back to setegid(), another safe way would be to 
>> RevertToSelf(),..,Impersonate..() if currently impersonated.

>Did you try if that works reliable?  Nobody keeps you from patching it ;-)
Well, if I remove the RevertToSelf() [see just above], then it shouldn't 
work. I'd like to get a stable spawn.cc [and fork.cc too, by the way]
(and understand how MS really propagates security info), before 
taking care of the PrimaryGroup.

Pierre

Attachment: spawn.diff
Description: Text document

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

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