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