public inbox for cygwin-apps@cygwin.com
 help / color / mirror / Atom feed
* [PATCH setup] Add new option --chown-admin
@ 2022-07-06  7:14 Christian Franke
  2022-07-06 13:53 ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-07-06  7:14 UTC (permalink / raw)
  To: cygwin-apps

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

If an installer is run elevated, the installed files will be typically 
owned by the local administrator (or in some cases SYSTEM or 
TrustedInstaller) instead of the current user. This is not the case for 
a Cygwin "All Users" installation. The files are then not protected from 
accidental changes by this user.

The attached patch adds an experimental --chown-admin option which 
allows (new) installations owned by local administrator user and group.

A drawback is that files generated by postinstall scripts are still 
owned by current user + "None" group. It should be possible to fix this 
with some perpetual preremove+postinstall scripts.

I also don't know whether this may break some postinstall scripts.

BTW: 'nt_sec.setDefaultSecurity (isAdmin)' is never called with 
'isAdmin==true' as 'root_scope' is always 0.

-- 
Regards,
Christian


[-- Attachment #2: 0001-Add-new-option-chown-admin.patch --]
[-- Type: text/plain, Size: 4531 bytes --]

From 1dfc8d63a8438e42544b06cfdf225f222107eed2 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Wed, 6 Jul 2022 07:41:18 +0200
Subject: [PATCH] Add new option --chown-admin

If specified and the process token owner is the local administrator,
the owner is preserved and the primary group is set to the local
administrator group.
---
 main.cc  |  3 ++-
 win32.cc | 41 ++++++++++++++++++++++++++++++-----------
 win32.h  |  2 +-
 3 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/main.cc b/main.cc
index 3a8c5ea..81c4a65 100644
--- a/main.cc
+++ b/main.cc
@@ -99,6 +99,7 @@ static StringOption Arch ("", 'a', "arch", IDS_HELPTEXT_ARCH, false);
 static BoolOption UnattendedOption (false, 'q', "quiet-mode", IDS_HELPTEXT_QUIET_MODE);
 static BoolOption PackageManagerOption (false, 'M', "package-manager", IDS_HELPTEXT_PACKAGE_MANAGER);
 static BoolOption NoAdminOption (false, 'B', "no-admin", IDS_HELPTEXT_NO_ADMIN);
+static BoolOption ChownAdminOption (false, '\0', "chown-admin" /*, TODO: IDS_HELPTEXT_... */);
 static BoolOption WaitOption (false, 'W', "wait", IDS_HELPTEXT_WAIT);
 static BoolOption HelpOption (false, 'h', "help", IDS_HELPTEXT_HELP);
 static BoolOption VersionOption (false, 'V', "version", IDS_HELPTEXT_VERSION);
@@ -359,7 +360,7 @@ WinMain (HINSTANCE h,
       }
 
     /* Set default DACL and Group. */
-    nt_sec.setDefaultSecurity ((root_scope == IDC_ROOT_SYSTEM));
+    nt_sec.setDefaultSecurity ((root_scope == IDC_ROOT_SYSTEM), ChownAdminOption);
 
     /*
        If --symlink-type option isn't given, look for winsymlinks in CYGWIN
diff --git a/win32.cc b/win32.cc
index 55072a9..5dc9616 100644
--- a/win32.cc
+++ b/win32.cc
@@ -308,7 +308,7 @@ NTSecurity::setAdminGroup ()
 }
 
 void
-NTSecurity::setDefaultSecurity (bool isAdmin)
+NTSecurity::setDefaultSecurity (bool isAdmin, bool keepAdmin)
 {
   /* Get the processes access token. */
   if (!OpenProcessToken (GetCurrentProcess (),
@@ -335,21 +335,40 @@ NTSecurity::setDefaultSecurity (bool isAdmin)
   /* Set the default DACL to all permissions for everyone as a fallback. */
   setDefaultDACL ();
 
-  /* Get the user */
-  if (!GetTokenInformation (token.theHANDLE (), TokenUser, &ownerSID,
+  /* Get the owner */
+  if (!GetTokenInformation (token.theHANDLE (), TokenOwner, &ownerSID,
 			    sizeof ownerSID, &size))
     {
-      NoteFailedAPI ("GetTokenInformation(user)");
+      NoteFailedAPI ("GetTokenInformation(owner)");
       return;
     }
-  /* Make it the owner */
-  TOKEN_OWNER owner = { ownerSID.user.User.Sid };
-  if (!SetTokenInformation (token.theHANDLE (), TokenOwner, &owner,
-			    sizeof owner))
+
+  bool ownerIsAdmin = !!EqualSid (ownerSID.user.User.Sid, administratorsSID.theSID ());
+
+  if (keepAdmin && ownerIsAdmin)
+    Log (LOG_TIMESTAMP) << "Default owner is Administrator" << endLog;
+  else
     {
-      NoteFailedAPI ("SetTokenInformation(owner)");
-      return;
+      /* Get the user */
+      if (!GetTokenInformation (token.theHANDLE (), TokenUser, &ownerSID,
+				sizeof ownerSID, &size))
+	{
+	  NoteFailedAPI ("GetTokenInformation(user)");
+	  return;
+	}
+      /* Make it the owner */
+      TOKEN_OWNER owner = { ownerSID.user.User.Sid };
+      if (!SetTokenInformation (token.theHANDLE (), TokenOwner, &owner,
+				sizeof owner))
+	{
+	  NoteFailedAPI ("SetTokenInformation(owner)");
+	  return;
+	}
+      Log (LOG_TIMESTAMP) << "Default owner changed "
+			  << (ownerIsAdmin ? "from Administrator " : "")
+			  << "to current user" << endLog;
     }
+
   /* Get original primary group.  The token's primary group will be reset
      to the original group right before we call the postinstall scripts.
      This is necessary, otherwise, if the installing user is a domain user,
@@ -365,7 +384,7 @@ NTSecurity::setDefaultSecurity (bool isAdmin)
   /* Try to set the primary group to the Administrators group, but only if
      "Install for all users" has been chosen.  If it doesn't work, we're
      no admin and that's all there's to say about it. */
-  if (isAdmin)
+  if (isAdmin || (keepAdmin && ownerIsAdmin))
     setAdminGroup ();
 }
 
diff --git a/win32.h b/win32.h
index 02c1d06..560512b 100644
--- a/win32.h
+++ b/win32.h
@@ -130,7 +130,7 @@ public:
   void resetPrimaryGroup();
   void setAdminGroup ();
   void initialiseWellKnownSIDs ();
-  void setDefaultSecurity(bool isAdmin);
+  void setDefaultSecurity(bool isAdmin, bool keepAdmin);
   bool isRunAsAdmin ();
   bool hasSymlinkCreationRights ();
 private:
-- 
2.36.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-07-06  7:14 [PATCH setup] Add new option --chown-admin Christian Franke
@ 2022-07-06 13:53 ` Jon Turney
  2022-07-06 16:34   ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-07-06 13:53 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

On 06/07/2022 08:14, Christian Franke wrote:
> If an installer is run elevated, the installed files will be typically 
> owned by the local administrator (or in some cases SYSTEM or 
> TrustedInstaller) instead of the current user. This is not the case for 
> a Cygwin "All Users" installation. The files are then not protected from 

... instead the files are owned by the user running setup?

> accidental changes by this user.
> 
> The attached patch adds an experimental --chown-admin option which 
> allows (new) installations owned by local administrator user and group.

Thanks for the patch, but...

> A drawback is that files generated by postinstall scripts are still 
> owned by current user + "None" group. It should be possible to fix this 
> with some perpetual preremove+postinstall scripts.
> 
> I also don't know whether this may break some postinstall scripts.
> 
> BTW: 'nt_sec.setDefaultSecurity (isAdmin)' is never called with 
> 'isAdmin==true' as 'root_scope' is always 0.

root_scope is set later, by the "Install For" option on the "Select Root 
Install Directory" page.

To me, this looks like a (very long standing) bug that we shouldn't be 
calling setAdminGroup() here, but after root_scope has been set.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-07-06 13:53 ` Jon Turney
@ 2022-07-06 16:34   ` Christian Franke
  2022-07-07 11:38     ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-07-06 16:34 UTC (permalink / raw)
  To: cygwin-apps

Jon Turney wrote:
> On 06/07/2022 08:14, Christian Franke wrote:
>> If an installer is run elevated, the installed files will be 
>> typically owned by the local administrator (or in some cases SYSTEM 
>> or TrustedInstaller) instead of the current user. This is not the 
>> case for a Cygwin "All Users" installation. The files are then not 
>> protected from 
>
> ... instead the files are owned by the user running setup?

Yes, because the TokenUser is unconditionally copied to TokenOwner. Some 
research with 'git blame' shows that many parts of the related code are 
from the early days of UAC and elevated processes (Vista 2006, Win7 
2009). Things may have changed since then, I don't know (or remember).

If a process is run elevated, Windows keeps TokenUser unchanged but sets 
the TokenOwner to local administrator. The --chown-admin option simply 
keeps this as is, so no actual chown() is needed later. The 
TokenPrimaryGroup is not changed by Windows, therefore --chown-admin 
calls setAdminGroup() to mimic the usual "root root" ownership.

Typical simple installers leave everything as is, so the installed files 
are owned by local adminstrator and group "None" (S-1-5-21-*-513), see 
'ls -l "$PROGRAMFILES"'.


>> accidental changes by this user.
>>
>> The attached patch adds an experimental --chown-admin option which 
>> allows (new) installations owned by local administrator user and group.
>
> Thanks for the patch, but...
>
>> A drawback is that files generated by postinstall scripts are still 
>> owned by current user + "None" group. It should be possible to fix 
>> this with some perpetual preremove+postinstall scripts.
>>
>> I also don't know whether this may break some postinstall scripts.
>>
>> BTW: 'nt_sec.setDefaultSecurity (isAdmin)' is never called with 
>> 'isAdmin==true' as 'root_scope' is always 0.
>
> root_scope is set later, by the "Install For" option on the "Select 
> Root Install Directory" page.
>
> To me, this looks like a (very long standing) bug that we shouldn't be 
> calling setAdminGroup() here, but after root_scope has been set.

If this bug is very old, I'm not sure whether this should be fixed. 
Setting admin group to files which are owned "only" by current user is 
possibly not very effective.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-07-06 16:34   ` Christian Franke
@ 2022-07-07 11:38     ` Jon Turney
  2022-07-07 14:45       ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-07-07 11:38 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

On 06/07/2022 17:34, Christian Franke wrote:
> Jon Turney wrote:
>> On 06/07/2022 08:14, Christian Franke wrote:
>>> If an installer is run elevated, the installed files will be 
>>> typically owned by the local administrator (or in some cases SYSTEM 
>>> or TrustedInstaller) instead of the current user. This is not the 
>>> case for a Cygwin "All Users" installation. The files are then not 
>>> protected from 
>>
>> ... instead the files are owned by the user running setup?
> 
> Yes, because the TokenUser is unconditionally copied to TokenOwner. Some 
> research with 'git blame' shows that many parts of the related code are 
> from the early days of UAC and elevated processes (Vista 2006, Win7 
> 2009). Things may have changed since then, I don't know (or remember).
> 
> If a process is run elevated, Windows keeps TokenUser unchanged but sets 
> the TokenOwner to local administrator. The --chown-admin option simply 
> keeps this as is, so no actual chown() is needed later. The 
> TokenPrimaryGroup is not changed by Windows, therefore --chown-admin 
> calls setAdminGroup() to mimic the usual "root root" ownership.
> 
> Typical simple installers leave everything as is, so the installed files 
> are owned by local adminstrator and group "None" (S-1-5-21-*-513), see 
> 'ls -l "$PROGRAMFILES"'.
> 
> 
>>> accidental changes by this user.
>>>
>>> The attached patch adds an experimental --chown-admin option which 
>>> allows (new) installations owned by local administrator user and group.
>>
>> Thanks for the patch, but...
>>
>>> A drawback is that files generated by postinstall scripts are still 
>>> owned by current user + "None" group. It should be possible to fix 
>>> this with some perpetual preremove+postinstall scripts.
>>>
>>> I also don't know whether this may break some postinstall scripts.
>>>
>>> BTW: 'nt_sec.setDefaultSecurity (isAdmin)' is never called with 
>>> 'isAdmin==true' as 'root_scope' is always 0.
>>
>> root_scope is set later, by the "Install For" option on the "Select 
>> Root Install Directory" page.
>>
>> To me, this looks like a (very long standing) bug that we shouldn't be 
>> calling setAdminGroup() here, but after root_scope has been set.
> 
> If this bug is very old, I'm not sure whether this should be fixed. 
> Setting admin group to files which are owned "only" by current user is 
> possibly not very effective.

It's true that some people might be relying on that buggy behaviour.

But, please help me understand how your patch differs from adding an 
option which fixes that misbehaviour when supplied?

(As an aside, looking at how setAdminGroup()/resetPrimaryGroup() are 
used, changing the token for the postinstall scripts seems unnecessary 
now, since we don't run mkpasswd there any more...)

[1] 
https://cygwin.com/git/?p=cygwin-apps/setup.git;a=blob;f=postinstall.cc;h=e990f520077f360d1b39f2d0fa915fd6110f4c84;hb=HEAD#l271


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-07-07 11:38     ` Jon Turney
@ 2022-07-07 14:45       ` Christian Franke
  2022-07-07 14:59         ` Christian Franke
       [not found]         ` <d7d51d1c-f6d5-2fac-3e6d-86714efd0734@dronecode.org.uk>
  0 siblings, 2 replies; 18+ messages in thread
From: Christian Franke @ 2022-07-07 14:45 UTC (permalink / raw)
  To: cygwin-apps

Jon Turney wrote:
> On 06/07/2022 17:34, Christian Franke wrote:
>> Jon Turney wrote:
>>> On 06/07/2022 08:14, Christian Franke wrote:
>>>> If an installer is run elevated, the installed files will be 
>>>> typically owned by the local administrator (or in some cases SYSTEM 
>>>> or TrustedInstaller) instead of the current user. This is not the 
>>>> case for a Cygwin "All Users" installation. The files are then not 
>>>> protected from 
>>>
>>> ... instead the files are owned by the user running setup?
>>
>> Yes, because the TokenUser is unconditionally copied to TokenOwner. 
>> Some research with 'git blame' shows that many parts of the related 
>> code are from the early days of UAC and elevated processes (Vista 
>> 2006, Win7 2009). Things may have changed since then, I don't know 
>> (or remember).
>>
>> If a process is run elevated, Windows keeps TokenUser unchanged but 
>> sets the TokenOwner to local administrator. The --chown-admin option 
>> simply keeps this as is, so no actual chown() is needed later. The 
>> TokenPrimaryGroup is not changed by Windows, therefore --chown-admin 
>> calls setAdminGroup() to mimic the usual "root root" ownership.
>>
>> Typical simple installers leave everything as is, so the installed 
>> files are owned by local adminstrator and group "None" 
>> (S-1-5-21-*-513), see 'ls -l "$PROGRAMFILES"'.
>>
>>
>>>> accidental changes by this user.
>>>>
>>>> The attached patch adds an experimental --chown-admin option which 
>>>> allows (new) installations owned by local administrator user and 
>>>> group.
>>>
>>> Thanks for the patch, but...
>>>
>>>> A drawback is that files generated by postinstall scripts are still 
>>>> owned by current user + "None" group. It should be possible to fix 
>>>> this with some perpetual preremove+postinstall scripts.
>>>>
>>>> I also don't know whether this may break some postinstall scripts.
>>>>
>>>> BTW: 'nt_sec.setDefaultSecurity (isAdmin)' is never called with 
>>>> 'isAdmin==true' as 'root_scope' is always 0.
>>>
>>> root_scope is set later, by the "Install For" option on the "Select 
>>> Root Install Directory" page.
>>>
>>> To me, this looks like a (very long standing) bug that we shouldn't 
>>> be calling setAdminGroup() here, but after root_scope has been set.
>>
>> If this bug is very old, I'm not sure whether this should be fixed. 
>> Setting admin group to files which are owned "only" by current user 
>> is possibly not very effective.
>
> It's true that some people might be relying on that buggy behaviour.

I have one very old Cygwin installation from Win7 times. Very old 
installed files still have group="Administrator", newer files have 
group="None". The timestamps suggest that the regression was introduced 
early in 2012. The first file with group="None" is from March 2 2012.


> But, please help me understand how your patch differs from adding an 
> option which fixes that misbehaviour when supplied?

It does fix the regression, it adds a new installation flavor which 
fixes it as a side effect.

Possible owner:group assignments of newly installed dirs/files:

"All Users":
adm:adm -- With this patch and --chown-admin set
usr:def -- Current behavior, regardless of this patch
usr:adm -- Before 'isAdmin always false' regression was introduced

"Just Me":
adm:adm -- With this patch, --chown-admin set and setup has admin rights
usr:def -- Otherwise

(usr = user running setup, adm = S-1-5-32-544, def = S-1-5-21-*-513)


>
> (As an aside, looking at how setAdminGroup()/resetPrimaryGroup() are 
> used, changing the token for the postinstall scripts seems unnecessary 
> now, since we don't run mkpasswd there any more...)

Agree.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-07-07 14:45       ` Christian Franke
@ 2022-07-07 14:59         ` Christian Franke
       [not found]         ` <d7d51d1c-f6d5-2fac-3e6d-86714efd0734@dronecode.org.uk>
  1 sibling, 0 replies; 18+ messages in thread
From: Christian Franke @ 2022-07-07 14:59 UTC (permalink / raw)
  To: cygwin-apps

Christian Franke wrote:
>
> It does fix the regression, it adds a new installation flavor which 
> fixes it as a side effect.

It does *not* fix the regression, of course



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
       [not found]           ` <32655945-5075-0823-2a1d-b72caa4b7791@t-online.de>
@ 2022-07-12 12:50             ` Jon Turney
  2022-08-23 15:20               ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-07-12 12:50 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

[Replying to the right list this time...]

On 09/07/2022 13:21, Christian Franke wrote:
> Jon Turney wrote:
>> On 07/07/2022 15:45, Christian Franke wrote:
>>> Jon Turney wrote:
>>>> On 06/07/2022 17:34, Christian Franke wrote:
>>>>> Jon Turney wrote:
>>>>>> On 06/07/2022 08:14, Christian Franke wrote:
>> [...]
>>>>>>>
>>>>>>> BTW: 'nt_sec.setDefaultSecurity (isAdmin)' is never called with 
>>>>>>> 'isAdmin==true' as 'root_scope' is always 0.
>>>>>>
>>>>>> root_scope is set later, by the "Install For" option on the 
>>>>>> "Select Root Install Directory" page.
>>>>>>
>>>>>> To me, this looks like a (very long standing) bug that we 
>>>>>> shouldn't be calling setAdminGroup() here, but after root_scope 
>>>>>> has been set.
>>>>>
>>>>> If this bug is very old, I'm not sure whether this should be fixed. 
>>>>> Setting admin group to files which are owned "only" by current user 
>>>>> is possibly not very effective.
>>>>
>>>> It's true that some people might be relying on that buggy behaviour.
>>>
>>> I have one very old Cygwin installation from Win7 times. Very old 
>>> installed files still have group="Administrator", newer files have 
>>> group="None". The timestamps suggest that the regression was 
>>> introduced early in 2012. The first file with group="None" is from 
>>> March 2 2012.
>>
>> Hmm... [1] seems like the obvious suspect for the change responsible 
>> for that, but I don't immediately see how...
>>
>> [1] 
>> https://cygwin.com/git/?p=cygwin-apps/setup.git;a=commitdiff;h=befc9dd806824f22ebb740be96ba8c0ae8f63bb4;hp=34d534a6d74e5516d6691fb1d9cb6309682afa0b 
>>
>>
> 
> Hmm... correct as this change moves UserSettings ctor behind 
> setDefaultSecurity ():
> 
> Old version 34d534a:
> ...
> UserSettings Settings (local_dir);
> ...
> nt_sec.setDefaultSecurity ();
> ...
> Main.WindowCreate()
> 
> 
> New version befc9dd:
> ...
> nt_sec.setDefaultSecurity ();
> ...
> UserSettings Settings (local_dir);
> ...
> Main.WindowCreate()
> 
> 
> The UserSettings ctor has a somewhat hidden side effect which sets 
> root_scope correctly:
> 
>   UserSettings::UserSettings(...);
>    open_settings("setup.rc", ...);
>     io_stream::open("cygfile:///etc/setup/setup.rc", ...);
>      io_stream_cygfile::io_stream_cygfile("/etc/setup/setup.rc", ...);
>       get_root_dir_now();
>        read_mounts("");
>         read_mounts_nt("");
>          root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
> 
> Conclusion: Regression introduced Feb 24, 2012 (befc9dd).
> 

Thanks for tracking this down.

That just seems... fractally wrong.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-07-12 12:50             ` Jon Turney
@ 2022-08-23 15:20               ` Jon Turney
  2022-08-23 17:27                 ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-08-23 15:20 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

On 12/07/2022 13:50, Jon Turney wrote:
> [Replying to the right list this time...]
> On 09/07/2022 13:21, Christian Franke wrote:
[...]
>>
>> The UserSettings ctor has a somewhat hidden side effect which sets 
>> root_scope correctly:
>>
>>   UserSettings::UserSettings(...);
>>    open_settings("setup.rc", ...);
>>     io_stream::open("cygfile:///etc/setup/setup.rc", ...);
>>      io_stream_cygfile::io_stream_cygfile("/etc/setup/setup.rc", ...);
>>       get_root_dir_now();
>>        read_mounts("");
>>         read_mounts_nt("");
>>          root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
>>
>> Conclusion: Regression introduced Feb 24, 2012 (befc9dd).
>>
> 
> Thanks for tracking this down.
> 
> That just seems... fractally wrong.

I kind of lost track of this.  Is there anything else needed to fix the 
original problem here?  Or is it solved by the change to defer setting 
the group until after root_scope is known?


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-08-23 15:20               ` Jon Turney
@ 2022-08-23 17:27                 ` Christian Franke
  2022-08-26 13:27                   ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-08-23 17:27 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

Jon Turney wrote:
> On 12/07/2022 13:50, Jon Turney wrote:
>> [Replying to the right list this time...]
>> On 09/07/2022 13:21, Christian Franke wrote:
> [...]
>>>
>>> The UserSettings ctor has a somewhat hidden side effect which sets 
>>> root_scope correctly:
>>>
>>>   UserSettings::UserSettings(...);
>>>    open_settings("setup.rc", ...);
>>>     io_stream::open("cygfile:///etc/setup/setup.rc", ...);
>>> io_stream_cygfile::io_stream_cygfile("/etc/setup/setup.rc", ...);
>>>       get_root_dir_now();
>>>        read_mounts("");
>>>         read_mounts_nt("");
>>>          root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
>>>
>>> Conclusion: Regression introduced Feb 24, 2012 (befc9dd).
>>>
>>
>> Thanks for tracking this down.
>>
>> That just seems... fractally wrong.
>
> I kind of lost track of this.  Is there anything else needed to fix 
> the original problem here?  Or is it solved by the change to defer 
> setting the group until after root_scope is known?
>
>

The group seems to be correctly set now.

An old problem still remains: root_scope always ends up as 
IDC_ROOT_SYSTEM if setup is run elevated, regardless off GUI setting. 
Apply the temporary patch from here to see what happens:
https://sourceware.org/pipermail/cygwin-apps/2022-July/042151.html

Possibly difficult to fix, in particular in conjunction with later 
changes via [< Back] button. An easier approach: Remove the GUI setting 
and connect root_scope to -B option.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-08-23 17:27                 ` Christian Franke
@ 2022-08-26 13:27                   ` Jon Turney
  2022-08-26 15:02                     ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-08-26 13:27 UTC (permalink / raw)
  To: cygwin-apps, Christian Franke

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

On 23/08/2022 18:27, Christian Franke wrote:
> Jon Turney wrote:
>> On 12/07/2022 13:50, Jon Turney wrote:
>>> [Replying to the right list this time...]
>>> On 09/07/2022 13:21, Christian Franke wrote:
>> [...]
>>>>
>>>> The UserSettings ctor has a somewhat hidden side effect which sets 
>>>> root_scope correctly:
>>>>
>>>>   UserSettings::UserSettings(...);
>>>>    open_settings("setup.rc", ...);
>>>>     io_stream::open("cygfile:///etc/setup/setup.rc", ...);
>>>> io_stream_cygfile::io_stream_cygfile("/etc/setup/setup.rc", ...);
>>>>       get_root_dir_now();
>>>>        read_mounts("");
>>>>         read_mounts_nt("");
>>>>          root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
>>>>
>>>> Conclusion: Regression introduced Feb 24, 2012 (befc9dd).
>>>>
>>>
>>> Thanks for tracking this down.
>>>
>>> That just seems... fractally wrong.
>>
>> I kind of lost track of this.  Is there anything else needed to fix 
>> the original problem here?  Or is it solved by the change to defer 
>> setting the group until after root_scope is known?
> 
> The group seems to be correctly set now.
> 
> An old problem still remains: root_scope always ends up as 
> IDC_ROOT_SYSTEM if setup is run elevated, regardless off GUI setting. 
> Apply the temporary patch from here to see what happens:
> https://sourceware.org/pipermail/cygwin-apps/2022-July/042151.html

Ah, right, I remember now.

I think having read_mounts() have a side effect of setting root_scope is 
just nonsense now (it might have made some sense back in the day when 
the mount table was also stored in the registry).

So, how about the attached?

(We should perhaps also set the installation root directory to something 
other than C:\cygwin64 if a non-admin user, since they are unlikely to 
be able to write there in current windows versions, but that's difficult 
from a sequencing point of view in that dialog, and for backwards 
compatibility)

> Possibly difficult to fix, in particular in conjunction with later 
> changes via [< Back] button. An easier approach: Remove the GUI setting 
> and connect root_scope to -B option.

Looking for that option is not correct, because we might be running 
without -B, but from an already elevated shell.  I think checking 
nt_sec.isRunAsAdmin() would be the correct test.

[-- Attachment #2: 0001-Drop-setting-root_scope-as-a-side-effect-of-read_mou.patch --]
[-- Type: text/plain, Size: 3589 bytes --]

From 8bb9d2d647c4a9c49e696efb14d976682ad28647 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Wed, 24 Aug 2022 13:31:49 +0100
Subject: [PATCH setup] Drop setting root_scope as a side-effect of
 read_mounts()

Default root_scope as appropriate, allowing GUI to override it
Only enable 'Install For All Users' if we have Admin privs
Drop some comment cruft
---
 mount.cc | 30 +++++++++++++++++++++++-------
 mount.h  |  9 +--------
 root.cc  | 12 ++++++++++++
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/mount.cc b/mount.cc
index 0136396..a38f52c 100644
--- a/mount.cc
+++ b/mount.cc
@@ -140,10 +140,6 @@ create_install_root ()
 						      : "LOCAL_MACHINE\\")
 		      << buf << "\\rootdir = \"" << get_root_dir () << "\""
 		      << endLog;
-
-  // The mount table is already in the right shape at this point.
-  // Reading it again is not necessary.
-  //read_mounts (std::string ());
 }
 
 inline char *
@@ -316,8 +312,6 @@ read_mounts (const std::string val)
     }
   got_usr_bin = got_usr_lib = false;
 
-  root_scope = (nt_sec.isRunAsAdmin ())? IDC_ROOT_SYSTEM : IDC_ROOT_USER;
-
   if (val.size ())
     {
       /* Cygwin rootdir always < MAX_PATH. */
@@ -353,7 +347,6 @@ read_mounts (const std::string val)
 	    {
 	      m->native = std::string (aBuffer);
 	      m->posix = "/";
-	      root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
 	      root_here = m++;
 	      from_fstab (m, root_here->native);
 	      add_usr_mnts (m);
@@ -376,6 +369,29 @@ read_mounts (const std::string val)
     }
 }
 
+// set default root_scope: USER if only HKEY_CURRENT_USER registry key exists,
+// otherwise SYSTEM.
+void set_default_root_scope()
+{
+  root_scope = IDC_ROOT_SYSTEM;
+
+  char buf[10000];
+  for (int isuser = 0; isuser <= 1; isuser++)
+    {
+      snprintf (buf, sizeof(buf), "Software\\%s\\%s",
+                CYGWIN_INFO_CYGWIN_REGISTRY_NAME,
+                CYGWIN_INFO_CYGWIN_SETUP_REGISTRY_NAME);
+      HKEY key = isuser ? HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE;
+      if (RegOpenKeyEx (key, buf, 0, KEY_ALL_ACCESS | SETUP_KEY_WOW64,
+                        &key) == ERROR_SUCCESS)
+        {
+          RegCloseKey (key);
+          root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
+          break;
+        }
+    }
+}
+
 void
 set_root_dir (const std::string val)
 {
diff --git a/mount.h b/mount.h
index a7d7e39..c451a02 100644
--- a/mount.h
+++ b/mount.h
@@ -15,11 +15,6 @@
 
 #ifndef SETUP_MOUNT_H
 #define SETUP_MOUNT_H
-
-/* Finds the existing root mount, or returns NULL.  istext is set to
-   nonzero if the existing mount is a text mount, else zero for
-   binary. */
-
 #include <string>
 #include "String++.h"
 
@@ -27,9 +22,7 @@
 
 void create_install_root ();
 void read_mounts (const std::string);
-
-/* Sets the cygdrive flags.  Used to make the automounted drives' binary/text
-mode consistent with the standard Cygwin mounts. */
+void set_default_root_scope();
 
 std::string cygpath (const std::string&);
 void set_root_dir (const std::string);
diff --git a/root.cc b/root.cc
index 9f072d7..ccbd6ae 100644
--- a/root.cc
+++ b/root.cc
@@ -259,6 +259,18 @@ RootPage::OnInit ()
   if (!get_root_dir ().size())
     read_mounts (std::string ());
   orig_root_dir = get_root_dir();
+
+  if (!nt_sec.isRunAsAdmin())
+    {
+      // disable IDC_ROOT_SYSTEM if not running as admin
+      EnableWindow(GetDlgItem(IDC_ROOT_SYSTEM), FALSE);
+      root_scope = IDC_ROOT_USER;
+    }
+  else
+    {
+      set_default_root_scope();
+    }
+
   load_dialog (GetHWND ());
 }
 
-- 
2.37.2


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-08-26 13:27                   ` Jon Turney
@ 2022-08-26 15:02                     ` Christian Franke
  2022-08-28 17:33                       ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-08-26 15:02 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

Jon Turney wrote:
> On 23/08/2022 18:27, Christian Franke wrote:
>> Jon Turney wrote:
>>> On 12/07/2022 13:50, Jon Turney wrote:
>>>> [Replying to the right list this time...]
>>>> On 09/07/2022 13:21, Christian Franke wrote:
>>> [...]
>>>>>
>>>>> The UserSettings ctor has a somewhat hidden side effect which sets 
>>>>> root_scope correctly:
>>>>>
>>>>>   UserSettings::UserSettings(...);
>>>>>    open_settings("setup.rc", ...);
>>>>>     io_stream::open("cygfile:///etc/setup/setup.rc", ...);
>>>>> io_stream_cygfile::io_stream_cygfile("/etc/setup/setup.rc", ...);
>>>>>       get_root_dir_now();
>>>>>        read_mounts("");
>>>>>         read_mounts_nt("");
>>>>>          root_scope = isuser ? IDC_ROOT_USER : IDC_ROOT_SYSTEM;
>>>>>
>>>>> Conclusion: Regression introduced Feb 24, 2012 (befc9dd).
>>>>>
>>>>
>>>> Thanks for tracking this down.
>>>>
>>>> That just seems... fractally wrong.
>>>
>>> I kind of lost track of this.  Is there anything else needed to fix 
>>> the original problem here?  Or is it solved by the change to defer 
>>> setting the group until after root_scope is known?
>>
>> The group seems to be correctly set now.
>>
>> An old problem still remains: root_scope always ends up as 
>> IDC_ROOT_SYSTEM if setup is run elevated, regardless off GUI setting. 
>> Apply the temporary patch from here to see what happens:
>> https://sourceware.org/pipermail/cygwin-apps/2022-July/042151.html
>
> Ah, right, I remember now.
>
> I think having read_mounts() have a side effect of setting root_scope 
> is just nonsense now (it might have made some sense back in the day 
> when the mount table was also stored in the registry).
>
> So, how about the attached?
>
> (We should perhaps also set the installation root directory to 
> something other than C:\cygwin64 if a non-admin user, since they are 
> unlikely to be able to write there in current windows versions, but 
> that's difficult from a sequencing point of view in that dialog, and 
> for backwards compatibility)
>
>> Possibly difficult to fix, in particular in conjunction with later 
>> changes via [< Back] button. An easier approach: Remove the GUI 
>> setting and connect root_scope to -B option.
>
> Looking for that option is not correct, because we might be running 
> without -B, but from an already elevated shell.  I think checking 
> nt_sec.isRunAsAdmin() would be the correct test.

Did some quick tests with patch applied. LGTM.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-08-26 15:02                     ` Christian Franke
@ 2022-08-28 17:33                       ` Christian Franke
  2022-09-02 13:56                         ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-08-28 17:33 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

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

As the 'root_scope' issues are now fixed, here a reworked and enhanced 
(checkbox, setup.rc entry) version of the original patch from this thread.

With the new setting enabled, setup behaves like other install tools 
when run elevated: The installation is then also protected against 
accidental modifications by the current user.

owner:group assignments of newly installed dirs/files:

adm:adm -- "All Users", "[X] Change owner of newly installed files to 
local Administrator"
usr:adm -- "All Users"
usr:def -- "Just Me"

(usr = user running setup, adm = S-1-5-32-544, def = S-1-5-21-*-513)

An alternative for the UI would be a 3rd radio button ("All Users - 
change owner of newly installed files to local Administrator"), but the 
checkbox makes this addition IMO more obvious.

The new setup.rc setting 'root-scope' is only used to read the 
chown_admin setting but this could be enhanced, e.g. warn user if 
root_scope selection differs from previous setup run.

The drawback that files generated by postinstall scripts are still owned 
by current user could be fixed with a perpetual postinstall script. I 
could provide one for base-files package if desired.

-- 
Regards,
Christian


[-- Attachment #2: 0001-Optionally-change-owner-of-installed-files-to-local-.patch --]
[-- Type: text/plain, Size: 10277 bytes --]

From 6d996b377b4a6a908fbb4c217bda24249b4b58c1 Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Sun, 28 Aug 2022 19:23:44 +0200
Subject: [PATCH] Optionally change owner of installed files to local
 administrator

Could be selected via new checkbox in Root dialog or via new option
--chown-admin.  This choice and the root_scope are stored in new
setup.rc entry 'root-scope'.
---
 res/en/res.rc |  6 +++++-
 res/fr/res.rc |  6 +++++-
 resource.h    |  2 ++
 root.cc       | 45 ++++++++++++++++++++++++++++++++++++++++-----
 win32.cc      | 33 ++++++++++++++++++++++++---------
 win32.h       |  2 ++
 6 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/res/en/res.rc b/res/en/res.rc
index ef5e8b1..74d3d0c 100644
--- a/res/en/res.rc
+++ b/res/en/res.rc
@@ -108,7 +108,10 @@ BEGIN
     CONTROL         "Just &Me",IDC_ROOT_USER,"Button",BS_AUTORADIOBUTTON | 
                     WS_TABSTOP,13,130,130,8
     LTEXT           "Cygwin will be available to all users of the system.",
-                    IDC_ALLUSERS_TEXT,25,101,300,28
+                    IDC_ALLUSERS_TEXT,25,100,300,28
+    CONTROL         "&Change owner of newly installed files to local Administrator",
+                    IDC_ROOT_CHOWN_ADMIN,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,
+                    25,111,300,16
     LTEXT           "Cygwin will still be available to all users, but "
                     "Desktop Icons, Cygwin Menu Entries, and important "
                     "Installer information are only available to the current "
@@ -668,6 +671,7 @@ BEGIN
     IDS_HELPTEXT_ALLOW_UNSUPPORTED_WINDOWS "Allow old, unsupported Windows versions"
     IDS_HELPTEXT_ARCH "Architecture to install (x86_64 or x86)"
     IDS_HELPTEXT_CATEGORIES "Specify categories to install"
+    IDS_HELPTEXT_CHOWN_ADMIN "Change owner of newly installed files to local Administrator"
     IDS_HELPTEXT_COMPACTOS "Compress installed files with Compact OS (xpress4k, xpress8k, xpress16k, lzx)"
     IDS_HELPTEXT_DELETE_ORPHANS "Remove orphaned packages"
     IDS_HELPTEXT_DISABLE_ANTIVIRUS "Disable known or suspected buggy anti virus software packages during execution"
diff --git a/res/fr/res.rc b/res/fr/res.rc
index d081bb2..21ba8f9 100644
--- a/res/fr/res.rc
+++ b/res/fr/res.rc
@@ -102,7 +102,10 @@ BEGIN
     CONTROL         "Juste &Moi",IDC_ROOT_USER,"Button",BS_AUTORADIOBUTTON |
                     WS_TABSTOP,13,130,130,8
     LTEXT           "Cygwin sera disponible pour tous les utilisateurs.",
-                    IDC_ALLUSERS_TEXT,25,101,300,28
+                    IDC_ALLUSERS_TEXT,25,100,300,28
+    CONTROL         "&Change owner of newly installed files to local Administrator", // XXX: missing translation
+                    IDC_ROOT_CHOWN_ADMIN,"Button",BS_AUTOCHECKBOX | WS_TABSTOP,
+                    25,111,300,16
     LTEXT           "Cygwin sera disponible pour tous les utilisateurs "
                     "mais les icônes et les menus uniquement pour l'utilisateur "
                     "en cours. Ne sélectionner que si vous n'avez pas les droits "
@@ -648,6 +651,7 @@ BEGIN
     IDS_HELPTEXT_ALLOW_UNSUPPORTED_WINDOWS "Autoriser les vieilles versions de Windows"
     IDS_HELPTEXT_ARCH "Architecture à installer (x86_64 ou x86)"
     IDS_HELPTEXT_CATEGORIES "Spécifie les catégories à installer"
+    // IDS_HELPTEXT_CHOWN_ADMIN "XXX: missing translation"
     // IDS_HELPTEXT_COMPACTOS "XXX: missing translation"
     IDS_HELPTEXT_DELETE_ORPHANS "Supprimer les paquets orphelins"
     IDS_HELPTEXT_DISABLE_ANTIVIRUS "Inhibe les anti-virus buggés à l'exécution"
diff --git a/resource.h b/resource.h
index cfe860b..91446b6 100644
--- a/resource.h
+++ b/resource.h
@@ -157,6 +157,7 @@
 #define IDS_HELPTEXT_HEADER              1546
 #define IDS_HELPTEXT_FOOTER              1547
 #define IDS_HELPTEXT_NO_WRITE_REGISTRY   1548
+#define IDS_HELPTEXT_CHOWN_ADMIN         1549
 
 // Dialogs
 
@@ -296,3 +297,4 @@
 #define IDC_FILE_INUSE_HELP_0             605
 #define IDC_FILE_INUSE_HELP_1             606
 #define IDC_FILE_INUSE_HELP_2             607
+#define IDC_ROOT_CHOWN_ADMIN              608
diff --git a/root.cc b/root.cc
index ccbd6ae..766edc3 100644
--- a/root.cc
+++ b/root.cc
@@ -20,6 +20,7 @@
 #include "root.h"
 
 #include "LogSingleton.h"
+#include "UserSettings.h"
 
 #include "win32.h"
 #include <shlobj.h>
@@ -36,9 +37,11 @@
 #include "mount.h"
 #include "propsheet.h"
 
+#include "getopt++/BoolOption.h"
 #include "getopt++/StringOption.h"
 
 StringOption RootOption ("", 'R', "root", IDS_HELPTEXT_ROOT, false);
+static BoolOption ChownAdminOption (false, '\0', "chown-admin", IDS_HELPTEXT_CHOWN_ADMIN);
 
 static ControlAdjuster::ControlInfo RootControlsInfo[] = {
   { IDC_ROOTDIR_GRP,              CP_STRETCH,           CP_TOP      },
@@ -47,6 +50,7 @@ static ControlAdjuster::ControlInfo RootControlsInfo[] = {
 
   { IDC_INSTALLFOR_GRP,           CP_STRETCH,		CP_STRETCH  },
   { IDC_ROOT_SYSTEM,              CP_LEFT,              CP_TOP      },
+  { IDC_ROOT_CHOWN_ADMIN,         CP_LEFT,              CP_TOP      },
   { IDC_ALLUSERS_TEXT,            CP_STRETCH,		CP_TOP      },
   { IDC_ROOT_USER,                CP_LEFT,              CP_BOTTOM   },
   { IDC_JUSTME_TEXT,              CP_STRETCH,		CP_BOTTOM   },
@@ -69,17 +73,20 @@ RootPage::check_if_enable_next (HWND h)
 }
 
 static void
-load_dialog (HWND h)
+load_dialog (HWND h, bool chown_admin)
 {
   rbset (h, su, root_scope);
   eset (h, IDC_ROOT_DIR, get_root_dir ());
+  CheckDlgButton (h, IDC_ROOT_CHOWN_ADMIN,
+		  (chown_admin ? BST_CHECKED : BST_UNCHECKED));
 }
 
-static void
+static bool
 save_dialog (HWND h)
 {
   root_scope = rbget (h, su);
   set_root_dir (egetString (h, IDC_ROOT_DIR));
+  return !!IsDlgButtonChecked (h, IDC_ROOT_CHOWN_ADMIN);
 }
 
 static int CALLBACK
@@ -228,6 +235,15 @@ RootPage::OnMessageCmd (int id, HWND hwndctl, UINT code)
     case IDC_ROOT_DIR:
     case IDC_ROOT_SYSTEM:
     case IDC_ROOT_USER:
+      switch (id)
+	{
+	case IDC_ROOT_SYSTEM:
+	  EnableWindow(GetDlgItem(IDC_ROOT_CHOWN_ADMIN), TRUE);
+	  break;
+	case IDC_ROOT_USER:
+	  EnableWindow(GetDlgItem(IDC_ROOT_CHOWN_ADMIN), FALSE);
+	  break;
+	}
       check_if_enable_next (GetHWND ());
       break;
 
@@ -260,18 +276,26 @@ RootPage::OnInit ()
     read_mounts (std::string ());
   orig_root_dir = get_root_dir();
 
+  const char *root_scope_setting = UserSettings::instance().get ("root-scope");
+
   if (!nt_sec.isRunAsAdmin())
     {
       // disable IDC_ROOT_SYSTEM if not running as admin
       EnableWindow(GetDlgItem(IDC_ROOT_SYSTEM), FALSE);
+      EnableWindow(GetDlgItem(IDC_ALLUSERS_TEXT), FALSE);
       root_scope = IDC_ROOT_USER;
     }
   else
     {
+      // FIXME: Use root_scope_setting if set?
       set_default_root_scope();
     }
+  if (root_scope == IDC_ROOT_USER)
+    EnableWindow(GetDlgItem(IDC_ROOT_CHOWN_ADMIN), FALSE);
 
-  load_dialog (GetHWND ());
+  bool chown_admin = (ChownAdminOption || (root_scope_setting
+                     && !strcmp (root_scope_setting, "AllUsers,ChownAdmin")));
+  load_dialog (GetHWND (), chown_admin);
 }
 
 void
@@ -291,7 +315,9 @@ RootPage::OnNext ()
 {
   HWND h = GetHWND ();
 
-  save_dialog (h);
+  bool chown_admin = save_dialog (h);
+  if (root_scope == IDC_ROOT_USER)
+    chown_admin = false;
 
   if (!directory_is_absolute ())
     {
@@ -307,13 +333,22 @@ RootPage::OnNext ()
     return -1;
 
   Log (LOG_PLAIN) << "root: " << get_root_dir ()
-    << (root_scope == IDC_ROOT_USER ? " user" : " system") << endLog;
+    << (root_scope == IDC_ROOT_USER ? " user" : " system")
+    << (chown_admin ? ",chown_admin" : "") << endLog;
+
+  if (chown_admin)
+    nt_sec.setAdminAsOwner ();
+  else
+    nt_sec.setUserAsOwner ();
 
   if (root_scope == IDC_ROOT_SYSTEM)
     nt_sec.setAdminGroup ();
   else
     nt_sec.resetPrimaryGroup ();
 
+  UserSettings::instance().set("root-scope",
+                               (root_scope == IDC_ROOT_USER ? "CurrentUser" :
+			       chown_admin ? "AllUsers,ChownAdmin" : "AllUsers"));
   return 0;
 }
 
diff --git a/win32.cc b/win32.cc
index ea3d53a..467a1b2 100644
--- a/win32.cc
+++ b/win32.cc
@@ -281,12 +281,35 @@ NTSecurity::setBackupPrivileges ()
     }
 }
 
+void
+NTSecurity::setUserAsOwner ()
+{
+  if (ownerSID.user.User.Sid)
+    {
+      TOKEN_OWNER owner = { ownerSID.user.User.Sid };
+      Log (LOG_TIMESTAMP) << "Changing uid to current user" << endLog;
+      if (!SetTokenInformation (token.theHANDLE (), TokenOwner, &owner,
+				sizeof owner))
+	NoteFailedAPI ("SetTokenInformation(owner)");
+    }
+}
+
+void
+NTSecurity::setAdminAsOwner ()
+{
+  TOKEN_OWNER owner = { administratorsSID.theSID () };
+  Log (LOG_TIMESTAMP) << "Changing uid to Administrator" << endLog;
+  if (!SetTokenInformation (token.theHANDLE (), TokenOwner, &owner,
+			    sizeof owner))
+    NoteFailedAPI ("SetTokenInformation(owner)");
+}
+
 void
 NTSecurity::resetPrimaryGroup ()
 {
   if (primaryGroupSID.pgrp.PrimaryGroup)
     {
-      Log (LOG_TIMESTAMP) << "Changing gid back to original" << endLog;
+      Log (LOG_TIMESTAMP) << "Changing gid to original" << endLog;
       if (!SetTokenInformation (token.theHANDLE (), TokenPrimaryGroup,
 				&primaryGroupSID, sizeof primaryGroupSID))
 	NoteFailedAPI ("SetTokenInformation");
@@ -342,14 +365,6 @@ NTSecurity::setDefaultSecurity ()
       NoteFailedAPI ("GetTokenInformation(user)");
       return;
     }
-  /* Make it the owner */
-  TOKEN_OWNER owner = { ownerSID.user.User.Sid };
-  if (!SetTokenInformation (token.theHANDLE (), TokenOwner, &owner,
-			    sizeof owner))
-    {
-      NoteFailedAPI ("SetTokenInformation(owner)");
-      return;
-    }
   /* Get original primary group */
   if (!GetTokenInformation (token.theHANDLE (), TokenPrimaryGroup,
 			    &primaryGroupSID, sizeof primaryGroupSID, &size))
diff --git a/win32.h b/win32.h
index bf3ff10..1d31d8c 100644
--- a/win32.h
+++ b/win32.h
@@ -127,6 +127,8 @@ public:
   PSECURITY_DESCRIPTOR GetPosixPerms (const char *fname, PSID owner_sid,
 				      PSID group_sid, mode_t mode,
 				      SECURITY_DESCRIPTOR &out_sd, acl_t &acl);
+  void setUserAsOwner ();
+  void setAdminAsOwner ();
   void resetPrimaryGroup();
   void setAdminGroup ();
   void initialiseWellKnownSIDs ();
-- 
2.37.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-08-28 17:33                       ` Christian Franke
@ 2022-09-02 13:56                         ` Jon Turney
  2022-09-02 15:17                           ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-09-02 13:56 UTC (permalink / raw)
  To: cygwin-apps, Christian Franke

On 28/08/2022 18:33, Christian Franke wrote:
> As the 'root_scope' issues are now fixed, here a reworked and enhanced 
> (checkbox, setup.rc entry) version of the original patch from this thread.
> 
> With the new setting enabled, setup behaves like other install tools 
> when run elevated: The installation is then also protected against 
> accidental modifications by the current user.
> 
> owner:group assignments of newly installed dirs/files:
> 
> adm:adm -- "All Users", "[X] Change owner of newly installed files to 
> local Administrator"
> usr:adm -- "All Users"
> usr:def -- "Just Me"
> 
> (usr = user running setup, adm = S-1-5-32-544, def = S-1-5-21-*-513)
> 

Thanks.  When writing the change summary for the last RC, I wondered 
what the file owner should be.

I guess my question is, if adm:adm ownership is correct, and expected 
for consistency with other Windows installers, why not make that the 
default? and then do we really need to provide the current behaviour as 
an option, if it's "wrong".

> An alternative for the UI would be a 3rd radio button ("All Users - 
> change owner of newly installed files to local Administrator"), but the 
> checkbox makes this addition IMO more obvious.
> 
> The new setup.rc setting 'root-scope' is only used to read the 
> chown_admin setting but this could be enhanced, e.g. warn user if 
> root_scope selection differs from previous setup run.
> 
> The drawback that files generated by postinstall scripts are still owned 
> by current user could be fixed with a perpetual postinstall script. I 
> could provide one for base-files package if desired.

Doesn't this mean that we are using the wrong user-context to run those 
scripts?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-09-02 13:56                         ` Jon Turney
@ 2022-09-02 15:17                           ` Christian Franke
  2022-09-15 17:45                             ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-09-02 15:17 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

Jon Turney wrote:
> On 28/08/2022 18:33, Christian Franke wrote:
>> As the 'root_scope' issues are now fixed, here a reworked and 
>> enhanced (checkbox, setup.rc entry) version of the original patch 
>> from this thread.
>>
>> With the new setting enabled, setup behaves like other install tools 
>> when run elevated: The installation is then also protected against 
>> accidental modifications by the current user.
>>
>> owner:group assignments of newly installed dirs/files:
>>
>> adm:adm -- "All Users", "[X] Change owner of newly installed files to 
>> local Administrator"
>> usr:adm -- "All Users"
>> usr:def -- "Just Me"
>>
>> (usr = user running setup, adm = S-1-5-32-544, def = S-1-5-21-*-513)
>>
>
> Thanks.  When writing the change summary for the last RC, I wondered 
> what the file owner should be.
>
> I guess my question is, if adm:adm ownership is correct, and expected 
> for consistency with other Windows installers, why not make that the 
> default? and then do we really need to provide the current behaviour 
> as an option, if it's "wrong".

Two good questions. I'm not sure.


>
>> An alternative for the UI would be a 3rd radio button ("All Users - 
>> change owner of newly installed files to local Administrator"), but 
>> the checkbox makes this addition IMO more obvious.
>>
>> The new setup.rc setting 'root-scope' is only used to read the 
>> chown_admin setting but this could be enhanced, e.g. warn user if 
>> root_scope selection differs from previous setup run.
>>
>> The drawback that files generated by postinstall scripts are still 
>> owned by current user could be fixed with a perpetual postinstall 
>> script. I could provide one for base-files package if desired.
>
> Doesn't this mean that we are using the wrong user-context to run 
> those scripts?
>

The correct user context for running the script would be an equivalent 
to 'sudo administrator' which is not possible.

A change or addition (environment CYGWIN=chown_admin) in the Cygwin DLL 
would help: If launched with TokenOwner = Administrator, make sure that 
all newly created dirs/files are owned by TokenOwner instead of current 
user.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-09-02 15:17                           ` Christian Franke
@ 2022-09-15 17:45                             ` Jon Turney
  2022-10-04 12:05                               ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-09-15 17:45 UTC (permalink / raw)
  To: Christian Franke, cygwin-apps

On 02/09/2022 16:17, Christian Franke wrote:
> Jon Turney wrote:
>> On 28/08/2022 18:33, Christian Franke wrote:
>>> As the 'root_scope' issues are now fixed, here a reworked and 
>>> enhanced (checkbox, setup.rc entry) version of the original patch 
>>> from this thread.
>>>
>>> With the new setting enabled, setup behaves like other install tools 
>>> when run elevated: The installation is then also protected against 
>>> accidental modifications by the current user.
>>>
>>> owner:group assignments of newly installed dirs/files:
>>>
>>> adm:adm -- "All Users", "[X] Change owner of newly installed files to 
>>> local Administrator"
>>> usr:adm -- "All Users"
>>> usr:def -- "Just Me"
>>>
>>> (usr = user running setup, adm = S-1-5-32-544, def = S-1-5-21-*-513)
>>>
>>
>> Thanks.  When writing the change summary for the last RC, I wondered 
>> what the file owner should be.
>>
>> I guess my question is, if adm:adm ownership is correct, and expected 
>> for consistency with other Windows installers, why not make that the 
>> default? and then do we really need to provide the current behaviour 
>> as an option, if it's "wrong".
> 
> Two good questions. I'm not sure.

Well, perhaps we can explore that by asking what is the motivation for 
this change?  Does the current situation cause you a problem?  Is is it 
just motivated by the concern that the user running setup could 
accidentally modify the installation, or something else?

Corinna had some concerns about making the owner a group, rather than a 
user, which I believe historically caused some difficulties in Cygwin, 
so I think I'll need to understand that better before making a decision 
about this change.

>>> An alternative for the UI would be a 3rd radio button ("All Users - 
>>> change owner of newly installed files to local Administrator"), but 
>>> the checkbox makes this addition IMO more obvious.
>>>
>>> The new setup.rc setting 'root-scope' is only used to read the 
>>> chown_admin setting but this could be enhanced, e.g. warn user if 
>>> root_scope selection differs from previous setup run.
>>>
>>> The drawback that files generated by postinstall scripts are still 
>>> owned by current user could be fixed with a perpetual postinstall 
>>> script. I could provide one for base-files package if desired.
>>
>> Doesn't this mean that we are using the wrong user-context to run 
>> those scripts?
> 
> The correct user context for running the script would be an equivalent 
> to 'sudo administrator' which is not possible.
> 
> A change or addition (environment CYGWIN=chown_admin) in the Cygwin DLL 
> would help: If launched with TokenOwner = Administrator, make sure that 
> all newly created dirs/files are owned by TokenOwner instead of current 
> user.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-09-15 17:45                             ` Jon Turney
@ 2022-10-04 12:05                               ` Christian Franke
  2022-11-29 21:37                                 ` Jon Turney
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Franke @ 2022-10-04 12:05 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

Jon Turney wrote:
> On 02/09/2022 16:17, Christian Franke wrote:
>> Jon Turney wrote:
>>> On 28/08/2022 18:33, Christian Franke wrote:
>>>> As the 'root_scope' issues are now fixed, here a reworked and 
>>>> enhanced (checkbox, setup.rc entry) version of the original patch 
>>>> from this thread.
>>>>
>>>> With the new setting enabled, setup behaves like other install 
>>>> tools when run elevated: The installation is then also protected 
>>>> against accidental modifications by the current user.
>>>>
>>>> owner:group assignments of newly installed dirs/files:
>>>>
>>>> adm:adm -- "All Users", "[X] Change owner of newly installed files 
>>>> to local Administrator"
>>>> usr:adm -- "All Users"
>>>> usr:def -- "Just Me"
>>>>
>>>> (usr = user running setup, adm = S-1-5-32-544, def = S-1-5-21-*-513)
>>>>
>>>
>>> Thanks.  When writing the change summary for the last RC, I wondered 
>>> what the file owner should be.
>>>
>>> I guess my question is, if adm:adm ownership is correct, and 
>>> expected for consistency with other Windows installers, why not make 
>>> that the default? and then do we really need to provide the current 
>>> behaviour as an option, if it's "wrong".
>>
>> Two good questions. I'm not sure.
>
> Well, perhaps we can explore that by asking what is the motivation for 
> this change?  Does the current situation cause you a problem? Is is it 
> just motivated by the concern that the user running setup could 
> accidentally modify the installation, or something else?

If "All Users" is selected, the installation should IMO be protected 
against the same user if not elevated. This is automatically the case 
for other installers because Windows sets TokenOwner=Administrator if 
elevated.


>
> Corinna had some concerns about making the owner a group, rather than 
> a user, which I believe historically caused some difficulties in 
> Cygwin, so I think I'll need to understand that better before making a 
> decision about this change.

I see. Do you have any info about these difficulties?
Are these still relevant? If yes, let's forget this patch.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-10-04 12:05                               ` Christian Franke
@ 2022-11-29 21:37                                 ` Jon Turney
  2022-11-30 18:49                                   ` Christian Franke
  0 siblings, 1 reply; 18+ messages in thread
From: Jon Turney @ 2022-11-29 21:37 UTC (permalink / raw)
  To: cygwin-apps, Christian Franke

On 04/10/2022 13:05, Christian Franke wrote:
> Jon Turney wrote:
>>
>> Corinna had some concerns about making the owner a group, rather than 
>> a user, which I believe historically caused some difficulties in 
>> Cygwin, so I think I'll need to understand that better before making a 
>> decision about this change.
> 
> I see. Do you have any info about these difficulties?
> Are these still relevant? If yes, let's forget this patch.

After a bit of research, I think the issue was that if you make user 
owner and group owner map onto the same Windows SID, certain unix access 
permissions cannot be reversibly mapped onto a Windows ACL.

(e.g you can't set the mode to 0600, because when you read that back, 
it's mode is 0660. Some programs e.g ssh check for and require 0600 
permission on some files)

This perhaps isn't terribly relevant to files created by setup.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH setup] Add new option --chown-admin
  2022-11-29 21:37                                 ` Jon Turney
@ 2022-11-30 18:49                                   ` Christian Franke
  0 siblings, 0 replies; 18+ messages in thread
From: Christian Franke @ 2022-11-30 18:49 UTC (permalink / raw)
  To: Jon Turney, cygwin-apps

Jon Turney wrote:
> On 04/10/2022 13:05, Christian Franke wrote:
>> Jon Turney wrote:
>>>
>>> Corinna had some concerns about making the owner a group, rather 
>>> than a user, which I believe historically caused some difficulties 
>>> in Cygwin, so I think I'll need to understand that better before 
>>> making a decision about this change.
>>
>> I see. Do you have any info about these difficulties?
>> Are these still relevant? If yes, let's forget this patch.
>
> After a bit of research, I think the issue was that if you make user 
> owner and group owner map onto the same Windows SID, certain unix 
> access permissions cannot be reversibly mapped onto a Windows ACL.
>
> (e.g you can't set the mode to 0600, because when you read that back, 
> it's mode is 0660. Some programs e.g ssh check for and require 0600 
> permission on some files)
>

No and yes.

No, a quick test shows that stat() returns what chmod() sets even in 
this case:

# for p in 600 640 660 644 664; do f=perm-$p &&
     touch $f && chown Administrators.Administrators $f &&
     chmod $p $f && ls -l $f
   done
-rw------- 1 Administrators Administrators 0 Nov 30 18:39 perm-600
-rw-r----- 1 Administrators Administrators 0 Nov 30 18:39 perm-640
-rw-rw---- 1 Administrators Administrators 0 Nov 30 18:39 perm-660
-rw-r--r-- 1 Administrators Administrators 0 Nov 30 18:39 perm-644
-rw-rw-r-- 1 Administrators Administrators 0 Nov 30 18:39 perm-664

The above likely works due to some heuristic based on ACE order.

Yes, the effective permissions of 0600 are always the same as 0660 
because the first ACE is already for the group:

# icacls perm-\*
perm-600 BUILTIN\Administrators:(R,W,D,WDAC,WO)
          BUILTIN\Administrators:(Rc,S,RA)
          Everyone:(Rc,S,RA)

perm-640 BUILTIN\Administrators:(R,W,D,WDAC,WO)
          BUILTIN\Administrators:(R)
          Everyone:(Rc,S,RA)

perm-644 BUILTIN\Administrators:(R,W,D,WDAC,WO)
          BUILTIN\Administrators:(R)
          Everyone:(R)

perm-660 BUILTIN\Administrators:(R,W,D,WDAC,WO)
          BUILTIN\Administrators:(R,W)
          Everyone:(Rc,S,RA)

perm-664 BUILTIN\Administrators:(R,W,D,WDAC,WO)
          BUILTIN\Administrators:(R,W)
          Everyone:(R)

(Tests done on German Windows and localized names renamed afterwards).


> This perhaps isn't terribly relevant to files created by setup

It may depend on how access checks are done by ssh etc.. (mode bits or 
effective permissions).


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-11-30 18:50 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-06  7:14 [PATCH setup] Add new option --chown-admin Christian Franke
2022-07-06 13:53 ` Jon Turney
2022-07-06 16:34   ` Christian Franke
2022-07-07 11:38     ` Jon Turney
2022-07-07 14:45       ` Christian Franke
2022-07-07 14:59         ` Christian Franke
     [not found]         ` <d7d51d1c-f6d5-2fac-3e6d-86714efd0734@dronecode.org.uk>
     [not found]           ` <32655945-5075-0823-2a1d-b72caa4b7791@t-online.de>
2022-07-12 12:50             ` Jon Turney
2022-08-23 15:20               ` Jon Turney
2022-08-23 17:27                 ` Christian Franke
2022-08-26 13:27                   ` Jon Turney
2022-08-26 15:02                     ` Christian Franke
2022-08-28 17:33                       ` Christian Franke
2022-09-02 13:56                         ` Jon Turney
2022-09-02 15:17                           ` Christian Franke
2022-09-15 17:45                             ` Jon Turney
2022-10-04 12:05                               ` Christian Franke
2022-11-29 21:37                                 ` Jon Turney
2022-11-30 18:49                                   ` Christian Franke

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).