[Patch] Allow to disable root privileges with CYGWIN=noroot

[Christian Franke]

For members of administrator group, Cygwin runs with root access rights.
Cygwin enables the Windows backup and restore privileges which are not
enabled by default.

This is IMO not desirable under all circumstances.

This patch adds a new flag to the Cygwin environment variable.
If 'CYGWIN=noroot' is set, the extra privileges are removed after Cygwin
startup.

This allows to run a Cygwin shell with the same default access rights as
cmd or explorer.


Testcase:

$ cd /cygdrive/c

$ ls -l 'System Volume Information'
----rwx---  1 root SYSTEM   0 Feb 22  2009 MountPointManagerRemoteDatabase
...

$ CYGWIN=noroot ls -l 'System Volume Information'
ls: cannot open directory System Volume Information: Permission denied


I'm sure there is something missing, so no changelog for now :-)

Christian


diff --git a/winsup/cygwin/environ.cc b/winsup/cygwin/environ.cc
index d4e003f..eff982d 100644
--- a/winsup/cygwin/environ.cc
+++ b/winsup/cygwin/environ.cc
@@ -33,6 +33,7 @@ details. */
 extern bool dos_file_warning;
 extern bool ignore_case_with_glob;
 extern bool allow_winsymlinks;
+extern bool use_root_privileges;
 bool reset_com = false;
 static bool envcache = true;
 static bool create_upcaseenv = false;
@@ -565,6 +566,23 @@ set_proc_retry (const char *buf)
   child_info::retry_count = strtoul (buf, NULL, 0);
 }
 
+static void
+set_root_mode (const char *buf)
+{
+  bool old_state = use_root_privileges;
+
+  if (!buf || !*buf)
+    use_root_privileges = false;
+  else
+    use_root_privileges = true;
+
+  if (use_root_privileges != old_state)
+    {
+      OpenProcessToken (hMainProc, MAXIMUM_ALLOWED, &hProcToken);
+      set_cygwin_privileges (hProcToken);
+    }
+}
+
 /* The structure below is used to set up an array which is used to
    parse the CYGWIN environment variable or, if enabled, options from
    the registry.  */
@@ -596,6 +614,7 @@ static struct parse_thing
   {"glob", {func: &glob_init}, isfunc, NULL, {{0}, {s: "normal"}}},
   {"proc_retry", {func: set_proc_retry}, isfunc, NULL, {{0}, {5}}},
   {"reset_com", {&reset_com}, justset, NULL, {{false}, {true}}},
+  {"root", {func: &set_root_mode}, isfunc, NULL, {{s:NULL}, {s:"yes"}}},
   {"strip_title", {&strip_title_path}, justset, NULL, {{false}, {true}}},
   {"title", {&display_title}, justset, NULL, {{false}, {true}}},
   {"tty", {NULL}, set_process_state, NULL, {{0}, {PID_USETTY}}},
diff --git a/winsup/cygwin/sec_helper.cc b/winsup/cygwin/sec_helper.cc
index 63be25c..c567dfd 100644
--- a/winsup/cygwin/sec_helper.cc
+++ b/winsup/cygwin/sec_helper.cc
@@ -406,15 +406,22 @@ out:
   return ret;
 }
 
+/* This may be set to false by 'CYGWIN=noroot' or by parent process
+   through fork().  */
+bool use_root_privileges = true;
+
 /* This is called very early in process initialization.  The code must
-   not depend on anything. */
+   not depend on anything.
+   Note: During fork() this is called twice:
+   first with use_root_privileges = always_true,
+   then with  use_root_privileges = inherited_from_parent.  */
 void
 set_cygwin_privileges (HANDLE token)
 {
-  set_privilege (token, SE_RESTORE_PRIVILEGE, true);
-  set_privilege (token, SE_BACKUP_PRIVILEGE, true);
+  set_privilege (token, SE_RESTORE_PRIVILEGE, use_root_privileges);
+  set_privilege (token, SE_BACKUP_PRIVILEGE, use_root_privileges);
   if (wincap.has_create_global_privilege ())
-    set_privilege (token, SE_CREATE_GLOBAL_PRIVILEGE, true);
+    set_privilege (token, SE_CREATE_GLOBAL_PRIVILEGE, use_root_privileges);
 }
 
 /* Function to return a common SECURITY_DESCRIPTOR that

[Corinna Vinschen-2]

On Aug 29 16:04, Christian Franke wrote:

> For members of administrator group, Cygwin runs with root access rights.  
> Cygwin enables the Windows backup and restore privileges which are not  
> enabled by default.
>
> This is IMO not desirable under all circumstances.
>
> This patch adds a new flag to the Cygwin environment variable.
> If 'CYGWIN=noroot' is set, the extra privileges are removed after Cygwin  
> startup.
>
> This allows to run a Cygwin shell with the same default access rights as  
> cmd or explorer.

I don't like the idea of the patch for three reasons:

- It adds a new CYGWIN setting and
- It adds a new CYGWIN setting which is redundant on all systems
  supporting UAC.  Either you're running in a default shell with
  restricted rights, which means, you don't have admin privileges
  anyway, or you're runnning in a admin shell and you very likely want
  all rights you can get as admin.
- On all older systems you shouldn't work as admin by default anyway,
  especially not on Windows XP.  And then, *if* you're running an admin
  session, you usually want admin rights.  What's the advantage of
  faking you don't have these rights?


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Christian Franke]

Corinna Vinschen wrote:
> - On all older systems you shouldn't work as admin by default anyway,
>   especially not on Windows XP.  And then, *if* you're running an admin
>   session, you usually want admin rights.  What's the advantage of
>   faking you don't have these rights?
>
>  

*If* running an admin session, I expect (Windows) admin rights:
- Access restrictions from ACLs are effective.
- Further rights can be obtained if desired by
-- changing ACLs
-- disabling ACL check via backup/restore privileges (which
unfortunately cannot be inherited to child processes).

This is not equivalent with (Unix) root rights, which means
- No access restrictions apply, period.

Of course this makes no difference for malware.
But it IMO makes a practical difference if an admin runs Cygwin apps.

The patch give the user the ability to run Cygwin with the default admin
rights. These are also effective for explorer, cmd.exe and all other
Windows apps which do not set backup/restore privileges.

Christian

[Corinna Vinschen-2]

On Aug 29 23:33, Christian Franke wrote:

> Corinna Vinschen wrote:
>> - On all older systems you shouldn't work as admin by default anyway,
>>   especially not on Windows XP.  And then, *if* you're running an admin
>>   session, you usually want admin rights.  What's the advantage of  
>> faking you don't have these rights?
>>
>>  
>
> *If* running an admin session, I expect (Windows) admin rights:
> - Access restrictions from ACLs are effective.
> - Further rights can be obtained if desired by
> -- changing ACLs
> -- disabling ACL check via backup/restore privileges (which  
> unfortunately cannot be inherited to child processes).
>
> This is not equivalent with (Unix) root rights, which means
> - No access restrictions apply, period.
>
> Of course this makes no difference for malware.
> But it IMO makes a practical difference if an admin runs Cygwin apps.

But *why*?  What is the pratical difference, except that you take away
rights from your Cygwin app which in turn has no POSIX way to re-enable
these rights?  I don't see any real advantage.

If you plan to run a Cygwin application with restricted rights from your
administrative account, the IMHO right way would be to start the Cygwin
application through another application which creates a *really*
restricted user token using the Win32 function CreateRestrictedToken and
then call cygwin_set_impersonation_token/execv to start the restricted
process.  A Cygwin tool which accomplishes that would be much more
useful and much more generic than this patch, IMHO.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Christian Franke]

Corinna Vinschen wrote:
> If you plan to run a Cygwin application with restricted rights from your
> administrative account, the IMHO right way would be to start the Cygwin
> application through another application which creates a *really*
> restricted user token using the Win32 function CreateRestrictedToken and
> then call cygwin_set_impersonation_token/execv to start the restricted
> process.  A Cygwin tool which accomplishes that would be much more
> useful and much more generic than this patch, IMHO.
>
>  
I agree, let's forget the patch.

But I'm not sure how cygwin_set_impersonation_token() could be of any
help here. This function sets user.external_token which is only used in
seteuid32(). Setuid/seteuid() cannot be used because the restricted
token is not related to another user id.

A quick test with native calls works for me:

  HANDLE t, rt;
  OpenProcessToken (GetCurrentProcess (), TOKEN_ALL_ACCESS, &t);
  CreateRestrictedToken (t, DISABLE_MAX_PRIVILEGE, 0, ..., 0, &rt);
  CreateProcessAsUser (rt, 0, "c:/cygwin/bin/mintty...", ...);

BTW: CreateRestrictedToken is apparently missing in
/usr/include/w32api/*.h, but it is present in libadvapi32.a

Christian

[Corinna Vinschen-2]

On Aug 30 21:38, Christian Franke wrote:

> Corinna Vinschen wrote:
>> If you plan to run a Cygwin application with restricted rights from your
>> administrative account, the IMHO right way would be to start the Cygwin
>> application through another application which creates a *really*
>> restricted user token using the Win32 function CreateRestrictedToken and
>> then call cygwin_set_impersonation_token/execv to start the restricted
>> process.  A Cygwin tool which accomplishes that would be much more
>> useful and much more generic than this patch, IMHO.
>>
>>  
> I agree, let's forget the patch.
>
> But I'm not sure how cygwin_set_impersonation_token() could be of any  
> help here. This function sets user.external_token which is only used in  
> seteuid32(). Setuid/seteuid() cannot be used because the restricted  
> token is not related to another user id.

I had a quick look into the seteuid code and I see the problem.  I don't
see a quick way around it, unfortunately.  I'll have a deeper look into
it when I'm back from vacation.

> A quick test with native calls works for me:
>
>  HANDLE t, rt;
>  OpenProcessToken (GetCurrentProcess (), TOKEN_ALL_ACCESS, &t);
>  CreateRestrictedToken (t, DISABLE_MAX_PRIVILEGE, 0, ..., 0, &rt);
>  CreateProcessAsUser (rt, 0, "c:/cygwin/bin/mintty...", ...);

Cool.  Some stuff in the child won't work though since the entire
exec(2) magic is missing.

> BTW: CreateRestrictedToken is apparently missing in  
> /usr/include/w32api/*.h, but it is present in libadvapi32.a

PTC.  The w32api files always need a lot of work.  Microsoft adds
stuff with every new OS release.  It's hard to stay on top.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Corinna Vinschen-2]

Hi Christian,

On Sep  1 20:32, Corinna Vinschen wrote:

> On Aug 30 21:38, Christian Franke wrote:
> > Corinna Vinschen wrote:
> >> If you plan to run a Cygwin application with restricted rights from your
> >> administrative account, the IMHO right way would be to start the Cygwin
> >> application through another application which creates a *really*
> >> restricted user token using the Win32 function CreateRestrictedToken and
> >> then call cygwin_set_impersonation_token/execv to start the restricted
> >> process.  A Cygwin tool which accomplishes that would be much more
> >> useful and much more generic than this patch, IMHO.
> >>
> >>  
> > I agree, let's forget the patch.
> >
> > But I'm not sure how cygwin_set_impersonation_token() could be of any  
> > help here. This function sets user.external_token which is only used in  
> > seteuid32(). Setuid/seteuid() cannot be used because the restricted  
> > token is not related to another user id.
>
> I had a quick look into the seteuid code and I see the problem.  I don't
> see a quick way around it, unfortunately.  I'll have a deeper look into
> it when I'm back from vacation.

It took some time, but today it occured to me how we could solve this
issue.

The functionality we're talking about is not creating a new token from
scratch, like an external call to LogonUser would create.  It's just
creating a restricted token for the same user, and fortunately there's a
function IsTokenRestricted() in the Win32 API, available since Windows
2000.

However, we can't just simply test for a restricted token since
restricted tokens are used a lot under UAC starting with Windows Vista.
Therefore the idea is that a restricted token is only recogized as a
special case if you call setuid(getuid()) or seteuid(geteuid()).

So, to use a restricted token, you call code along these lines:

  restricted_token = CreateRestrictedToken();
  /* Switch to restricted token. */
  cygwin_set_impersonation_token (restricted_token);
  setuid (getuid ());
  /* Do stuff with restricted rights */
  [...]
  /* Revert process to original token. */
  cygwin_set_impersonation_token (INVALID_HANDLE_VALUE);
  setuid (getuid ());

I created a patch to seteuid32 which compiles fine.  It just needs
testing.  Since you're interested in this functionality in the first
place, I thought you might consider to give this a nice, thorough test.

Patch attached.  For simplicity I just applied the patch to the w32api
winbase.h header file which defines CreateRestrictedToken and
IsTokenRestricted.


Thanks,
Corinna


        * autoload.cc (IsTokenRestricted): Define.
        * syscalls.cc (seteuid32): Create special case for restricted
        external tokens to switch to a restricted token for the current
        user.


Index: autoload.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/autoload.cc,v
retrieving revision 1.165
diff -u -p -r1.165 autoload.cc
--- autoload.cc 22 Sep 2009 14:27:57 -0000 1.165
+++ autoload.cc 4 Oct 2009 12:24:56 -0000
@@ -422,6 +422,8 @@ LoadDLLfuncEx (GetSystemDEPPolicy, 0, ke
 LoadDLLfuncEx (GetProcessDEPPolicy, 12, kernel32, 1)
 LoadDLLfuncEx (SetProcessDEPPolicy, 4, kernel32, 1)
 
+LoadDLLfunc (IsTokenRestricted, 4, advapi32)
+
 LoadDLLfunc (SHGetDesktopFolder, 4, shell32)
 
 LoadDLLfuncEx (waveOutGetNumDevs, 0, winmm, 1)
Index: syscalls.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/syscalls.cc,v
retrieving revision 1.540
diff -u -p -r1.540 syscalls.cc
--- syscalls.cc 4 Oct 2009 11:32:07 -0000 1.540
+++ syscalls.cc 4 Oct 2009 12:24:57 -0000
@@ -2664,7 +2664,30 @@ seteuid32 (__uid32_t uid)
   debug_printf ("uid: %u myself->uid: %u myself->gid: %u",
  uid, myself->uid, myself->gid);
 
-  if (uid == myself->uid && !cygheap->user.groups.ischanged)
+  /* Same uid as we're just running under is usually a no-op.
+  
+     Except we have an external token which is a restricted token.  Or,
+     the external token is NULL, but the current impersonation token is
+     a restricted token.  This allows to restrict user rights temporarily
+     like this:
+
+       cygwin_set_impersonation_token (restricted_token);
+       setuid (getuid ());
+       [...do stuff with restricted rights...]
+       cygwin_set_impersonation_token (INVALID_HANDLE_VALUE);
+       setuid (getuid ());
+
+    Note that using the current uid is a requirement!  Starting with Windows
+    Vista, we have restricted tokens galore (UAC), so this is really just
+    a special case to restict your own processes to lesser rights. */
+  bool request_restricted_uid_switch =
+     uid == myself->uid
+     && (   (cygheap->user.external_token != NO_IMPERSONATION
+     && IsTokenRestricted (cygheap->user.external_token))
+ || (cygheap->user.external_token == NO_IMPERSONATION
+     && IsTokenRestricted (cygheap->user.curr_primary_token)));
+  if (uid == myself->uid && !cygheap->user.groups.ischanged
+      && !request_restricted_uid_switch)
     {
       debug_printf ("Nothing happens");
       return 0;
@@ -2686,6 +2709,21 @@ seteuid32 (__uid32_t uid)
   cygheap->user.deimpersonate ();
 
   /* Verify if the process token is suitable. */
+  /* First of all, skip all checks if a switch to a restricted token has been
+     requested, or if trying to switch back from it. */
+  if (request_restricted_uid_switch)
+    {
+      if (cygheap->user.external_token != NO_IMPERSONATION)
+       {
+  debug_printf ("Switch to restricted token");
+  new_token = cygheap->user.external_token;
+ }
+      else
+       {
+  debug_printf ("Switch back from restricted token");
+  new_token = hProcToken;
+ }
+    }
   /* TODO, CV 2008-11-25: The check against saved_sid is a kludge and a
      shortcut.  We must check if it's really feasible in the long run.
      The reason to add this shortcut is this:  sshd switches back to the


--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Corinna Vinschen-2]

On Oct  4 14:30, Corinna Vinschen wrote:

> [...]
> Patch attached.  For simplicity I just applied the patch to the w32api
> winbase.h header file which defines CreateRestrictedToken and
> IsTokenRestricted.
>
>
> Thanks,
> Corinna
>
>
> * autoload.cc (IsTokenRestricted): Define.
> * syscalls.cc (seteuid32): Create special case for restricted
> external tokens to switch to a restricted token for the current
> user.

New patch attached.  I made the test a bit more foolproof, hopefully.
And a restricted token does not require to load the user's registry hive,
nor should Cygwin try to enable the backup/restore permissions in the
new token.  That spoils the idea of a restricted token a bit...


Corinna


Index: autoload.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/autoload.cc,v
retrieving revision 1.165
diff -u -p -r1.165 autoload.cc
--- autoload.cc 22 Sep 2009 14:27:57 -0000 1.165
+++ autoload.cc 4 Oct 2009 12:54:15 -0000
@@ -422,6 +422,8 @@ LoadDLLfuncEx (GetSystemDEPPolicy, 0, ke
 LoadDLLfuncEx (GetProcessDEPPolicy, 12, kernel32, 1)
 LoadDLLfuncEx (SetProcessDEPPolicy, 4, kernel32, 1)
 
+LoadDLLfunc (IsTokenRestricted, 4, advapi32)
+
 LoadDLLfunc (SHGetDesktopFolder, 4, shell32)
 
 LoadDLLfuncEx (waveOutGetNumDevs, 0, winmm, 1)
Index: syscalls.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/syscalls.cc,v
retrieving revision 1.540
diff -u -p -r1.540 syscalls.cc
--- syscalls.cc 4 Oct 2009 11:32:07 -0000 1.540
+++ syscalls.cc 4 Oct 2009 12:54:17 -0000
@@ -2664,7 +2664,31 @@ seteuid32 (__uid32_t uid)
   debug_printf ("uid: %u myself->uid: %u myself->gid: %u",
  uid, myself->uid, myself->gid);
 
-  if (uid == myself->uid && !cygheap->user.groups.ischanged)
+  /* Same uid as we're just running under is usually a no-op.
+  
+     Except we have an external token which is a restricted token.  Or,
+     the external token is NULL, but the current impersonation token is
+     a restricted token.  This allows to restrict user rights temporarily
+     like this:
+
+       cygwin_set_impersonation_token (restricted_token);
+       setuid (getuid ());
+       [...do stuff with restricted rights...]
+       cygwin_set_impersonation_token (INVALID_HANDLE_VALUE);
+       setuid (getuid ());
+
+    Note that using the current uid is a requirement!  Starting with Windows
+    Vista, we have restricted tokens galore (UAC), so this is really just
+    a special case to restict your own processes to lesser rights. */
+  bool request_restricted_uid_switch =
+     uid == myself->uid
+     && (   (cygheap->user.external_token != NO_IMPERSONATION
+     && IsTokenRestricted (cygheap->user.external_token))
+ || (cygheap->user.external_token == NO_IMPERSONATION
+     && cygheap->user.issetuid ()
+     && IsTokenRestricted (cygheap->user.curr_primary_token)));
+  if (uid == myself->uid && !cygheap->user.groups.ischanged
+      && !request_restricted_uid_switch)
     {
       debug_printf ("Nothing happens");
       return 0;
@@ -2686,6 +2710,21 @@ seteuid32 (__uid32_t uid)
   cygheap->user.deimpersonate ();
 
   /* Verify if the process token is suitable. */
+  /* First of all, skip all checks if a switch to a restricted token has been
+     requested, or if trying to switch back from it. */
+  if (request_restricted_uid_switch)
+    {
+      if (cygheap->user.external_token != NO_IMPERSONATION)
+       {
+  debug_printf ("Switch to restricted token");
+  new_token = cygheap->user.external_token;
+ }
+      else
+       {
+  debug_printf ("Switch back from restricted token");
+  new_token = hProcToken;
+ }
+    }
   /* TODO, CV 2008-11-25: The check against saved_sid is a kludge and a
      shortcut.  We must check if it's really feasible in the long run.
      The reason to add this shortcut is this:  sshd switches back to the
@@ -2763,9 +2802,12 @@ seteuid32 (__uid32_t uid)
 
   if (new_token != hProcToken)
     {
-      /* Avoid having HKCU use default user */
-      WCHAR name[128];
-      load_registry_hive (usersid.string (name));
+      if (!request_restricted_uid_switch)
+ {
+  /* Avoid having HKCU use default user */
+  WCHAR name[128];
+  load_registry_hive (usersid.string (name));
+ }
 
       /* Try setting owner to same value as user. */
       if (!SetTokenInformation (new_token, TokenOwner,
@@ -2805,7 +2847,8 @@ seteuid32 (__uid32_t uid)
   cygheap->user.curr_primary_token = NO_IMPERSONATION;
   return -1;
  }
-      set_cygwin_privileges (cygheap->user.curr_imp_token);
+      if (!request_restricted_uid_switch)
+ set_cygwin_privileges (cygheap->user.curr_imp_token);
     }
   if (!cygheap->user.reimpersonate ())
     {

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Christian Franke]

Hi Corinna,

Corinna Vinschen wrote:
> New patch attached.  I made the test a bit more foolproof, hopefully.
> And a restricted token does not require to load the user's registry hive,
> nor should Cygwin try to enable the backup/restore permissions in the
> new token.  That spoils the idea of a restricted token a bit...
> ...
>
>  

Thanks!

> +  bool request_restricted_uid_switch =
> +     uid == myself->uid
> +     && (   (cygheap->user.external_token != NO_IMPERSONATION
> +     && IsTokenRestricted (cygheap->user.external_token))
> + || (cygheap->user.external_token == NO_IMPERSONATION
> +     && cygheap->user.issetuid ()
> +     && IsTokenRestricted (cygheap->user.curr_primary_token)));
>  


Unfortunately this does not work for a typical use case: an admin
process creates a restricted token with standard user rights. The
function IsTokenRestricted() returns TRUE only if the token contains
'restricted SIDs'.
(http://msdn.microsoft.com/en-us/library/aa379137(VS.85).aspx)

Test with tokens returned by SaferComputeTokenFromLevel():
(http://msdn.microsoft.com/en-us/library/ms972827.aspx)

SAFER_LEVELID_NORMALUSER:  IsTokenRestricted()=FALSE
SAFER_LEVELID_CONSTRAINED: IsTokenRestricted()=TRUE
SAFER_LEVELID_UNTRUSTED:   IsTokenRestricted()=TRUE

BTW: Only NORMALUSER is works for Cygwin. Using DropMyRights.exe to
start of a Cygwin process with a CONTRAINED token results in:

5 [sig] true 3788 C:\cygwin-1.7\bin\true.exe:
   *** fatal error - couldn't create signal pipe, Win32 error 5


There is apparently no function to check whether a token is a result of
CreateRestrictedToken() or SaferComputeTokenFromLevel().

Would'nt it be easier to add a new function
'cygwin_set_restricted_token(token)' instead of the test of the token type?

Christian

[Corinna Vinschen-2]

On Oct  4 21:08, Christian Franke wrote:
> Hi Corinna,
>[...]
> Unfortunately this does not work for a typical use case: an admin process
> creates a restricted token with standard user rights. The function
> IsTokenRestricted() returns TRUE only if the token contains 'restricted
> SIDs'.
> (http://msdn.microsoft.com/en-us/library/aa379137(VS.85).aspx)

Bummer.

> There is apparently no function to check whether a token is a result of
> CreateRestrictedToken() or SaferComputeTokenFromLevel().
>
> Would'nt it be easier to add a new function
> 'cygwin_set_restricted_token(token)' instead of the test of the token type?

The idea was to avoid another non-standard system call.  Maybe you're
right, but we should create another cygwin_internal call instead, like,
say,

  cygwin_internal (CW_SET_RESTRICTED_TOKEN, token_handle);

Since you have a copyright assignment in place anyway, would you like
to do that and change the seteuid32 call accordingly?  A bool value in
cygheap->user could store the fact that the current external token is
a restricted token.  That would simplify the seteuid32 extension
enormously.


Thanks,
Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Corinna Vinschen-2]

On Oct  4 21:57, Corinna Vinschen wrote:

> On Oct  4 21:08, Christian Franke wrote:
> > Hi Corinna,
> >[...]
> > Unfortunately this does not work for a typical use case: an admin process
> > creates a restricted token with standard user rights. The function
> > IsTokenRestricted() returns TRUE only if the token contains 'restricted
> > SIDs'.
> > (http://msdn.microsoft.com/en-us/library/aa379137(VS.85).aspx)
>
> Bummer.
>
> > There is apparently no function to check whether a token is a result of
> > CreateRestrictedToken() or SaferComputeTokenFromLevel().
> >
> > Would'nt it be easier to add a new function
> > 'cygwin_set_restricted_token(token)' instead of the test of the token type?
>
> The idea was to avoid another non-standard system call.  Maybe you're
> right, but we should create another cygwin_internal call instead, like,
> say,
>
>   cygwin_internal (CW_SET_RESTRICTED_TOKEN, token_handle);

...and maybe it's time to create a cygwin_internal call which replaces
cygwin_set_impersonation_token and deprecate cygwin_set_impersonation_token
in the long run.  So, instead of the above we could have this call
taking a HANDLE and a BOOL value:

  cygwin_internal (CW_SET_EXTERNAL_TOKEN, token_handle, restricted?);


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Christian Franke]

Corinna Vinschen wrote:
> ...and maybe it's time to create a cygwin_internal call which replaces
> cygwin_set_impersonation_token and deprecate cygwin_set_impersonation_token
> in the long run.  So, instead of the above we could have this call
> taking a HANDLE and a BOOL value:
>
>   cygwin_internal (CW_SET_EXTERNAL_TOKEN, token_handle, restricted?);
>
>  

OK.

I have a very first experimental version which works for me. It also
requires a new flag 'cygheap->user.is_restricted_token' to tell
spawn_guts() to use CreateProcessAsUser().

I will post the patch in a few days.

A question:

Why does seteuid32() call 'set_cygwin_privileges ()' on 'curr_imp_token'
and not on 'curr_primary_token' ? The curr_primary_token is used for
impersonation and therefore the privileges are not set for the thread
itself.

Christian

[Corinna Vinschen-2]

On Oct  6 22:15, Christian Franke wrote:

> Corinna Vinschen wrote:
>> ...and maybe it's time to create a cygwin_internal call which replaces
>> cygwin_set_impersonation_token and deprecate cygwin_set_impersonation_token
>> in the long run.  So, instead of the above we could have this call
>> taking a HANDLE and a BOOL value:
>>
>>   cygwin_internal (CW_SET_EXTERNAL_TOKEN, token_handle, restricted?);
>>
>>  
>
> OK.
>
> I have a very first experimental version which works for me. It also
> requires a new flag 'cygheap->user.is_restricted_token' to tell
> spawn_guts() to use CreateProcessAsUser().
>
> I will post the patch in a few days.
>
> A question:
>
> Why does seteuid32() call 'set_cygwin_privileges ()' on 'curr_imp_token'
> and not on 'curr_primary_token' ? The curr_primary_token is used for
> impersonation and therefore the privileges are not set for the thread
> itself.

Oops.  Thanks for catching.  I applied a patch.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Christian Franke]

Corinna Vinschen wrote:
> ...and maybe it's time to create a cygwin_internal call which replaces
> cygwin_set_impersonation_token and deprecate cygwin_set_impersonation_token
> in the long run.  So, instead of the above we could have this call
> taking a HANDLE and a BOOL value:
>
>   cygwin_internal (CW_SET_EXTERNAL_TOKEN, token_handle, restricted?);
>
>
>  

Attached a patch (based on your patch) which works for me on XP SP3.

Note that unlike with setuid(other_uid), fork() and exec() do not fail
for non-system processes.


A simple Testcase: Exec with most privileges removed:

int main(int argc, char **argv)
{
  if (argc < 2) {
    printf("Usage: %s command args ...\n", argv[0]); return 1;
  }

  HANDLE pt, rt;
  if (!OpenProcessToken(GetCurrentProcess (), TOKEN_ALL_ACCESS, &pt)) {
    printf("OpenProcessToken failed\n"); return 1;
  }
  if (!CreateRestrictedToken(pt, DISABLE_MAX_PRIVILEGE,
      0, (PSID_AND_ATTRIBUTES)0, 0, (PLUID_AND_ATTRIBUTES)0,
      0, (PSID_AND_ATTRIBUTES)0, &rt)) {
    printf("CreateRestrictedToken failed\n"); return 1;
  }
  if (!SetHandleInformation(rt, HANDLE_FLAG_INHERIT,
      HANDLE_FLAG_INHERIT)) {
    printf("SetHandleInformation failed\n"); return 1;
  }

  cygwin_internal(CW_SET_EXTERNAL_TOKEN, rt, CW_TOKEN_RESTRICTED);

  // seteuid(getuid()) would allow child to revert to original privileges.
  setuid(getuid());

  execvp(argv[1], argv+1);
  perror("exec");
  return 1;
}

Running e.g. 'ls /proc/registry/HKEY_LOCAL_MACHINE/SECURITY' from an
admin with and without the above program shows the difference.
(The process is not really restricted because the admin group is not
removed :-)


I would suggest to add another cygwin_internal() call to check if
current process is considered 'equivalent root'. This could be used e.g.
by shells to set the root prompt properly.
http://sourceware.org/ml/cygwin/2009-09/msg00138.html

Christian

2009-10-09  Christian Franke  <franke@...>
            Corinna Vinschen  <corinna@...>

        * include/sys/cygwin.h: Add new cygwin_getinfo_type
        CW_SET_EXTERNAL_TOKEN.
        Add new enum CW_TOKEN_IMPERSONATION, CW_TOKEN_RESTRICTED.
        * cygheap.h (cyguser): New flags ext_token_is_restricted,
        curr_token_is_restricted and setuid_to_restricted.
        * external.cc (cygwin_internal): Add CW_SET_EXTERNAL_TOKEN.
        * fork.cc (frok::child): Abort if reimpersonate fails.
        * sec_auth.cc (cygwin_set_impersonation_token): Set
        ext_token_is_restricted flag.
        * spawn.cc (spawn_guts): Use CreateProcessAsUserW if
        restricted token was enabled by setuid ().
        Do not create new window station in this case.
        * syscalls.cc (seteuid32): Add handling of restricted
        external tokens.
        (setuid32): Set setuid_to_restricted flag.
        * uinfo.cc (uinfo_init): Do not reimpersonate if
        restricted token was enabled by setuid ().
        Abort if reimpersonate fails.
        Initialize user.*_restricted flags.



diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h
index 7a39de6..b8ffd3f 100644
--- a/winsup/cygwin/cygheap.h
+++ b/winsup/cygwin/cygheap.h
@@ -108,6 +108,9 @@ public:
   HANDLE internal_token;
   HANDLE curr_primary_token;
   HANDLE curr_imp_token;
+  bool ext_token_is_restricted;  /* external_token is restricted token */
+  bool curr_token_is_restricted; /* curr_primary_token is restricted token */
+  bool setuid_to_restricted;     /* switch to restricted token by setuid () */
 
   /* CGF 2002-06-27.  I removed the initializaton from this constructor
      since this class is always allocated statically.  That means that everything
diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
index 38b8c71..9b030c9 100644
--- a/winsup/cygwin/external.cc
+++ b/winsup/cygwin/external.cc
@@ -413,6 +413,15 @@ cygwin_internal (cygwin_getinfo_types t, ...)
   int useTerminateProcess = va_arg (arg, int);
   exit_process (status, !!useTerminateProcess); /* no return */
  }
+      case CW_SET_EXTERNAL_TOKEN:
+ {
+  HANDLE token = va_arg (arg, HANDLE);
+  int type = va_arg (arg, int);
+  cygheap->user.external_token = (token == INVALID_HANDLE_VALUE
+      ? NO_IMPERSONATION : token);
+  cygheap->user.ext_token_is_restricted = (type == CW_TOKEN_RESTRICTED);
+ }
+ break;
 
       default:
  break;
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 49ff2e1..5ae69c7 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -188,7 +188,10 @@ frok::child (volatile char * volatile here)
 
   set_cygwin_privileges (hProcToken);
   clear_procimptoken ();
-  cygheap->user.reimpersonate ();
+
+  if (!cygheap->user.reimpersonate ())
+    /* User possibly set an external token without HANDLE_FLAG_INHERIT.  */
+    api_fatal ("reimpersonate after fork failed (%d)", (int)GetLastError());
 
 #ifdef DEBUGGING
   if (GetEnvironmentVariableA ("FORKDEBUG", NULL, 0))
diff --git a/winsup/cygwin/include/sys/cygwin.h b/winsup/cygwin/include/sys/cygwin.h
index 5f38278..ce9bebf 100644
--- a/winsup/cygwin/include/sys/cygwin.h
+++ b/winsup/cygwin/include/sys/cygwin.h
@@ -143,9 +143,17 @@ typedef enum
     CW_SET_DOS_FILE_WARNING,
     CW_SET_PRIV_KEY,
     CW_SETERRNO,
-    CW_EXIT_PROCESS
+    CW_EXIT_PROCESS,
+    CW_SET_EXTERNAL_TOKEN
   } cygwin_getinfo_types;
 
+/* Token type for CW_SET_EXTERNAL_TOKEN */
+enum
+{
+  CW_TOKEN_IMPERSONATION = 0,
+  CW_TOKEN_RESTRICTED    = 1
+};
+
 #define CW_NEXTPID 0x80000000 /* or with pid to get next one */
 unsigned long cygwin_internal (cygwin_getinfo_types, ...);
 
diff --git a/winsup/cygwin/sec_auth.cc b/winsup/cygwin/sec_auth.cc
index 028b5a8..c84a7e5 100644
--- a/winsup/cygwin/sec_auth.cc
+++ b/winsup/cygwin/sec_auth.cc
@@ -35,6 +35,7 @@ cygwin_set_impersonation_token (const HANDLE hToken)
 {
   debug_printf ("set_impersonation_token (%d)", hToken);
   cygheap->user.external_token = hToken == INVALID_HANDLE_VALUE ? NO_IMPERSONATION : hToken;
+  cygheap->user.ext_token_is_restricted = false;
 }
 
 void
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 5c86c4b..a6ac9f0 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -537,7 +537,8 @@ loop:
   if (!cygheap->user.issetuid ()
       || (cygheap->user.saved_uid == cygheap->user.real_uid
   && cygheap->user.saved_gid == cygheap->user.real_gid
-  && !cygheap->user.groups.issetgroups ()))
+  && !cygheap->user.groups.issetgroups ()
+  && !cygheap->user.setuid_to_restricted))
     {
       rc = CreateProcessW (runpath,  /* image name - with full path */
    wone_line,  /* what was passed to exec */
@@ -571,7 +572,8 @@ loop:
  risk, but we don't want to disable this behaviour for older
  OSes because it's still heavily used by some users.  They have
  been warned. */
-      if (wcscasecmp (wstname, L"WinSta0") != 0)
+      if (!cygheap->user.setuid_to_restricted
+  && wcscasecmp (wstname, L"WinSta0") != 0)
  {
   WCHAR sid[128];
 
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 1529cb6..5cfad3b 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -2664,7 +2664,28 @@ seteuid32 (__uid32_t uid)
   debug_printf ("uid: %u myself->uid: %u myself->gid: %u",
  uid, myself->uid, myself->gid);
 
-  if (uid == myself->uid && !cygheap->user.groups.ischanged)
+  /* Same uid as we're just running under is usually a no-op.
+
+     Except we have an external token which is a restricted token.  Or,
+     the external token is NULL, but the current impersonation token is
+     a restricted token.  This allows to restrict user rights temporarily
+     like this:
+
+       cygwin_internal(CW_SET_EXTERNAL_TOKEN, restricted_token,
+                       CW_TOKEN_RESTRICTED);
+       setuid (getuid ());
+       [...do stuff with restricted rights...]
+       cygwin_internal(CW_SET_EXTERNAL_TOKEN, INVALID_HANDLE_VALUE,
+                       CW_TOKEN_RESTRICTED);
+       setuid (getuid ());
+
+    Note that using the current uid is a requirement!  Starting with Windows
+    Vista, we have restricted tokens galore (UAC), so this is really just
+    a special case to restict your own processes to lesser rights. */
+  bool request_restricted_uid_switch = (uid == myself->uid
+      && cygheap->user.ext_token_is_restricted);
+  if (uid == myself->uid && !cygheap->user.groups.ischanged
+      && !request_restricted_uid_switch)
     {
       debug_printf ("Nothing happens");
       return 0;
@@ -2686,6 +2707,21 @@ seteuid32 (__uid32_t uid)
   cygheap->user.deimpersonate ();
 
   /* Verify if the process token is suitable. */
+  /* First of all, skip all checks if a switch to a restricted token has been
+     requested, or if trying to switch back from it. */
+  if (request_restricted_uid_switch)
+    {
+      if (cygheap->user.external_token != NO_IMPERSONATION)
+ {
+  debug_printf ("Switch to restricted token");
+  new_token = cygheap->user.external_token;
+ }
+      else
+ {
+  debug_printf ("Switch back from restricted token");
+  new_token = hProcToken;
+ }
+    }
   /* TODO, CV 2008-11-25: The check against saved_sid is a kludge and a
      shortcut.  We must check if it's really feasible in the long run.
      The reason to add this shortcut is this:  sshd switches back to the
@@ -2701,7 +2737,7 @@ seteuid32 (__uid32_t uid)
      Therefore we try this shortcut now.  When switching back to the
      privileged user, we probably always want a correct (aka original)
      user token for this privileged user, not only in sshd. */
-  if ((uid == cygheap->user.saved_uid && usersid == cygheap->user.saved_sid ())
+  else if ((uid == cygheap->user.saved_uid && usersid == cygheap->user.saved_sid ())
       || verify_token (hProcToken, usersid, groups))
     new_token = hProcToken;
   /* Verify if the external token is suitable */
@@ -2763,9 +2799,12 @@ seteuid32 (__uid32_t uid)
 
   if (new_token != hProcToken)
     {
-      /* Avoid having HKCU use default user */
-      WCHAR name[128];
-      load_registry_hive (usersid.string (name));
+      if (!request_restricted_uid_switch)
+ {
+  /* Avoid having HKCU use default user */
+  WCHAR name[128];
+  load_registry_hive (usersid.string (name));
+ }
 
       /* Try setting owner to same value as user. */
       if (!SetTokenInformation (new_token, TokenOwner,
@@ -2790,6 +2829,8 @@ seteuid32 (__uid32_t uid)
   cygheap->user.set_sid (usersid);
   cygheap->user.curr_primary_token = new_token == hProcToken ? NO_IMPERSONATION
  : new_token;
+  cygheap->user.curr_token_is_restricted = false;
+  cygheap->user.setuid_to_restricted = false;
   if (cygheap->user.curr_imp_token != NO_IMPERSONATION)
     {
       CloseHandle (cygheap->user.curr_imp_token);
@@ -2805,6 +2846,7 @@ seteuid32 (__uid32_t uid)
   cygheap->user.curr_primary_token = NO_IMPERSONATION;
   return -1;
  }
+      cygheap->user.curr_token_is_restricted = request_restricted_uid_switch;
       set_cygwin_privileges (cygheap->user.curr_primary_token);
       set_cygwin_privileges (cygheap->user.curr_imp_token);
     }
@@ -2835,7 +2877,11 @@ setuid32 (__uid32_t uid)
 {
   int ret = seteuid32 (uid);
   if (!ret)
-    cygheap->user.real_uid = myself->uid;
+    {
+      cygheap->user.real_uid = myself->uid;
+      /* If restricted token, forget original privileges on exec ().  */
+      cygheap->user.setuid_to_restricted = cygheap->user.curr_token_is_restricted;
+    }
   debug_printf ("real: %d, effective: %d", cygheap->user.real_uid, myself->uid);
   return ret;
 }
diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index 8adfd37..56c2f47 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -136,9 +136,12 @@ uinfo_init ()
   else if (cygheap->user.issetuid ()
    && cygheap->user.saved_uid == cygheap->user.real_uid
    && cygheap->user.saved_gid == cygheap->user.real_gid
-   && !cygheap->user.groups.issetgroups ())
+   && !cygheap->user.groups.issetgroups ()
+   && !cygheap->user.setuid_to_restricted)
     {
-      cygheap->user.reimpersonate ();
+      if (!cygheap->user.reimpersonate ())
+ /* User possibly set an external token without HANDLE_FLAG_INHERIT.  */
+ api_fatal ("reimpersonate after exec failed (%d)", (int)GetLastError());
       return;
     }
   else
@@ -150,6 +153,9 @@ uinfo_init ()
   cygheap->user.internal_token = NO_IMPERSONATION;
   cygheap->user.curr_primary_token = NO_IMPERSONATION;
   cygheap->user.curr_imp_token = NO_IMPERSONATION;
+  cygheap->user.ext_token_is_restricted = false;
+  cygheap->user.curr_token_is_restricted = false;
+  cygheap->user.setuid_to_restricted = false;
   cygheap->user.set_saved_sid (); /* Update the original sid */
   cygheap->user.reimpersonate ();
 }

[Corinna Vinschen-2]

On Oct  9 23:42, Christian Franke wrote:

> Corinna Vinschen wrote:
>> ...and maybe it's time to create a cygwin_internal call which replaces
>> cygwin_set_impersonation_token and deprecate cygwin_set_impersonation_token
>> in the long run.  So, instead of the above we could have this call
>> taking a HANDLE and a BOOL value:
>>
>>   cygwin_internal (CW_SET_EXTERNAL_TOKEN, token_handle, restricted?);
>>
>>
>>  
>
> Attached a patch (based on your patch) which works for me on XP SP3.

Thanks for the patch.  You did check that the normal setuid/seteuid
cases still work, didn't you?

> I would suggest to add another cygwin_internal() call to check if current
> process is considered 'equivalent root'. This could be used e.g. by shells
> to set the root prompt properly.
> http://sourceware.org/ml/cygwin/2009-09/msg00138.html

What's wrong with:

  for i in $(id -G);
  do
    [ $i -eq 544 ] && PS1='# '
  done

The patch looks pretty good.  I have a few remarks, though.

> diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
> index 38b8c71..9b030c9 100644
> --- a/winsup/cygwin/external.cc
> +++ b/winsup/cygwin/external.cc
> @@ -413,6 +413,15 @@ cygwin_internal (cygwin_getinfo_types t, ...)
>    int useTerminateProcess = va_arg (arg, int);
>    exit_process (status, !!useTerminateProcess); /* no return */
>   }
> +      case CW_SET_EXTERNAL_TOKEN:
> + {
> +  HANDLE token = va_arg (arg, HANDLE);
> +  int type = va_arg (arg, int);
> +  cygheap->user.external_token = (token == INVALID_HANDLE_VALUE
> +      ? NO_IMPERSONATION : token);
> +  cygheap->user.ext_token_is_restricted = (type == CW_TOKEN_RESTRICTED);
> + }
> + break;

I would like it better to have the actual functionality concentrated in
a function in sec_auth.cc.  A function `set_imp_token (HANDLE, int)'
which is called from cygwin_internal and from cygwin_set_impersonation_token,
for instance.  The debug output in cygwin_set_impersonation_token should
be moved into set_imp_token, too.  What do you think?

> diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
> index 49ff2e1..5ae69c7 100644
> --- a/winsup/cygwin/fork.cc
> +++ b/winsup/cygwin/fork.cc
> @@ -188,7 +188,10 @@ frok::child (volatile char * volatile here)
>  
>    set_cygwin_privileges (hProcToken);
>    clear_procimptoken ();
> -  cygheap->user.reimpersonate ();
> +
> +  if (!cygheap->user.reimpersonate ())
> +    /* User possibly set an external token without HANDLE_FLAG_INHERIT.  */
> +    api_fatal ("reimpersonate after fork failed (%d)", (int)GetLastError());

This bugs me.  Wouldn't it be better if we call DuplicateHandleEx
on the external token to make sure it's inheritable?  This would also
require to call CloseHandle on an existing external token if
a process calls cygwin_set_impersonation_token (INVALID_HANDLE_VALUE) or
INVALID_HANDLE_VALUE (NULL), but that's not really much extra code to
worry about.  Dropping any chance that fork or exec fail sounds worth
it, IMHO.  I'm still blushing that this never occured to me before.

> @@ -2686,6 +2707,21 @@ seteuid32 (__uid32_t uid)
>    cygheap->user.deimpersonate ();
>  
>    /* Verify if the process token is suitable. */
> +  /* First of all, skip all checks if a switch to a restricted token has been
> +     requested, or if trying to switch back from it. */
> +  if (request_restricted_uid_switch)
> +    {
> +      if (cygheap->user.external_token != NO_IMPERSONATION)
> + {
> +  debug_printf ("Switch to restricted token");
> +  new_token = cygheap->user.external_token;
> + }
> +      else
> + {
> +  debug_printf ("Switch back from restricted token");
> +  new_token = hProcToken;

This should make sense here:

          cygheap->user.ext_token_is_restricted = false;

That will make sure the seteui32 function will not do this twice if
seteuid is called the next time with uid == myself->uid.

> @@ -2701,7 +2737,7 @@ seteuid32 (__uid32_t uid)
>       Therefore we try this shortcut now.  When switching back to the
>       privileged user, we probably always want a correct (aka original)
>       user token for this privileged user, not only in sshd. */
> -  if ((uid == cygheap->user.saved_uid && usersid == cygheap->user.saved_sid ())
> +  else if ((uid == cygheap->user.saved_uid && usersid == cygheap->user.saved_sid ())

Please break up the line so that it's shorter than 80 chars.

> @@ -2790,6 +2829,8 @@ seteuid32 (__uid32_t uid)
>    cygheap->user.set_sid (usersid);
>    cygheap->user.curr_primary_token = new_token == hProcToken ? NO_IMPERSONATION
>   : new_token;
> +  cygheap->user.curr_token_is_restricted = false;
> +  cygheap->user.setuid_to_restricted = false;
>    if (cygheap->user.curr_imp_token != NO_IMPERSONATION)
>      {
>        CloseHandle (cygheap->user.curr_imp_token);
> [...]
> @@ -2835,7 +2877,11 @@ setuid32 (__uid32_t uid)
>  {
>    int ret = seteuid32 (uid);
>    if (!ret)
> -    cygheap->user.real_uid = myself->uid;
> +    {
> +      cygheap->user.real_uid = myself->uid;
> +      /* If restricted token, forget original privileges on exec ().  */
> +      cygheap->user.setuid_to_restricted = cygheap->user.curr_token_is_restricted;
> +    }

Do I miss something or is the setuid_to_restricted flag equivalent to
the curr_token_is_restricted flag anyway, and as such redundant?  If so,
it would look nice to make setuid_to_restricted an inline method instead:

  bool setuid_to_restricted () const { return curr_token_is_restricted; }


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Christian Franke]

Corinna Vinschen wrote:
> Thanks for the patch.  You did check that the normal setuid/seteuid
> cases still work, didn't you?
>
>  

Yes.

>> I would suggest to add another cygwin_internal() call to check if current
>> process is considered 'equivalent root'. This could be used e.g. by shells
>> to set the root prompt properly.
>> http://sourceware.org/ml/cygwin/2009-09/msg00138.html
>>    
>
> What's wrong with:
>
>   for i in $(id -G);
>   do
>     [ $i -eq 544 ] && PS1='# '
>   done
>
>  

Is OK, except if admin group is mapped to other gid (0?) in /etc/group.


A function that checks the token instead would IMO be helpful, e.g.:

#ifdef __CYGWIN__
  if (cygwin_internal (CW_IS_ROOT_EQUIV, flags?))
#else
  if (geteuid () == 0)
#endif



> The patch looks pretty good.  I have a few remarks, though.
>
>  

Thanks. New patch below.


> I would like it better to have the actual functionality concentrated in
> a function in sec_auth.cc.  A function `set_imp_token (HANDLE, int)'
> which is called from cygwin_internal and from cygwin_set_impersonation_token,
> for instance.  The debug output in cygwin_set_impersonation_token should
> be moved into set_imp_token, too.  What do you think?
>
>  

Good idea. Done.

>> -  cygheap->user.reimpersonate ();
>> +
>> +  if (!cygheap->user.reimpersonate ())
>> +    /* User possibly set an external token without HANDLE_FLAG_INHERIT.  */
>> +    api_fatal ("reimpersonate after fork failed (%d)", (int)GetLastError());
>>    
>
> This bugs me.  Wouldn't it be better if we call DuplicateHandleEx
> on the external token to make sure it's inheritable?  This would also
> require to call CloseHandle on an existing external token if
> a process calls cygwin_set_impersonation_token (INVALID_HANDLE_VALUE) or
> INVALID_HANDLE_VALUE (NULL), but that's not really much extra code to
> worry about.  Dropping any chance that fork or exec fail sounds worth
> it, IMHO.  I'm still blushing that this never occured to me before.
>
>  

I removed the error check and set HANDLE_FLAG_INHERIT in seteuid32().

>> @@ -2835,7 +2877,11 @@ setuid32 (__uid32_t uid)
>>  {
>>    int ret = seteuid32 (uid);
>>    if (!ret)
>> -    cygheap->user.real_uid = myself->uid;
>> +    {
>> +      cygheap->user.real_uid = myself->uid;
>> +      /* If restricted token, forget original privileges on exec ().  */
>> +      cygheap->user.setuid_to_restricted = cygheap->user.curr_token_is_restricted;
>> +    }
>>    
>
> Do I miss something or is the setuid_to_restricted flag equivalent to
> the curr_token_is_restricted flag anyway, and as such redundant?  If so,
> it would look nice to make setuid_to_restricted an inline method instead:
>
>   bool setuid_to_restricted () const { return curr_token_is_restricted; }
>
>  

setuid_to_restricted is only set in setuid32, not in seteuid32. If
seteuid(geteuid()) is called, the behaviour is similar to the ruid !=
euid case: The exec()ed process can revert to the original token.

Christian

2009-10-11  Christian Franke  <franke@...>
            Corinna Vinschen  <corinna@...>

        * include/sys/cygwin.h: Add new cygwin_getinfo_type
        CW_SET_EXTERNAL_TOKEN.
        Add new enum CW_TOKEN_IMPERSONATION, CW_TOKEN_RESTRICTED.
        * cygheap.h (cyguser): New flags ext_token_is_restricted,
        curr_token_is_restricted and setuid_to_restricted.
        * external.cc (cygwin_internal): Add CW_SET_EXTERNAL_TOKEN.
        * sec_auth.cc (set_imp_token): New function.
        (cygwin_set_impersonation_token): Call set_imp_token ().
        * security.h (set_imp_token): New prototype.
        * spawn.cc (spawn_guts): Use CreateProcessAsUserW if
        restricted token was enabled by setuid ().
        Do not create new window station in this case.
        * syscalls.cc (seteuid32): Add handling of restricted
        external tokens. Set HANDLE_FLAG_INHERIT for primary token.
        (setuid32): Set setuid_to_restricted flag.
        * uinfo.cc (uinfo_init): Do not reimpersonate if
        restricted token was enabled by setuid ().
        Initialize user.*_restricted flags.



diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h
index 7a39de6..b8ffd3f 100644
--- a/winsup/cygwin/cygheap.h
+++ b/winsup/cygwin/cygheap.h
@@ -108,6 +108,9 @@ public:
   HANDLE internal_token;
   HANDLE curr_primary_token;
   HANDLE curr_imp_token;
+  bool ext_token_is_restricted;  /* external_token is restricted token */
+  bool curr_token_is_restricted; /* curr_primary_token is restricted token */
+  bool setuid_to_restricted;     /* switch to restricted token by setuid () */
 
   /* CGF 2002-06-27.  I removed the initializaton from this constructor
      since this class is always allocated statically.  That means that everything
diff --git a/winsup/cygwin/external.cc b/winsup/cygwin/external.cc
index 38b8c71..b099205 100644
--- a/winsup/cygwin/external.cc
+++ b/winsup/cygwin/external.cc
@@ -413,6 +413,13 @@ cygwin_internal (cygwin_getinfo_types t, ...)
   int useTerminateProcess = va_arg (arg, int);
   exit_process (status, !!useTerminateProcess); /* no return */
  }
+      case CW_SET_EXTERNAL_TOKEN:
+ {
+  HANDLE token = va_arg (arg, HANDLE);
+  int type = va_arg (arg, int);
+  set_imp_token (token, type);
+  return 0;
+ }
 
       default:
  break;
diff --git a/winsup/cygwin/include/sys/cygwin.h b/winsup/cygwin/include/sys/cygwin.h
index 5f38278..ce9bebf 100644
--- a/winsup/cygwin/include/sys/cygwin.h
+++ b/winsup/cygwin/include/sys/cygwin.h
@@ -143,9 +143,17 @@ typedef enum
     CW_SET_DOS_FILE_WARNING,
     CW_SET_PRIV_KEY,
     CW_SETERRNO,
-    CW_EXIT_PROCESS
+    CW_EXIT_PROCESS,
+    CW_SET_EXTERNAL_TOKEN
   } cygwin_getinfo_types;
 
+/* Token type for CW_SET_EXTERNAL_TOKEN */
+enum
+{
+  CW_TOKEN_IMPERSONATION = 0,
+  CW_TOKEN_RESTRICTED    = 1
+};
+
 #define CW_NEXTPID 0x80000000 /* or with pid to get next one */
 unsigned long cygwin_internal (cygwin_getinfo_types, ...);
 
diff --git a/winsup/cygwin/sec_auth.cc b/winsup/cygwin/sec_auth.cc
index 028b5a8..34e571f 100644
--- a/winsup/cygwin/sec_auth.cc
+++ b/winsup/cygwin/sec_auth.cc
@@ -30,11 +30,19 @@ details. */
 #include "cygserver_setpwd.h"
 #include <cygwin/version.h>
 
+void
+set_imp_token (HANDLE token, int type)
+{
+  debug_printf ("set_imp_token (%d, %d)", token, type);
+  cygheap->user.external_token = (token == INVALID_HANDLE_VALUE
+  ? NO_IMPERSONATION : token);
+  cygheap->user.ext_token_is_restricted = (type == CW_TOKEN_RESTRICTED);
+}
+
 extern "C" void
 cygwin_set_impersonation_token (const HANDLE hToken)
 {
-  debug_printf ("set_impersonation_token (%d)", hToken);
-  cygheap->user.external_token = hToken == INVALID_HANDLE_VALUE ? NO_IMPERSONATION : hToken;
+  set_imp_token (hToken, CW_TOKEN_IMPERSONATION);
 }
 
 void
diff --git a/winsup/cygwin/security.h b/winsup/cygwin/security.h
index 781def9..d0cb226 100644
--- a/winsup/cygwin/security.h
+++ b/winsup/cygwin/security.h
@@ -366,6 +366,8 @@ extern "C" int acl32 (const char *, int, int, __acl32 *);
 int getacl (HANDLE, path_conv &, int, __acl32 *);
 int setacl (HANDLE, path_conv &, int, __acl32 *, bool &);
 
+/* Set impersonation or restricted token.  */
+void set_imp_token (HANDLE token, int type);
 /* Function creating a token by calling NtCreateToken. */
 HANDLE create_token (cygsid &usersid, user_groups &groups, struct passwd * pw);
 /* LSA authentication function. */
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 5c86c4b..a6ac9f0 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -537,7 +537,8 @@ loop:
   if (!cygheap->user.issetuid ()
       || (cygheap->user.saved_uid == cygheap->user.real_uid
   && cygheap->user.saved_gid == cygheap->user.real_gid
-  && !cygheap->user.groups.issetgroups ()))
+  && !cygheap->user.groups.issetgroups ()
+  && !cygheap->user.setuid_to_restricted))
     {
       rc = CreateProcessW (runpath,  /* image name - with full path */
    wone_line,  /* what was passed to exec */
@@ -571,7 +572,8 @@ loop:
  risk, but we don't want to disable this behaviour for older
  OSes because it's still heavily used by some users.  They have
  been warned. */
-      if (wcscasecmp (wstname, L"WinSta0") != 0)
+      if (!cygheap->user.setuid_to_restricted
+  && wcscasecmp (wstname, L"WinSta0") != 0)
  {
   WCHAR sid[128];
 
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 1529cb6..aa112c4 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -2664,7 +2664,28 @@ seteuid32 (__uid32_t uid)
   debug_printf ("uid: %u myself->uid: %u myself->gid: %u",
  uid, myself->uid, myself->gid);
 
-  if (uid == myself->uid && !cygheap->user.groups.ischanged)
+  /* Same uid as we're just running under is usually a no-op.
+
+     Except we have an external token which is a restricted token.  Or,
+     the external token is NULL, but the current impersonation token is
+     a restricted token.  This allows to restrict user rights temporarily
+     like this:
+
+       cygwin_internal(CW_SET_EXTERNAL_TOKEN, restricted_token,
+                       CW_TOKEN_RESTRICTED);
+       setuid (getuid ());
+       [...do stuff with restricted rights...]
+       cygwin_internal(CW_SET_EXTERNAL_TOKEN, INVALID_HANDLE_VALUE,
+                       CW_TOKEN_RESTRICTED);
+       setuid (getuid ());
+
+    Note that using the current uid is a requirement!  Starting with Windows
+    Vista, we have restricted tokens galore (UAC), so this is really just
+    a special case to restict your own processes to lesser rights. */
+  bool request_restricted_uid_switch = (uid == myself->uid
+      && cygheap->user.ext_token_is_restricted);
+  if (uid == myself->uid && !cygheap->user.groups.ischanged
+      && !request_restricted_uid_switch)
     {
       debug_printf ("Nothing happens");
       return 0;
@@ -2686,6 +2707,22 @@ seteuid32 (__uid32_t uid)
   cygheap->user.deimpersonate ();
 
   /* Verify if the process token is suitable. */
+  /* First of all, skip all checks if a switch to a restricted token has been
+     requested, or if trying to switch back from it. */
+  if (request_restricted_uid_switch)
+    {
+      if (cygheap->user.external_token != NO_IMPERSONATION)
+ {
+  debug_printf ("Switch to restricted token");
+  new_token = cygheap->user.external_token;
+ }
+      else
+ {
+  debug_printf ("Switch back from restricted token");
+  new_token = hProcToken;
+  cygheap->user.ext_token_is_restricted = false;
+ }
+    }
   /* TODO, CV 2008-11-25: The check against saved_sid is a kludge and a
      shortcut.  We must check if it's really feasible in the long run.
      The reason to add this shortcut is this:  sshd switches back to the
@@ -2701,8 +2738,9 @@ seteuid32 (__uid32_t uid)
      Therefore we try this shortcut now.  When switching back to the
      privileged user, we probably always want a correct (aka original)
      user token for this privileged user, not only in sshd. */
-  if ((uid == cygheap->user.saved_uid && usersid == cygheap->user.saved_sid ())
-      || verify_token (hProcToken, usersid, groups))
+  else if ((uid == cygheap->user.saved_uid
+   && usersid == cygheap->user.saved_sid ())
+   || verify_token (hProcToken, usersid, groups))
     new_token = hProcToken;
   /* Verify if the external token is suitable */
   else if (cygheap->user.external_token != NO_IMPERSONATION
@@ -2763,9 +2801,12 @@ seteuid32 (__uid32_t uid)
 
   if (new_token != hProcToken)
     {
-      /* Avoid having HKCU use default user */
-      WCHAR name[128];
-      load_registry_hive (usersid.string (name));
+      if (!request_restricted_uid_switch)
+ {
+  /* Avoid having HKCU use default user */
+  WCHAR name[128];
+  load_registry_hive (usersid.string (name));
+ }
 
       /* Try setting owner to same value as user. */
       if (!SetTokenInformation (new_token, TokenOwner,
@@ -2790,6 +2831,8 @@ seteuid32 (__uid32_t uid)
   cygheap->user.set_sid (usersid);
   cygheap->user.curr_primary_token = new_token == hProcToken ? NO_IMPERSONATION
  : new_token;
+  cygheap->user.curr_token_is_restricted = false;
+  cygheap->user.setuid_to_restricted = false;
   if (cygheap->user.curr_imp_token != NO_IMPERSONATION)
     {
       CloseHandle (cygheap->user.curr_imp_token);
@@ -2797,14 +2840,19 @@ seteuid32 (__uid32_t uid)
     }
   if (cygheap->user.curr_primary_token != NO_IMPERSONATION)
     {
-      if (!DuplicateTokenEx (cygheap->user.curr_primary_token, MAXIMUM_ALLOWED,
-     &sec_none, SecurityImpersonation,
-     TokenImpersonation, &cygheap->user.curr_imp_token))
+      /* HANDLE_FLAG_INHERIT may be missing in external token.  */
+      if (!SetHandleInformation (cygheap->user.curr_primary_token,
+ HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT)
+  || !DuplicateTokenEx (cygheap->user.curr_primary_token,
+ MAXIMUM_ALLOWED, &sec_none,
+ SecurityImpersonation, TokenImpersonation,
+ &cygheap->user.curr_imp_token))
  {
   __seterrno ();
   cygheap->user.curr_primary_token = NO_IMPERSONATION;
   return -1;
  }
+      cygheap->user.curr_token_is_restricted = request_restricted_uid_switch;
       set_cygwin_privileges (cygheap->user.curr_primary_token);
       set_cygwin_privileges (cygheap->user.curr_imp_token);
     }
@@ -2835,7 +2883,11 @@ setuid32 (__uid32_t uid)
 {
   int ret = seteuid32 (uid);
   if (!ret)
-    cygheap->user.real_uid = myself->uid;
+    {
+      cygheap->user.real_uid = myself->uid;
+      /* If restricted token, forget original privileges on exec ().  */
+      cygheap->user.setuid_to_restricted = cygheap->user.curr_token_is_restricted;
+    }
   debug_printf ("real: %d, effective: %d", cygheap->user.real_uid, myself->uid);
   return ret;
 }
diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
index 8adfd37..a480f99 100644
--- a/winsup/cygwin/uinfo.cc
+++ b/winsup/cygwin/uinfo.cc
@@ -136,7 +136,8 @@ uinfo_init ()
   else if (cygheap->user.issetuid ()
    && cygheap->user.saved_uid == cygheap->user.real_uid
    && cygheap->user.saved_gid == cygheap->user.real_gid
-   && !cygheap->user.groups.issetgroups ())
+   && !cygheap->user.groups.issetgroups ()
+   && !cygheap->user.setuid_to_restricted)
     {
       cygheap->user.reimpersonate ();
       return;
@@ -150,6 +151,9 @@ uinfo_init ()
   cygheap->user.internal_token = NO_IMPERSONATION;
   cygheap->user.curr_primary_token = NO_IMPERSONATION;
   cygheap->user.curr_imp_token = NO_IMPERSONATION;
+  cygheap->user.ext_token_is_restricted = false;
+  cygheap->user.curr_token_is_restricted = false;
+  cygheap->user.setuid_to_restricted = false;
   cygheap->user.set_saved_sid (); /* Update the original sid */
   cygheap->user.reimpersonate ();
 }

[Corinna Vinschen-2]

On Oct 11 22:45, Christian Franke wrote:
> Corinna Vinschen wrote:
>> Thanks for the patch.  You did check that the normal setuid/seteuid
>> cases still work, didn't you?
>>
>>  
>
> Yes.

Cool.  I just tested it myself and it looks good.

>> What's wrong with:
>>
>>   for i in $(id -G);
>>   do
>>     [ $i -eq 544 ] && PS1='# '
>>   done
>>
>>  
>
> Is OK, except if admin group is mapped to other gid (0?) in /etc/group.

It isn't in the default case.  And it's important that there is a way
to handle this with simple POSIXy means.

> I removed the error check and set HANDLE_FLAG_INHERIT in seteuid32().

Oh, sure!  That's much simpler than duplicating the token handle at
set_imp_token time.

>> Do I miss something or is the setuid_to_restricted flag equivalent to
>> the curr_token_is_restricted flag [...]
>
> setuid_to_restricted is only set in setuid32, not in seteuid32. If
> seteuid(geteuid()) is called, the behaviour is similar to the ruid != euid
> case: The exec()ed process can revert to the original token.

Ok, so I missed something, sorry.

> * include/sys/cygwin.h: Add new cygwin_getinfo_type
> CW_SET_EXTERNAL_TOKEN.
> Add new enum CW_TOKEN_IMPERSONATION, CW_TOKEN_RESTRICTED.
> * cygheap.h (cyguser): New flags ext_token_is_restricted,
> curr_token_is_restricted and setuid_to_restricted.
> * external.cc (cygwin_internal): Add CW_SET_EXTERNAL_TOKEN.
> * sec_auth.cc (set_imp_token): New function.
> (cygwin_set_impersonation_token): Call set_imp_token ().
> * security.h (set_imp_token): New prototype.
> * spawn.cc (spawn_guts): Use CreateProcessAsUserW if
> restricted token was enabled by setuid ().
> Do not create new window station in this case.
> * syscalls.cc (seteuid32): Add handling of restricted
> external tokens. Set HANDLE_FLAG_INHERIT for primary token.
> (setuid32): Set setuid_to_restricted flag.
> * uinfo.cc (uinfo_init): Do not reimpersonate if
> restricted token was enabled by setuid ().
> Initialize user.*_restricted flags.

Patch checked in.

Thanks for doing this.  Would you have fun to provide a tool for the
net distro which uses this feature?


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Christian Franke on 10/11/2009 2:45 PM:
> 2009-10-11  Christian Franke  <franke@...>
>            Corinna Vinschen  <corinna@...>
>
>     * include/sys/cygwin.h: Add new cygwin_getinfo_type
>     CW_SET_EXTERNAL_TOKEN.
>     Add new enum CW_TOKEN_IMPERSONATION, CW_TOKEN_RESTRICTED.

Shouldn't we also bump version.h when adding new CW_ flags?

> +      case CW_SET_EXTERNAL_TOKEN:
> + {
> +  HANDLE token = va_arg (arg, HANDLE);
> +  int type = va_arg (arg, int);
> +  set_imp_token (token, type);
> +  return 0;
> + }

Not the first time this is done in this function.  But generally,
shouldn't we follow the good practice of using va_end any time we used
va_arg, in case cygwin is ever ported to a system where va_end is more
than a no-op?  [At least, I'm assuming that __builtin_va_end() is a no-op
for x86?]

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrUbB4ACgkQ84KuGfSFAYAHOQCgt+MI1ALkqnMMwPX6QlJ7VwJZ
mYMAn37mvgvZDZzBw27vXcKutLGwilpW
=8hvQ
-----END PGP SIGNATURE-----

[Corinna Vinschen-2]

On Oct 13 06:01, Eric Blake wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> According to Christian Franke on 10/11/2009 2:45 PM:
> > 2009-10-11  Christian Franke  <franke@...>
> >            Corinna Vinschen  <corinna@...>
> >
> >     * include/sys/cygwin.h: Add new cygwin_getinfo_type
> >     CW_SET_EXTERNAL_TOKEN.
> >     Add new enum CW_TOKEN_IMPERSONATION, CW_TOKEN_RESTRICTED.
>
> Shouldn't we also bump version.h when adding new CW_ flags?

You're right.  Done in CVS.

> > +      case CW_SET_EXTERNAL_TOKEN:
> > + {
> > +  HANDLE token = va_arg (arg, HANDLE);
> > +  int type = va_arg (arg, int);
> > +  set_imp_token (token, type);
> > +  return 0;
> > + }
>
> Not the first time this is done in this function.  But generally,
> shouldn't we follow the good practice of using va_end any time we used
> va_arg, in case cygwin is ever ported to a system where va_end is more
> than a no-op?  [At least, I'm assuming that __builtin_va_end() is a no-op
> for x86?]

That's probably a good idea, given that POSIX requires the usage of
va_end.  PTC?


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          cygwin AT cygwin DOT com
Red Hat

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Corinna Vinschen on 10/13/2009 6:17 AM:
>> Not the first time this is done in this function.  But generally,
>> shouldn't we follow the good practice of using va_end any time we used
>> va_arg, in case cygwin is ever ported to a system where va_end is more
>> than a no-op?  [At least, I'm assuming that __builtin_va_end() is a no-op
>> for x86?]
>
> That's probably a good idea, given that POSIX requires the usage of
> va_end.  PTC?

Sure, I'll do an audit.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@...
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrUcMMACgkQ84KuGfSFAYCrngCfbAYGeGsmatSNZVyKkBSOqotl
cY0An3rGFaS2cQ1RErZsbKDVJUutE1xF
=F1oq
-----END PGP SIGNATURE-----