From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout04.t-online.de (mailout04.t-online.de [194.25.134.18]) by sourceware.org (Postfix) with ESMTPS id 4429D3857B99 for ; Thu, 7 Jul 2022 14:45:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4429D3857B99 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=t-online.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=t-online.de Received: from fwd81.dcpf.telekom.de (fwd81.aul.t-online.de [10.223.144.107]) by mailout04.t-online.de (Postfix) with SMTP id F408D7FB4 for ; Thu, 7 Jul 2022 16:45:02 +0200 (CEST) Received: from [192.168.2.102] ([87.187.34.65]) by fwd81.t-online.de with (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384 encrypted) esmtp id 1o9SkY-1601qa0; Thu, 7 Jul 2022 16:45:02 +0200 Subject: Re: [PATCH setup] Add new option --chown-admin To: "cygwin-apps@cygwin.com" References: <3096f251-d7ca-073b-d7d7-751b7fe3e8c1@t-online.de> <405df5c6-ce47-0254-ae4d-4a23ff3533d5@dronecode.org.uk> <5b45ccdc-da32-ff11-037f-c00828f397c5@dronecode.org.uk> From: Christian Franke Message-ID: Date: Thu, 7 Jul 2022 16:45:02 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 SeaMonkey/2.53.12 MIME-Version: 1.0 In-Reply-To: <5b45ccdc-da32-ff11-037f-c00828f397c5@dronecode.org.uk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-TOI-EXPURGATEID: 150726::1657205102-0143E336-E2A2108E/0/0 CLEAN NORMAL X-TOI-MSGID: 4a7317f6-2ba2-48b0-9aaf-ec721f7236b1 X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00, FREEMAIL_FROM, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: cygwin-apps@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin package maintainer discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Jul 2022 14:45:55 -0000 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.