* More security issues @ 2002-02-10 11:34 Pierre A. Humblet 2002-02-12 7:57 ` Corinna Vinschen 0 siblings, 1 reply; 10+ messages in thread From: Pierre A. Humblet @ 2002-02-10 11:34 UTC (permalink / raw) To: Corinna Vinschen 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-10 11:34 More security issues Pierre A. Humblet @ 2002-02-12 7:57 ` Corinna Vinschen 2002-02-13 0:41 ` Pierre A. Humblet 2002-02-13 12:50 ` Pierre A. Humblet 0 siblings, 2 replies; 10+ messages in thread From: Corinna Vinschen @ 2002-02-12 7:57 UTC (permalink / raw) To: Pierre A. Humblet; +Cc: cygwin 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-12 7:57 ` Corinna Vinschen @ 2002-02-13 0:41 ` Pierre A. Humblet 2002-02-13 12:50 ` Pierre A. Humblet 1 sibling, 0 replies; 10+ messages in thread From: Pierre A. Humblet @ 2002-02-13 0:41 UTC (permalink / raw) To: Corinna Vinschen [-- 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-12 7:57 ` Corinna Vinschen 2002-02-13 0:41 ` Pierre A. Humblet @ 2002-02-13 12:50 ` Pierre A. Humblet 2002-02-14 1:13 ` Corinna Vinschen 1 sibling, 1 reply; 10+ messages in thread From: Pierre A. Humblet @ 2002-02-13 12:50 UTC (permalink / raw) To: Corinna Vinschen 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-13 12:50 ` Pierre A. Humblet @ 2002-02-14 1:13 ` Corinna Vinschen 2002-02-22 20:41 ` Pierre A. Humblet 0 siblings, 1 reply; 10+ messages in thread From: Corinna Vinschen @ 2002-02-14 1:13 UTC (permalink / raw) To: Pierre A. Humblet; +Cc: cygwin 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-14 1:13 ` Corinna Vinschen @ 2002-02-22 20:41 ` Pierre A. Humblet 2002-02-23 15:54 ` Corinna Vinschen 0 siblings, 1 reply; 10+ messages in thread From: Pierre A. Humblet @ 2002-02-22 20:41 UTC (permalink / raw) To: Corinna Vinschen 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-22 20:41 ` Pierre A. Humblet @ 2002-02-23 15:54 ` Corinna Vinschen 2002-03-03 19:05 ` Pierre A. Humblet 0 siblings, 1 reply; 10+ messages in thread From: Corinna Vinschen @ 2002-02-23 15:54 UTC (permalink / raw) To: Pierre A. Humblet; +Cc: cygwin 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-02-23 15:54 ` Corinna Vinschen @ 2002-03-03 19:05 ` Pierre A. Humblet 2002-03-05 0:57 ` Corinna Vinschen 0 siblings, 1 reply; 10+ messages in thread From: Pierre A. Humblet @ 2002-03-03 19:05 UTC (permalink / raw) To: Corinna Vinschen 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-03-03 19:05 ` Pierre A. Humblet @ 2002-03-05 0:57 ` Corinna Vinschen 2002-03-05 11:21 ` Pierre A. Humblet 0 siblings, 1 reply; 10+ messages in thread From: Corinna Vinschen @ 2002-03-05 0:57 UTC (permalink / raw) To: Pierre A. Humblet; +Cc: cygwin 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: More security issues 2002-03-05 0:57 ` Corinna Vinschen @ 2002-03-05 11:21 ` Pierre A. Humblet 0 siblings, 0 replies; 10+ messages in thread From: Pierre A. Humblet @ 2002-03-05 11:21 UTC (permalink / raw) To: Corinna Vinschen 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2002-03-05 19:21 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2002-02-10 11:34 More security issues Pierre A. Humblet 2002-02-12 7:57 ` Corinna Vinschen 2002-02-13 0:41 ` Pierre A. Humblet 2002-02-13 12:50 ` Pierre A. Humblet 2002-02-14 1:13 ` Corinna Vinschen 2002-02-22 20:41 ` Pierre A. Humblet 2002-02-23 15:54 ` Corinna Vinschen 2002-03-03 19:05 ` Pierre A. Humblet 2002-03-05 0:57 ` Corinna Vinschen 2002-03-05 11:21 ` Pierre A. Humblet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).