More security issues

More security issues

[Pierre A. Humblet]

Hi Corinna,

I have some free time and easy access to an NT
so I came back to security issues.

As you recall, in setegid(), setting the PrimaryGroup
in the process token isn't reliable and was #if'ed out.
Consequently non-cygwin subprocesses may create objects
with the wrong primary group.

I tried to fix that by setting the primary group based on  getegid()
in the security descriptor created in sec_user(). To my surprise
that didn't have any effect. In fact sec_user() doesn't seem to
have much effect at all! It creates an ACL with 4 or 5 ACE's, but
my token printing program only shows two ACE's in the process
tokens: admins and system. I wonder what the sa in CreateProcess
really does... The only thing that has an effect is the Inherit flag.

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()]

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. 
It would be much better if we could get the sd to have an effect
in DuplicateTokenEx() [in create_token(), security.cc].
That may be related to what I observed above.
Any ideas?

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.
  
Pierre
P.S.: please cc me directly. 


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

Re: More security issues

[Corinna Vinschen]

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/

Re: More security issues

[Pierre A. Humblet]

[-- Attachment #1: Type: text/plain, Size: 4173 bytes --]

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 #2: spawn.diff --]
[-- Type: text/plain, Size: 3093 bytes --]

--- spawn.cc.org	Tue Feb 12 11:28:18 2002
+++ spawn.cc	Tue Feb 12 12:53:16 2002
@@ -612,9 +612,6 @@
   else
     envblock = winenv (envp, 0);

-  /* Preallocated buffer for `sec_user' call */
-  char sa_buf[1024];
-
   if (!hToken && cygheap->user.impersonated
       && cygheap->user.token != INVALID_HANDLE_VALUE)
     hToken = cygheap->user.token;
@@ -634,10 +631,8 @@
       newheap = cygheap_setup_for_child (&ciresrv, cygheap->fdtab.need_fixup_before ());
       rc = CreateProcess (runpath,	/* image name - with full path */
 			  one_line.buf,	/* what was passed to exec */
-					  /* process security attrs */
-			  allow_ntsec ? sec_user (sa_buf) : &sec_all_nih,
-					  /* thread security attrs */
-			  allow_ntsec ? sec_user (sa_buf) : &sec_all_nih,
+			  &sec_all_nih, /* process security attrs */
+			  &sec_all_nih, /* thread security attrs */
 			  TRUE,	/* inherit handles from parent */
 			  flags,
 			  envblock,/* environment */
@@ -647,23 +642,10 @@
     }
   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)
-					 : &sec_all_nih;
-
       /* Remove impersonation */
-      if (cygheap->user.impersonated
-	  && cygheap->user.token != INVALID_HANDLE_VALUE)
-	RevertToSelf ();
+//      if (cygheap->user.impersonated
+//	  && cygheap->user.token != INVALID_HANDLE_VALUE)
+//	RevertToSelf ();

       static BOOL first_time = TRUE;
       if (first_time)
@@ -673,7 +655,7 @@
 	}

       /* Load users registry hive. */
-      load_registry_hive (sid);
+      load_registry_hive (cygheap->user.sid ());

       /* allow the child to interact with our window station/desktop */
       HANDLE hwst, hdsk;
@@ -697,8 +679,8 @@
       rc = CreateProcessAsUser (hToken,
 		       runpath,		/* image name - with full path */
 		       one_line.buf,	/* what was passed to exec */
-		       sec_attribs,     /* process security attrs */
-		       sec_attribs,     /* thread security attrs */
+		       &sec_all_nih,    /* process security attrs */
+		       &sec_all_nih,    /* thread security attrs */
 		       TRUE,	/* inherit handles from parent */
 		       flags,
 		       envblock,/* environment */
@@ -707,10 +689,10 @@
 		       &pi);
       /* Restore impersonation. In case of _P_OVERLAY this isn't
 	 allowed since it would overwrite child data. */
-      if (mode != _P_OVERLAY
-	  && cygheap->user.impersonated
-	  && cygheap->user.token != INVALID_HANDLE_VALUE)
-	ImpersonateLoggedOnUser (cygheap->user.token);
+//      if (mode != _P_OVERLAY
+//	  && cygheap->user.impersonated
+//	  && cygheap->user.token != INVALID_HANDLE_VALUE)
+//	ImpersonateLoggedOnUser (cygheap->user.token);
     }

   MALLOC_CHECK;


[-- Attachment #3: Type: text/plain, Size: 214 bytes --]

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

Re: More security issues

[Pierre A. Humblet]

Corinna,

please forget my previous message for now.

Pierre


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

Re: More security issues

[Corinna Vinschen]

On Wed, Feb 13, 2002 at 03:50:51PM -0500, Pierre A. Humblet wrote:
> Corinna,
> 
> please forget my previous message for now.

No problem (I'm very busy currently).  Just a side note I forgot
in my previous posting.  The sec_user() call in CreateProcess()
was never intended to set the default DACL (I didn't even know
that something like that exists when I added that) but to set the
permissions to access the process.  If you're running processes
under different user accounts you can't kill processes of other
accounts if the SA is sec_all_nih.  This is unfortunately also true
for admins.  Even worse, admins can't stop processes running under
SYSTEM account (services).  Therefore, when using ntsec, the sec_user()
call should set an SD with explicit permissions for the process which
always should allow access for

  - the user
  - admin
  - system

and, if the process is started from a different user account under
setuid() conditions,

  - the original user of the starting process

When I implemented this, the fork/exec implementation was pretty
different from today.  As far as I rememeber, the code which copied
data from one process to the other needed access under the 2nd SID.
This could qualify for some code which could be pretty useless
today.  E.g. your observation that RevertToSelf() could be dropped,
probably.

Just if that's not clear, I'm really appreciating that you're
trying to get to the bottom of that code.

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/

Re: More security issues

[Pierre A. Humblet]

Hi Corinna

At 10:13 AM 2/14/2002 +0100, you wrote:

>The sec_user() call in CreateProcess()
>was never intended to set the default DACL (I didn't even know
>that something like that exists when I added that) but to set the
>permissions to access the process.  
<snip>
Yes, and in the case of DuplicateTokenEx(), the permissions of the
new primary token. However the sd's of a new process TOKEN and of a 
new impersonation token are always initialized from the default in the 
(parent) process token. I think I now understand what's going on.
The confusion between the impersonated sid and the original sid 
that we have observed in LookupAccountSid() is also present in the 
token sd, but things work out all right, for some reason. I will 
send you some patches shortly.
<snip>

>When I implemented this, the fork/exec implementation was pretty
>different from today.  As far as I rememeber, the code which copied
>data from one process to the other needed access under the 2nd SID.
>This could qualify for some code which could be pretty useless
>today.  E.g. your observation that RevertToSelf() could be dropped,
>probably.

I am still looking at that. On 2001-10-31 you added RevertToSelf() in 
dtable.cc (dtable::vfork_child_dup). Do you remember why?

Pierre


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

Re: More security issues

[Corinna Vinschen]

On Fri, Feb 22, 2002 at 10:06:53PM -0500, Pierre A. Humblet wrote:
> >today.  E.g. your observation that RevertToSelf() could be dropped,
> >probably.
> 
> I am still looking at that. On 2001-10-31 you added RevertToSelf() in 
> dtable.cc (dtable::vfork_child_dup). Do you remember why?

Yes!  It's very important.  Without that RevertToSelf(), the
process has no access to it's own open socket handles if a setuid()
has been called before.  Go figure!

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/

Re: More security issues

[Pierre A. Humblet]

At 11:19 PM 2/23/2002 +0100, Corinna Vinschen wrote:
>> I am still looking at that. On 2001-10-31 you added RevertToSelf() in 
>> dtable.cc (dtable::vfork_child_dup). Do you remember why?
>
>Yes!  It's very important.  Without that RevertToSelf(), the
>process has no access to it's own open socket handles if a setuid()
>has been called before.  Go figure!

Hi Corinna,

OK, that's good to know! I am almost done, just some cleanup and more 
testing needed. Before starting on that I would like to get your 
comments on what I did, and I have a few questions.

1) To solve my original impersonation token access problem 
on NT, the easiest way is to change the default DACL of the 
process (the impersonation token will get the right default). 
The alternative is to set the token sd explicitly, but this 
must be redone after every RevertToSelf().
To set the dacl I use the dacl part from __sec_user(), which 
I have put in a separate function sec_dacl() (in shared.cc). 

2) To make sure Windows process use the right default group,
the default group must be set both in the process token 
(using RevertToSelf() if needed), and in the primary token
(for CreateProcessAsUser) (syscalls.cc).

3) after a sequence setegid(newg1), seteuid(newuid), 
seteuid(original), the process has an unused primary token, 
which can be used again if there is another setegid(newg2), 
seteuid(newuid). 
However that token may not be appropriate, depending on newg1 
and newg2. If the token is internal and newg1 was not in the 
"natural groups" of newuid, then newg2 must be the same as 
newg1. Otherwise it is enough that newg2 be in the token groups.
[what is there now is too strict for internal tokens, but not 
strict enough for tokens from cygwin_logon_user()] 
That is checked in a new function verify_token() 
(in security.cc), called from seteuid().

4) the primary group that was used when creating an internal token
is now saved in the token sd. This allows to set the token default
primary group appropriately if the user calls setegid() after
creating the token, e.g. seteuid(uid1) .... setegid(gid1) ....  
[can this order be legitimate ?].

5) internal_getlogin() is called in seteuid(). Why is it necessary
to call it again after CreateProcessAsUser()? Won't the environment 
already be OK?

6) The role of/need for  the last creator/owner ACE in __sec_user() 
is not clear to me. Are we ever in situations to propagate 
permissions?  
  
7) The current call to __sec_user() from spawn.cc gives access twice to 
the new user, but not to the original user. However it doesn't seem
to matter. So the code can be simplified. Also, no need to RevertToSelf().

8) Also in spawn.cc, I believe that the following code
      static BOOL first_time = TRUE;
      if (first_time)
	{
	  set_process_privilege (SE_RESTORE_NAME);
	  first_time = FALSE;
	}
isn't robust. The static variable could be FALSE in a forked process
if the parent had spawned something before. 
My suggestion is to delete those lines and modify registry.cc as follows:

--- registry.cc.org     Tue Feb 19 20:39:44 2002
+++ registry.cc Thu Feb 21 10:56:32 2002
@@ -235,12 +235,13 @@
   /* Check if user hive is already loaded. */
   cygsid csid (psid);
   csid.string (sid);
-  if (!RegOpenKeyExA (HKEY_USERS, csid.string (sid), 0, KEY_READ, &hkey))
+  if (!RegOpenKeyExA (HKEY_USERS, sid, 0, KEY_READ, &hkey))
     {
       debug_printf ("User registry hive for %s already exists", sid);
       RegCloseKey (hkey);
       return;
     }
+  set_process_privilege (SE_RESTORE_NAME);
   if (get_registry_hive_path (psid, path))
     {
       strcat (path, "\\NTUSER.DAT");

9) get_dacl() (in security.cc) gives no access to admins if the
user is not in the admins group. I don't understand the logic.
My suggestion is to call instead the new sec_dacl() (see above),
which always has system, admins, sid1, [sid2] and creator/owner.

10) It's also possible to optimize fork.cc to avoid calling sec_user()
twice.

11) Can cygwin_logon_user() be called by a user not in admins?
[If so, I will test that case].

Pierre


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

Re: More security issues

[Corinna Vinschen]

On Sun, Mar 03, 2002 at 10:05:39PM -0500, Pierre A. Humblet wrote:
> At 11:19 PM 2/23/2002 +0100, Corinna Vinschen wrote:
> 3) after a sequence setegid(newg1), seteuid(newuid), 
> seteuid(original), the process has an unused primary token, 
> which can be used again if there is another setegid(newg2), 
> seteuid(newuid). 
> However that token may not be appropriate, depending on newg1 
> and newg2. If the token is internal and newg1 was not in the 
> "natural groups" of newuid, then newg2 must be the same as 
> newg1. Otherwise it is enough that newg2 be in the token groups.
> [what is there now is too strict for internal tokens, but not 
> strict enough for tokens from cygwin_logon_user()] 
> That is checked in a new function verify_token() 
> (in security.cc), called from seteuid().

I don't understand that description.  Could you try to explain
in other words?  What do you mean by "natural group"?  Primary
group as set by Windows (RID 513, "None" or "Domain Users",
typically) or the primary group as set in /etc/passwd or ...?

> 4) the primary group that was used when creating an internal token
> is now saved in the token sd. This allows to set the token default

With what access rights?  The pgrp shouldn't have write access
to the token's sd, isn't it?

> primary group appropriately if the user calls setegid() after
> creating the token, e.g. seteuid(uid1) .... setegid(gid1) ....  
> [can this order be legitimate ?].

That won't work due to the create_token call being only in
seteuid().  This would require a redesign.

> 5) internal_getlogin() is called in seteuid(). Why is it necessary
> to call it again after CreateProcessAsUser()? Won't the environment 
> already be OK?

Not if somebody called spawn()/exec() using an external token
(one that hasn't been created inside of Cygwin).  This is true
for older implementations of login or sshd which used some
flavor of `sexecve(token, ...)'.

> 6) The role of/need for  the last creator/owner ACE in __sec_user() 
> is not clear to me. Are we ever in situations to propagate 
> permissions?  

Uhm, perhaps, it's actually not needed?

> 8) Also in spawn.cc, I believe that the following code
>       static BOOL first_time = TRUE;
>       if (first_time)
> 	{
> 	  set_process_privilege (SE_RESTORE_NAME);
> 	  first_time = FALSE;
> 	}
> isn't robust. The static variable could be FALSE in a forked process
> if the parent had spawned something before. 
> My suggestion is to delete those lines and modify registry.cc as follows:
> 
> --- registry.cc.org     Tue Feb 19 20:39:44 2002
> +++ registry.cc Thu Feb 21 10:56:32 2002
> @@ -235,12 +235,13 @@
>    /* Check if user hive is already loaded. */
>    cygsid csid (psid);
>    csid.string (sid);
> -  if (!RegOpenKeyExA (HKEY_USERS, csid.string (sid), 0, KEY_READ, &hkey))
> +  if (!RegOpenKeyExA (HKEY_USERS, sid, 0, KEY_READ, &hkey))
>      {
>        debug_printf ("User registry hive for %s already exists", sid);
>        RegCloseKey (hkey);
>        return;
>      }
> +  set_process_privilege (SE_RESTORE_NAME);
>    if (get_registry_hive_path (psid, path))
>      {
>        strcat (path, "\\NTUSER.DAT");

Ok with me.
> 
> 9) get_dacl() (in security.cc) gives no access to admins if the
> user is not in the admins group. I don't understand the logic.

It's Windows logic.  This is how a default security descriptor
is created.

> My suggestion is to call instead the new sec_dacl() (see above),
> which always has system, admins, sid1, [sid2] and creator/owner.

Why?  What's the effect you want to get by this?

> 11) Can cygwin_logon_user() be called by a user not in admins?

Yep.  Don't think in groups here, it just depends on the privileges
set for the user in the security policy.  Up to W2K the user needs
the SE_TCB_NAME privilege ("Act as part of the operating system").
The admin group doesn't have that permissions either (by default).
Since XP, this restriction has been dropped, fortunately.  But that
only helps applications which are never intended to run on OSes
prior to XP.

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/

Re: More security issues

[Pierre A. Humblet]

Corinna Vinschen wrote:
> 
> I don't understand that description.  Could you try to explain
> in other words?  What do you mean by "natural group"?  Primary
> group as set by Windows (RID 513, "None" or "Domain Users",
> typically) or the primary group as set in /etc/passwd or ...?

When an internal token is created, groups come from two sources:
1) the "natural groups" including those associated with the user
on the logon server and some of those associated with the current 
process token
2) the primarygroupsid, which in many applications is also 
a natural group but in others (e.g. mail server) isn't. 
When the primary group is not a natural group the token has more
rights that the "normal user" and caution must be exercised when 
reusing it. 

> > 4) the primary group that was used when creating an internal token
> > is now saved in the token sd. This allows to set the token default
> 
> With what access rights?  The pgrp shouldn't have write access
> to the token's sd, isn't it?
I agree, it (usually) has no access. It's simply used as a 
storage spot.
 
> > primary group appropriately if the user calls setegid() after
> > creating the token, e.g. seteuid(uid1) .... setegid(gid1) ....
> > [can this order be legitimate ?].
> 
> That won't work due to the create_token call being only in
> seteuid().  This would require a redesign.

It works when gid1 is a "natural group". I am just trying to insure
that it works in as many reasonable cases as possible.

> > 9) get_dacl() (in security.cc) gives no access to admins if the
> > user is not in the admins group. I don't understand the logic.
> 
> It's Windows logic.  This is how a default security descriptor
> is created.
> 
> > My suggestion is to call instead the new sec_dacl() (see above),
> > which always has system, admins, sid1, [sid2] and creator/owner.
> 
> Why?  What's the effect you want to get by this?
Adherence to the policy that admins always has access, as in sec_user().
Actually it's not a good enough reason, I won't do it. 
Also allows code reuse, but this can be done easily enough.

> > 11) Can cygwin_logon_user() be called by a user not in admins?
> 
> Yep.  Don't think in groups here, it just depends on the privileges
> set for the user in the security policy.  Up to W2K the user needs

I fully agree, so I just created a local account, no admins, just TCB + 
IncQuota + ReplaceToken (just in case...). Token creation and impersonation
go fine (with the standard cygwin1.dll). However when forking I hit
Impersonate for forked child failed: 1307 
I will investigate.

Pierre

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