Re: [PATCH setup] Add new option --chown-admin

[Christian Franke <Christian.Franke@t-online.de>]

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.