Skip Menu |
 

This queue is for tickets about the Perl-Tidy CPAN distribution.

Report information
The Basics
Id: 128477
Status: open
Priority: 0/
Queue: Perl-Tidy

People
Owner: perltidy [...] users.sourceforge.net
Requestors: tlhackque [...] yahoo.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: File ownership not preserved
Date: Mon, 11 Feb 2019 14:15:30 -0500
To: bug-Perl-Tidy [...] rt.cpan.org
From: tlhackque <tlhackque [...] yahoo.com>
Download (untitled) / with headers
text/plain 1.9k
It seems that perltidy preserves permissions, but not ownership.  This is a (potential security) problem with suid/sgid scripts. E.g.  (this is Linux) #  perltidy --version This is perltidy, v20181120 # ls -l pdft -rwsr-sr-x 1 noreply noreply 27280 Feb 11 12:46 pdft # perltidy -b pdft # ls -l pdft -rwsr-sr-x 1 root devel 26712 Feb 11 13:46 pdft In this case, the working directory is g+s to 'devel' Note that the permissions were retained, but the *ownership* (both user and group) changed. This is not good; running the script would setuid and setgid to the wrong users! User 'noreply', Group 'noreply' were intended; 'root', 'devel' are the result. That's a potential security issue... when the developer tests the reformatted code, and even when the script ships. There are some choices for how to fix: a) don't preserve the setuid/setgid bits b) attempt to change file ownership; if that fails, see (a) c) don't preserve permissions at all (b) seems like the right choice.  (a) is safe, but annoying.  (c) would be a regression. Note that the perltidy user may not have permission to change ownership (on most systems you need to be the superuser to change owner; changing groups is less constrained), so blindly changing it would be a mistake.  (always check return codes; the current chmod does not.)  But it's pretty easy to make the attempt, then compare input file with output - clear the setxid bits if ownership doesn't match. (It might match by default, inheritance, or if you do an explicit chown.)  You only need to clear setuid if uid mismatches; likewise clear setgid if gid mismatches. See Tidy.pm line 1328 in the current release.  The logic there adds u+rw, which seems like a hack. Also, consider passing the output file handle to chcon & chown(avoids any races). What to do about ACLs and/or security contexts is left as an opportunity for the reader...  They would seem to have similar issues - but they ought to inherit, and are more platform-specific. Thanks.
Thanks, I'll fix that in the next release. Steve
Download (untitled) / with headers
text/plain 170b
On Sun Feb 17 10:17:06 2019, SHANCOCK wrote: Show quoted text
> Thanks, I'll fix that in the next release. Steve
I forgot to add that this problem is specific to when -b option is used.
setuid/setgid are consistent with file ownership in v20190601. I will leave this ticket open.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.