Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542

sidney has asked for the wisdom of the Perl Monks concerning the following question:

Update: I posted an update in a response below with a much simpler reproducible test case and some more insight about what is going on.

After putting aside the problem I asked about last year in Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 I went back to it and made some headway. Now I would like to see if any of you esteemed monks with more insight into the inner workings of perl will have an idea about what is going on.

I distilled the problem down to the following code which fails only on certain CPAN testers that run various older Linux and FreeBSD distros with perl versions in the 5.16 to 5.20 range, but not on all testers running the same OS and perl versions, and not on any VM I set up trying to copy the different configurations of the failing testers. So the only way I can debug this is by submitting to CPAN and look at the failure reports.

The following sequence of code results in those testers getting the insecure dependency failure. The first line creates a tainted string that is an absolute directory path that exists. It doesn't seem to matter which directory or how I get it to be tainted. The second line has to consist of the -d expression and the call to file_name_is_absolute in that order. Even though both are true and therefore both are evaluated, the error only happens if it is done in that order. Also, the error does not happen if I make it two consecutive statements rather than combined by and. The third line is the call to tempdir in which the insecure dependency happens. In my tests the directory ./log does exist, but the program dies before it actually tries to make a subdirectory under it, and I haven't tested what happens if ./log doesn't exist. Notice that the ./log directory has nothing to do with $pathdir. Also, I happened to have started out with code that had use File::Temp qw(tempdir); so that's what's here, but the results are unchanged without the qw and with the function call line having File::Temp::tempdir

my $pathdir = $ENV{HOME};
(-d $pathdir) and File::Spec->file_name_is_absolute($pathdir);
my $workdir = tempdir("temp.XXXXXX", DIR => "log");
[download]

I created an Acme module to test this, monkey patching some functions in File::Temp and File::Spec to narrow down where the taint happens. Here is one example report. All of the testers that exhibit the problem have the same results in the tests. The test t/a1.t demonstrates the failure with the above code. The other test files add monkey patching with carp output to narrow down exactly where something becomes tainted. In t/a4.t the output shows that File::Temp::tempdir calls File::Spec::Unix->catdir which calls File::Spec::Unix->canonpath passing it untainted values, and getting a tainted value returned back. In t/t5.t and t/t6.t I find that if I try to monkey patch File::Spec::Unix->canonpath even making the patch a duplicate of the original source code, the error does not happen, the return value is not tainted. I can't see anything in the code in File::Spec::Unix->canonpath that could possibly result in a tainted value given untainted input.

I specified File::Temp and File::Spec as required dependencies in Makefile.PL so that the reports would list which versions the testers were running. You can see that both are the latest released versions, which is what I based the monkey patches on.

Does this look like some obscure bug in these versions of perl? Any ideas?

Comment on Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 Select or Download Code

Replies are listed 'Best First'.
Re: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by hv (Prior) on Apr 21, 2024 at 11:22 UTC
Does this look like some obscure bug in these versions of perl? Based on the information you've given so far, it sounds like a bug somewhere, but more likely in one of the dependent modules. As a starting point I suggest getting the module versions for yourself: usually `use Some::Module; print $Some::Module::VERSION;` should be enough. You may also need to check any other modules used by `qw{ File::Spec File::Spec::Unix File::Temp }` that could possibly be getting involved. My first guess is that one of the modules initializes a package variable or file-scope lexical in a way that happens to taint it from one of your `$pathdir` calls, and `tempdir()` then calls into the same module in a way that causes it to use that variable. If you're lucky you might also find something that stands out as relevant in the changelog files of the relevant packages.	[reply] [d/l] [select]
Update: Simpler reproducible test case by sidney (Acolyte) on Apr 24, 2024 at 03:46 UTC
I figured out how to reproduce this reliably on my own VM without a CPAN tester, and was able to figure out most of what is happening. It does not need the call to tempdir to demonstrate the bug, it can be shown as an unexpectedly tainted return from canonpath. `File:Spec` is part of `PathTools`, which also includes `Cwd`. Version 3.56_01, in perl 5.22.0, added `File::Spec` functions in C to `Cwd.xs`, including the Unix version of canonpath. Version 3.62 added the line of code that causes the xs canonpath to return a tainted value if any of the inputs are tainted. The environment: On any Unix or Linux system install perlbrew and use it to install perl version 5.20.3 or older. I tried this on 5.20.3, 5.18.4, 5.16.3 and 5.14.3. On an OS with a recent C compiler you may need to use the workaround described in the last comment in this perlbrew bug report. Use `perlbrew switch` to run with that installed perl. If you print File::Spec->VERSION you will see that it is an old version. Run `cpanm File::Spec` to get the latest release. The bug appears to be something wonky going on when installing a version of PathTools that has an xs version of File::Spec::Unix->canonpath on top of an old perl that has only a perl version of it. Or maybe those old perls have something else wrong in the SvTAINT macro that has a subtle effect that this is revealing? Here is a self-contained simple example that produces the tainted return from canonpath when run in the environment set up as above. As in my original example, `$pathdir` has to be set to a tainted string value of an absolute pathname that exists, and the line after that has to evaluate the `-d` and call to the function in that order, connected in one statement by `and`. The sub that is in there can be anything that causes a bareword file handle (but not STDERR, STDOUT, etc.) to be compiled. It can be put before or after the other code. It doesn't have to be run, just compiled. In fact, instead of the sub you can put in a `use` of any module whose loading compiles a statement that contains a bareword file handle, for example `use Test;`. Put this code in a file and run it with `perl -T` If you change any of the required conditions, for example replace the `and` with a semicolon to make it two statements, and run it, you will see the proper behavior that canonpath does not return a tainted value. `#!/usr/bin/perl -T use File::Spec; use Scalar::Util; my $pathdir = $ENV{HOME}; # tainted, existing dir, absolute path (-d $pathdir) and File::Spec->file_name_is_absolute($pathdir); my $stuff = File::Spec->canonpath("foo/bar"); print "\nerror $stuff is tainted\n" if Scalar::Util::tainted($stuff); sub foo { open(BAREWORD, "<foobar") \|\| return; close(BAREWORD); }` [download]	[reply] [d/l] [select]
Re^2: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by sidney (Acolyte) on Apr 21, 2024 at 23:24 UTC
Instead of printing out VERSION, I specified in Makefile.PL that `File::Temp` and `File::Spec` are required dependencies. That causes them to be listed with their version numbers in the PREREQUISITES section of the report. They are both the latest released version, which is what I used to get the source code for the monkey patches in the test files. If you look at the carp output produced by t/a4.t you can see that `File::Spec::Unix->canonpath` is called with untainted data and it returns tainted data. The source code for File::Spec::Unix->canonpath is very simple and has no variables that are not declared in `my`. It doesn't even have any use of `$1` that could have that scope problem.	[reply] [d/l] [select]
Re: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by ikegami (Patriarch) on Apr 21, 2024 at 14:25 UTC
Does this look like some obscure bug in these versions of perl? At a glance, the grid seems to indicate a problem with certain versions. But if you look more closely, the problem seems to be specific to the tests run by BINGOS, not specific to a version. This is most obvious is you look at the tests for 4.0.1 on 5.18.4. Every one of BINGOS's tests fails due to a taint error, while neither DCANTRELL's nor Slaven Rezić's tests do. Have you spoken to BINGOS?	[reply]
Re^2: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by sidney (Acolyte) on Apr 21, 2024 at 23:42 UTC
This grid is a better one to look at. SpamAssassin 4.0.1 test grid has other things going on. The Acme::TaintTest module I wrote is supposed to isolate just this one problem. Since it only happens on certain test machines, I have to upload a test and then wait for as many of the relevant test machines as possible to happen to pick it up. As of the time I'm writing this, that grid for Acme-TaintTest 0.0.8-rc1b-TRIAL does show the taint error in some, but not all or most, of BINGOS and Slaven Rezić's machines. I tried reaching out to them when all I knew about this was what I could glean from the SpamAssassin test failures, but that didn't go anywhere. I'm hoping to narrow things down much more with the Acme::TaintTest series of tests and the conversation here before I reach out to them again.	[reply]
Re: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by afoken (Chancellor) on Apr 21, 2024 at 10:57 UTC
I cannot reproduce your problem. The code you posted simply does not work. I added some required lines: `#!/usr/bin/perl -T use strict; use warnings; use File::Spec; use File::Temp qw( tempdir ); my $pathdir = $ENV{HOME}; (-d $pathdir) and File::Spec->file_name_is_absolute($pathdir); my $workdir = tempdir("temp.XXXXXX", DIR => "log");` [download] Running that with taint mode, i.e. `perl -T perlmonks.pl`, creates the following error: $ perl -T perlmonks.pl Error in tempdir() using log/temp.XXXXXX: Parent directory (log) does +not exist at perlmonks.pl line 10. $ [download] After creating the missing directory, there are no more errors or warnings: `$ mkdir log $ perl -T perlmonks.pl $` [download] Please post a Short, Self-Contained, Correct Example. Generally, taint errors indicate that you have used data from outside Perl (i.e. environment, command line parameters, file contents) without a formal validation. See Re: When not to use taint mode for what taint mode actually does. It is pretty simple, not much more than a flag on a value that is hard to remove, "infects" other values with most operations, and causes a crash when a flagged ("tainted") value passed to selected functions. Alexander -- Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)	[reply] [d/l] [select]
Re^2: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by sidney (Acolyte) on Apr 21, 2024 at 23:10 UTC
Alexander - I quoted just the most relevant lines of code. The link I posted to the failing test report shows it failing. As I meant to imply, the test `t/a1.t` in that module is the Short, Self-Contained, Correct Example of the problem, the other test files add monkey patches to try to diagnose where the taint happens. Like you, I cannot reproduce the error myself, it only happens on certain CPAN test machines and not on any attempt I've made to create a VM with the same OS and perl versions. As I said, my tests run with `./log` already existing in the environment. When `./log` exists, correct behavior is no error. Without `./log` correct behavior is the directory does not exist error. My test module has the log directory in the MANIFEST so that the only failures on CPAN are due to the bug. I think I understand what taint mode does. The monkey patched test in `t/a4.t` shows that untainted data is being passed to `File::Spec::Unix->canonpath` whose source code does not make use of any external data, and the return value from that function call is tainted. When I try to monkeypatch canonpath the error does not happen even on the relevant CPAN test machines. It's a heisenbug. Hence the mystery I am asking about.	[reply] [d/l] [select]
Re: Revisiting Insecure dependency in mkdir while running with -T switch at ... File/Temp.pm line 542 by bliako (Abbot) on Apr 24, 2024 at 18:07 UTC
afoken's refresher on what can cause failures on taint mode mentions environment variables. And made me check File::Spec::Unix's doc: `File::Spec::Unix checks $ENV{TMPDIR} (unless taint is on) and /tmp.` [download] Perhaps, the above is not the source of the problem (e.g. "`unless taint is on`") but other environment variables set on some tester machines may be. So, with your test script, you may also want to report on the ENV and loaded modules and their versions: `print $_."=>".$ENV{$_}."\n" for sort keys %ENV; print $_."=>".$INC{$_}." (v".((($_ =~ /^(.+?)\.pm$/)[0]=~s{/}{::}gr)[0 +])->VERSION.")\n" for sort keys %INC;` [download] bw, bliako	[reply] [d/l] [select]