155861 - showdependancygraph.cgi fails taint check with local dot installation

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Carl Makin]

Trying to show the dependancy graph using a locally installed version of dot
causes perl to throw a taint "insecure dependancy in showdependancygraph.cgi at
line 192" error.

This line is;

foreach my $f (glob("data/webdot/*.dot 
                     data/webdot/*.png 
                     data/webdot/*.map"))

There is a "trick_taint" further down in the loop which I guess is supposed to
stop this, but it seems to be the glob that is causing it.

I've got it working by replacing the above 3 lines with the following 4;

opendir(DIR, "data/webdot/");
my @taintf = grep { /\.dot$|\.png$|\.map$/ } readdir(DIR);
closedir(DIR);
foreach my $f (@taintf)


I'm not 100% sure the regex is right, but it seems to work now and the graph is
generated.  I'll have to wait a few days to so to see if it is deleting old graphs.

This is in Bugzilla v2.16RC2 running under FreeBSD 4.3-STABLE with Perl 5.005_03.

Comment 1

[Bradley Baetz (:bbaetz)]

-> 2.16

just calling glob('*.foo') is insecure in 5.0055, because it uses csh to do the
expansion - 5.8.0RC2 adds to teh perlsec man page:

# In Perl releases older than 5.6.0 the <*.c> and glob('*.c') would
# have used an external program to do the filename expansion; but in
# either case the result is tainted since the list of filenames comes
# from outside of the program.

We need to use readidr, but when deleteing, we also need to prepend 'data/graphs'

-> 2.16

Comment 2

[Carl Makin]

oops missed that.

So we need to add something like this;


    my $pathf = "data/webdot/" . $f;
    if (ModTime($pathf) < $since) {
        unlink $pathf;
    }

at line 209.

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: v1

Before I could test this I needed to get the perms set right. This changes
checksetup.pl to do that.

I don't have access to a perl5.005 system to test this on, since landfill is
down.

Comment 4

[Bradley Baetz (:bbaetz)]

-> patch author (me)

Comment 5

[Gervase Markham [:gerv]]

Comment on attachment 90452 [details] [diff] [review]
v1

r=gerv.

Gerv

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 90452 [details] [diff] [review]
v1

Works, looks-good.  r=myk

Am I right in thinking that mimedump-tmp is used only by contrib scripts and
shouldn't be created by checksetup.pl (and that's why you removed its creation
from the script)?

Comment 7

[Bradley Baetz (:bbaetz)]

I grepped for uses of that dir, but forgot to grep contrib. I've left that line
in, but because of bug 156568 its never actually run, anyway.

I don't have a perl 5.005 bz installation, but I did test this snippit on
solaris 5.005_02 without problems

Checked into trunk + branch

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.160; previous revision: 1.159
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <-- 
showdependencygraph.cgi
new revision: 1.20; previous revision: 1.19
done

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.149.2.8; previous revision: 1.149.2.7
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <-- 
showdependencygraph.cgi
new revision: 1.18.2.1; previous revision: 1.18
done