Bug #122820 for File-Temp: Change in tempdir behavior requires change in documentation

Summary: Because of a change in the behavior of File::Path::rmtree() for security reasons, the behavior of File::Temp::tempdir() has also changed. This change of behavior should be documented and illustrated with tests. A. Why Is File::Temp Affected by a Security Vulnerability in File::Path? File::Temp::tempdir()'s 'CLEANUP => 1' option is implemented with internal calls to File::Path::rmtree() (https://metacpan.org/source/DAGOLDEN/File-Temp-0.2304/lib/File/Temp.pm): ##### eval { rmtree($dir, $DEBUG, 0); }; # line 784 eval { rmtree($self->{REALNAME}, $File::Temp::DEBUG, 0); }; # line 1616 ##### The Perl-false value in the third positional argument means, in effect, "Do everything you can to remove all directories under the temporary directory, including directories on which you don't have normal permissions." The "everything you can" includes a lot of filesystem checks and attempts to chmod directories with fairly exotic permissions such as 0000 and 0200 -- directories on which the user lacks both 'read' and 'execute' permissions. This stands in contrast to a rmtree() call with a Perl-true value in the third parameter: ##### eval { rmtree($self->{REALNAME}, $File::Temp::DEBUG, 1); }; ##### In this case, rmtree() is documented to "skip the files for which the process lacks the required privileges needed to delete files .... In other words, the code will make no attempt to alter file permissions." (https://metacpan.org/pod/File::Path) In practice, this means that rmtree() with a true value in the third parameter will not remove a directory unless its permissions are something like 0700. Unfortunately, the hoops which rmtree() must internally jump through to remove directories regardless of permissions require many filesystem checks which introduce the possibility of "Time of Check to Time of Use" (TOCTTOU: https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use) situations which, in turn, introduce security vulnerabilities due to race conditions. These vulnerabilities are sufficiently serious that a CVE has been filed and the behavior of rmtree() in File::Path versions 2.14 and later has been changed (https://rt.cpan.org/Ticket/Display.html?id=121951). (These changes have been merged into Perl 5 blead.) The practical impact of this change is that rmtree() will no longer be able to directly remove directories on which the user lacks both 'read' and 'execute' permissions. Any code, including File::Temp::tempdir(), which uses rmtree() for recursive removal of directories will be similarly affected. Hence, the need to modify File::Temp::tempdir()'s documentation. B. Example of Change in tempdir()'s Behavior and How to Address An example of such code would be: ##### my $tdir = tempdir(CLEANUP => 1); my $subdir = File::Spec->catdir($tdir, 'abc'); mkdir $subdir; chmod 0000, $subdir; ... # do something with $subdir # CLEANUP triggered, making internal call to File::Path::rmtree() ##### Up through File::Path version 2.12 -- or up through perl-5.26.0 -- 'CLEANUP' would remove $subdir even though the user no longer has any permissions on it. Very convenient, but (as discussed above) performed via filesystem checks which open up security vulnerabilities. From File::Path version 2.14 forward -- or from perl 5.27.3 forward -- the rmtree() call inside the 'CLEANUP' option will no longer remove $subdir. The user will get warnings like this: ##### cannot chdir to child for /tmp/caIID_gLXE/abc: Permission denied at /home/jkeenan/testing/blead/lib/perl5/5.27.3/File/Temp.pm line 784. cannot remove directory for /tmp/caIID_gLXE: Directory not empty at /home/jkeenan/testing/blead/lib/perl5/5.27.3/File/Temp.pm line 784. ##### Manual inspection of /tmp shows that access was denied to the second-level temporary directory, preventing its removal and the removal of its parent directory. ##### $ ls -ld /tmp/caIID_gLXE/ drwx------ 3 jkeenan jkeenan 4096 Aug 16 09:40 /tmp/caIID_gLXE/ $ ls -l /tmp/caIID_gLXE/ total 4 d--------- 2 jkeenan jkeenan 4096 Aug 16 09:40 abc $ ls -l /tmp/caIID_gLXE/abc ls: cannot open directory '/tmp/caIID_gLXE/abc': Permission denied ##### But the code sample can easily be modified to restore reasonable permissions before tempdir() triggers cleanup. Under File::Path 2.14 (and perl 5.27.3) and later, we call: ##### my $tdir = tempdir(CLEANUP => 1); my $subdir = File::Spec->catdir($tdir, 'abc'); mkdir $subdir; chmod 0000, $subdir; ... # do something with $subdir chmod 0700, $subdir; # CLEANUP triggered, making internal call to File::Path::rmtree() ##### $subdir's permissions now permit File::Path::rmtree() to remove it. This in turns permit's tempdir()'s 'CLEANUP' option to remove it. All the tempdirs are removed and the user is no longer presented with warnings. (See attachments 'tempdir-cleanup.pl' and 'adjusted-tempdir-cleanup.pl' for program exemplars.) C. Scope of Impact I anticipate that the maintainers and users of File::Temp will have concerns about this change in tempdir()'s behavior. tempdir() is undoubtedly used extensively in production code, but since we have no access to such code, we have to treat the entirety of Perl code on CPAN as a proxy. Over the past several weeks, I conducted a two-stage study to address these concerns. 1. I grepped CPAN for code which creates directories without both 'read' and 'execute' permissions or so modifies permissions. That includes code like this: ##### mkdir $some_directory, 0000 mkdir $some_directory, 0200 mkdir $some_directory, 0 chmod 0000, $some_directory, $some_other_directory chmod 0200, $some_directory, $some_other_directory chmod 0, $some_directory, $some_other_directory File::Path::mkpath('foo/bar/baz', '/zug/zwang', { verbose => 1, mode => 0000 }) File::Path::mkpath('/foo/bar/baz', 1, 0000) File::Path::make_path('foo/bar/baz', '/zug/zwang', { mode => 0000 } ##### While I make no claim that this search was exhaustive, I located enough examples to have confidence in my inferences. (See https://metacpan.org/source/JKEENAN/Regexp-Functions-chmod_et_al-0.02/t/002-chmod-et-al.t for the full list of code strings searched for). 2. I then grepped the files identified in step #1 for 'tempdir' and 'newdir'. (In File::Temp's object-oriented interface $t->newdir is equivalent to 'tempdir(CLEANUP => 1)'.) I manually inspected those files for locations where the function calls discussed in step #1 fell within the scope of tempdir(). In all of CPAN I could locate only six files which needed patching. I created and submitted patches or pull requests for all six. ##### G/GR/GRAY|IO-AIO-Util-0.09.tar.gz|t/01_mkpath.t M/MA/MANWAR|Filesys-DiskUsage-0.10.tar.gz|t/02.warnings.t P/PE/PERLANCAR|File-MoreUtil-0.61.tar.gz|t/01-basics.t P/PE/PETDANCE|ack-2.18.tar.gz|t/ack-s.t S/SN/SNOWHARE|Tie-DB_File-SplitHash-1.05.tar.gz|t/01_Tie-DB_File-SplitHash.t X/XA/XAN|Archive-Tar-Builder-2.5002.tar.gz|t/lib-Archive-Tar-Builder.t ##### I am fairly confident that this substantially addresses the problem, as six is one more than the number of problematic files separately identified by experienced CPAN tester Slaven Rezic earlier this year (https://rt.cpan.org/Ticket/Display.html?id=122255#txn-1735608). D. Next Steps I can prepare a pull request for (a) changes in File::Path's documentation discussing this change in behavior; and (b) additional unit tests illustrating the change in behavior. But I would like to get some feedback from File::Temp's maintainers first. Thank you very much. Jim Keenan (maintainer of File::Path)