Remove Taint::Util

[davorg]

I'm puzzled by your use of Taint::Util in this distribution. For two reasons.

Firstly, Taint::Util::untaint() is a terrible function to use unconditionally. It's the equivalent of just ignoring warnings that your code is throwing. If you're getting a warning about insecure data, you should investigate it and work out if there's actually a problem. You certainly shouldn't just mark the data as clean.

Taint::Util::untaint() should only ever be used after you've checked that there's nothing nasty in your data. Something like this:

if (this_data_is_really_clean($data)) {
  untaint($data);
}

And, secondly, I can't see anywhere in your code where taint mode gets turned on. So I have no idea why you were getting the insecure dependency you mention in commit https://github.com/manwar/Map-Tube/commit/46c512b595afb68b0637c4c2bed12ca9442f4357

Can you remember what was causing the problem? I'd like to create a test that fails without the call to untaint(), so I can then find a cleaner way to mark the data as clean.

I'm thinking of writing a blog post on taint mode as I don't see people talking about it much these days. And your code would make a really good example.

[manwar]

Honestly speaking I don't remember exactly why I did it. I vaguely remember someone reported running the code with -T throwing taint related error. With my limited knowledge on the subject, I tried my best. As you correctly pointed out, it is badly implemented.

Can you please improve it? Also if you could please show us the better way of dealing with taint issue?

Thanks for your help as always, much appreciated.

[davorg]

Aha. I've found a little more explanation of the problem in your talk from Riga in 2019.

https://www.youtube.com/watch?v=aG4VBITkS24

And I've also remembered sitting with Julien in the audience, thinking "you really shouldn't tell people to do that!"

I'll investigate further.

[mohawk2]

Since I'm making PRs, want me to just remove this? If someone uses -T and has a problem, it shouldn't be masked.

Also, @manwar want me to incorporate Map::Tube::Exception and Map::Tube::Plugin::Formatter? It seems as obvious to me they should be in the main thing, since they're stated as prereqs for normal operation.

[davorg]

Looking at it in a little more detail, I think I see the problem. I suspect it's down to to_perl() opening and reading a file without checking the path. So that $file variable could contain ../../../etc/passwd or something like that.

I don't know where the file is expected to live or what a valid filename would look like here (perhaps @manwar can help with that).

But I think you have two options:

1. Remove Taint::Util and the call to untaint(). This would, of course, leave the original bug unfixed - but I'm not sure how much we care about that :-)

2. Rewrite to_perl() so it checks the value in $file before opening the file. So it becomes something like:

   sub toperl { my ($file) = @;

   if (is_scary_looking_file($file) {
       die "That's a scary looking file: [$file]\n";
   }
   
   my $json_text = do {
       open(my $json_fh, "<", $file) or die("ERROR: Can't open $file: $!\n");
       local $/;
       my $text = <$json_fh>;
       close($json_fh);
       $text;
   };
   
   untaint $json_text;
   return JSON->new->allow_nonref->utf8(1)->decode($json_text);

   }

Where is_scary_looking_file() would use a regex to check that $file looks legit.

Basically, my big concern here is that using Taint::Util::untaint() in the way it's used in this code is a VERY BAD IDEA. And someone as respected in the community as @manwar should be careful about people getting bad ideas from their code.

[manwar]

@davorg @mohawk2 Just to give more information so that we can all agree to a better solution/approach. It has been a long gap since I looked at the issue. I vaguely remember someone raised an issue (I have no record, sorry) that when creating a map using JSON format map data with taint mode on, it was throwing "Insecure dependency in eval while running with -T switch".

During my brief research, it turned out the get_map_data() in the package Map::Tube is doing eval { $data = to_perl($json) }; I don't recall why I am doing eval{} in the first place. If I am doing to json file then why not xml file also?

I tried the following to re-create the error but no luck.

good-map.json

{
    "name"  : "test",
    "lines" : {
        "line" : [
            { "id" : "A", "name" : "A", "color" : "red"     },
            { "id" : "B", "name" : "B", "color" : "#FFFF00" }
        ]
    },
    "stations" : {
        "station" : [
            { "id" : "A1", "name" : "A1", "line" : "A:2",     "link" : "A2"    },
            { "id" : "A2", "name" : "A2", "line" : "A:1",     "link" : "A1,A3" },
            { "id" : "A3", "name" : "A3", "line" : "A:3,B:3", "link" : "A2,B1" },
            { "id" : "B1", "name" : "B1", "line" : "A:4,B:1", "link" : "A3,B2" },
            { "id" : "B2", "name" : "B2", "line" : "B:2",     "link" : "B2"    }
        ]
    }
}

good-map.pl

#!/usr/bin/perl -T

package Good::Map;

use Moo;
use namespace::clean;

has json => (is => 'ro', default => sub { 'good-map.json' });
with 'Map::Tube';

package main;
use strict; use warnings;

my $map = Good::Map->new;
print $map->get_shortest_route('A2', 'B1');

Then ran with taint on:

$ perl -T good-map.pl
A2 (A), A3 (A, B), B1 (A, B)

My guess is the map data (xml or json) which is supplied by user as full path. It is the main cause of taint is not happy, again I am guessing. I think Dave is right in suggesting to get rid of Taint::Util completely. And to safeguard the data, we should sanitise the user map data file path. A simple regex would be good enough, in my humble opinion.

[mohawk2]

I believe that if people use perl -T they want to be warned about things like this, and if they don't use -T, then it's in a trusted environment. Blindly untainting data in this set of modules doesn't seem like the right place for that activity. If a module user wants to do their own checking (and untainting) of their data before passing it to your module, they will know their needs better than you?