Issue with perl in Tainted mode

[brunobergamaschi]

I'm currently migrating one of our systems that runs on Template::Toolkit to Text::Xslate (version 3.1.2) with TTerse syntax and I think that I found a little bug when running on tainted mode perl.

It happens in the following case:

TemplateA (includes TemplateB)
TemplateB

If I change the content of TemplateB file and generate new content with TemplateA, perl throws the following error:

Text::Xslate: Insecure dependency in unlink while running with -T switch 
at /usr/local/lib/perl/5.10.1/Text/Xslate.pm line 394.

Here is the code snippet of Xslate.pm that throws the error:

    if(-e $cachepath) {
        unlink $cachepath
            or Carp::carp("Xslate: cannot unlink $cachepath (ignored): $!");
    }

Seems that the $cachepath variable of include statements are not untainted, thus the error. Right now I don't know if it's only related to TTerse syntax or the others too. I'll make some test scripts to help investigate this.

[syohex]

You should pass absolute path(and untainted) both path and cache_dir parameters
of constructor in taint mode. This is referred to documentation.

Note that if you use taint mode (-T), you have to give absolute paths to path and cache_dir. Otherwise you'll get errors because they depend on the current working directory which might not be secure.

https://metacpan.org/pod/Text::Xslate#Text::Xslate-new-options

[brunobergamaschi]

Those two variables were already untainted absolute paths, and I did an additional untainting using a dumb regex (/.*/), just to be 100% sure. If they weren't I would get errors when using any template.

The issue don't happen when a template with no includes is used and the template is changed. Only when an included one is changed.

It seems that the included template path (like in [% INCLUDE "another_template.tpl" %]) is not properly untainted when extracted to be used in dangerous operations, like the unlink from the cache when it's outdated.

[syohex]

Could you show us code and templates(as small as possible) which reproduce this issue ?

[brunobergamaschi]

Sure!
I'll grab one of the benchmark scripts and modify it to reproduce the issue.

[brunobergamaschi]

It's done.
How can I send the test to you?

[syohex]

Please paste here(if they are not too large)or update gist or somewhere,
or creating repository.

[brunobergamaschi]

I uploaded the test to:

http://bruno.arqs.com.br/perl/text_xslate_issue_taintmode_test.zip

Just update the paths inside the script to match those of your system.
Run it the first time. It will work.
Run it the second time. It will fail.

I just run this test on 3 servers I have and it failed on the second run on all 3.
v5.8.8 built for x86_64-linux-thread-multi
and
v5.10.1 (*) built for x86_64-linux-gnu-thread-multi

[brunobergamaschi]

I made an additional test here using Kolon syntax, and the same thing happens.

[brunobergamaschi]

After some time doing a little debugging I found a place where it breaks.
I don't know if it's the ideal location for a fix, since I don't have enough time right now to look deeper in the code and test.

In the file Text/Xslate.pm I made a quick hack to test if the $file was tainted, and to no surprise it was, confirming my suspicions.

sub load_file {
    my($self, $file, $mtime, $omit_augment) = @_;

    local $self->{omit_augment} = $omit_augment;
    print STDERR "Tainted file name: $file\n" if (Scalar::Util::tainted($file));    
    ($file) = ($file =~ /(.*)/s);

When run with the test script it issues the warning to STDERR and processes the template normally after the dirty untainting.

[brunobergamaschi]

Any news on this?

[syohex]

Sorry, it makes no progress.

Would it be possible to disable taint mode for the files which uses including template ?

I think ($file) = ($file =~ /(.*)/s); might fix this issue, but it makes no sense
for taint mode. It just overlooks untainted value and same as untaint-mode.

[brunobergamaschi]

Unfortunately, I can't disable taint mode in the app i'm using Xslate.

I have a question: is the $file parameter in load_file already sanitized upon being called?
If it is, then the regex in load_file will do the job, if it's not then it might introduce a security issue (which already existed if the var wasn't sanitized before).

Thinking about it further, might be happening the following problem:
The include name is parsed properly on compilation in the first time and processed accordingly.
Then, in subsequent loads (from cache), it's not parsed and perl think that the cached filename is tainted.

In that case, the catchall regex is harmless and solves the problem.

I've found only those spots that call load_file:

Xslate/PP.pm:509:        $self->load_file( $name, undef, $from_include );
Xslate/PP.pm:532:        $self->load_file( $name, $cache_mtime, $from_include );
Xslate/Compiler.pm:432:        $base_code = $engine->load_file($base_file);
Xslate/Compiler.pm:447:        my $code     = $engine->load_file($cfile);
Xslate.pm:350:    $self->note("%s->load_file(%s)\n", $self, $file) if _DUMP_LOAD;

The $file parameter in load_file comes from the return of _bare_to_file in the Compiler.pm:

sub _bare_to_file {
    my($self, $file) = @_;
    if(ref($file) eq 'ARRAY') { # myapp::foo
        return join('/', map { $_->value } @{$file}) . $self->{engine}->{suffix};
    }
    elsif($file->arity eq 'literal') {
        return $file->value;
    }
    else {
        $self->_error("Expected a name or string literal", $file);
    }
}