Re: More security issues

["Pierre A. Humblet" <Pierre.Humblet@ieee.org>]

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/