Bug 419014: (CVE-2010-3764) [SECURITY] Old charts are not project specific, and product names are viewable in graphs/

r=wurblzap a=LpSolit

6 files changed, 75 insertions, 78 deletions

diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm
index 4ca04c478..9a0f6cd2e 100644
--- a/Bugzilla/Constants.pm
+++ b/Bugzilla/Constants.pm
@@ -623,6 +623,7 @@ sub bz_locations {
         'datadir'     => $datadir,
         'attachdir'   => "$datadir/attachments",
         'skinsdir'    => "$libpath/skins",
+        'graphsdir'   => "$libpath/graphs",
         # $webdotdir must be in the web server's tree somewhere. Even if you use a 
         # local dot, we output images to there. Also, if $webdotdir is 
         # not relative to the bugzilla root directory, you'll need to 
diff --git a/Bugzilla/Install/Filesystem.pm b/Bugzilla/Install/Filesystem.pm
index 42f292b6d..0bad63708 100644
--- a/Bugzilla/Install/Filesystem.pm
+++ b/Bugzilla/Install/Filesystem.pm
@@ -122,6 +122,7 @@ sub FILESYSTEM {
     my $skinsdir      = bz_locations()->{'skinsdir'};
     my $localconfig   = bz_locations()->{'localconfig'};
     my $template_cache = bz_locations()->{'template_cache'};
+    my $graphsdir     = bz_locations()->{'graphsdir'};
 
     # We want to set the permissions the same for all localconfig files
     # across all PROJECTs, so we do something special with $localconfig,
@@ -197,7 +198,7 @@ sub FILESYSTEM {
                                   dirs => DIR_CGI_WRITE },
          $webdotdir         => { files => WS_SERVE,
                                   dirs => DIR_CGI_WRITE | DIR_ALSO_WS_SERVE },
-         graphs             => { files => WS_SERVE,
+         $graphsdir         => { files => WS_SERVE,
                                   dirs => DIR_CGI_WRITE | DIR_ALSO_WS_SERVE },
          "$datadir/db"      => { files => CGI_WRITE,
                                   dirs => DIR_CGI_WRITE },
@@ -269,7 +270,7 @@ sub FILESYSTEM {
         # Directories that cgi scripts can write to.
         "$datadir/db"           => DIR_CGI_WRITE,
         $attachdir              => DIR_CGI_WRITE,
-        graphs                  => DIR_CGI_WRITE | DIR_ALSO_WS_SERVE,
+        $graphsdir              => DIR_CGI_WRITE | DIR_ALSO_WS_SERVE,
         $webdotdir              => DIR_CGI_WRITE | DIR_ALSO_WS_SERVE,
         # Directories that contain content served directly by the web server.
         "$skinsdir/custom"      => DIR_WS_SERVE,
@@ -331,6 +332,17 @@ EOT
         "$datadir/.htaccess"         => { perms    => WS_SERVE,
                                           contents => HT_DEFAULT_DENY },
 
+        "$graphsdir/.htaccess" => { perms => WS_SERVE, contents => <<EOT
+# Allow access to .png and .gif files.
+<FilesMatch (\\.gif|\\.png)\$>
+  Allow from all
+</FilesMatch>
+
+# And no directory listings, either.
+Deny from all
+EOT
+        },
+
         "$webdotdir/.htaccess" => { perms => WS_SERVE, contents => <<EOT
 # Restrict access to .dot files to the public webdot server at research.att.com
 # if research.att.com ever changes their IP, or if you use a different
@@ -373,10 +385,11 @@ sub update_filesystem {
     my %files = %{$fs->{create_files}};
 
     my $datadir = bz_locations->{'datadir'};
+    my $graphsdir = bz_locations->{'graphsdir'};
     # If the graphs/ directory doesn't exist, we're upgrading from
     # a version old enough that we need to update the $datadir/mining 
     # format.
-    if (-d "$datadir/mining" && !-d 'graphs') {
+    if (-d "$datadir/mining" && !-d $graphsdir) {
         _update_old_charts($datadir);
     }
 
diff --git a/collectstats.pl b/collectstats.pl
index f5ba2ddab..f8ee89215 100755
--- a/collectstats.pl
+++ b/collectstats.pl
@@ -49,9 +49,12 @@ use Bugzilla::Field;
 # in the regenerate mode).
 $| = 1;
 
+my $datadir = bz_locations()->{'datadir'};
+my $graphsdir = bz_locations()->{'graphsdir'};
+
 # Tidy up after graphing module
 my $cwd = Cwd::getcwd();
-if (chdir("graphs")) {
+if (chdir($graphsdir)) {
     unlink <./*.gif>;
     unlink <./*.png>;
     # chdir("..") doesn't work if graphs is a symlink, see bug 429378
@@ -68,8 +71,6 @@ if ($#ARGV >= 0 && $ARGV[0] eq "--regenerate") {
     $regenerate = 1;
 }
 
-my $datadir = bz_locations()->{'datadir'};
-
 # As we can now customize statuses and resolutions, looking at the current list
 # of legal values only is not enough as some now removed statuses and resolutions
 # may have existed in the past, or have been renamed. We want them all.
diff --git a/reports.cgi b/reports.cgi
index fd84f5330..01bbb50c7 100755
--- a/reports.cgi
+++ b/reports.cgi
@@ -38,31 +38,28 @@ use Bugzilla::Util;
 use Bugzilla::Error;
 use Bugzilla::Status;
 
+use File::Basename;
+use Digest::MD5 qw(md5_hex);
+
 # If we're using bug groups for products, we should apply those restrictions
 # to viewing reports, as well.  Time to check the login in that case.
 my $user = Bugzilla->login();
+my $cgi = Bugzilla->cgi;
+my $template = Bugzilla->template;
+my $vars = {};
 
 if (!Bugzilla->feature('old_charts')) {
     ThrowCodeError('feature_disabled', { feature => 'old_charts' });
 }
 
 my $dir       = bz_locations()->{'datadir'} . "/mining";
-my $graph_url = 'graphs';
-my $graph_dir = bz_locations()->{'libpath'} . '/' .$graph_url;
+my $graph_dir = bz_locations()->{'graphsdir'};
+my $graph_url = basename($graph_dir);
+my $product_name = $cgi->param('product') || '';
 
 Bugzilla->switch_to_shadow_db();
 
-my $cgi = Bugzilla->cgi;
-my $template = Bugzilla->template;
-my $vars = {};
-
-# We only want those products that the user has permissions for.
-my @myproducts;
-push( @myproducts, "-All-");
-# Extract product names from objects and add them to the list.
-push( @myproducts, map { $_->name } @{$user->get_selectable_products} );
-
-if (! defined $cgi->param('product')) {
+if (!$product_name) {
     # Can we do bug charts?
     (-d $dir && -d $graph_dir) 
       || ThrowCodeError('chart_dir_nonexistent',
@@ -80,49 +77,61 @@ if (! defined $cgi->param('product')) {
         push(@datasets, $datasets);
     }
 
+    # We only want those products that the user has permissions for.
+    my @myproducts = ('-All-');
+    # Extract product names from objects and add them to the list.
+    push( @myproducts, map { $_->name } @{$user->get_selectable_products} );
+
     $vars->{'datasets'} = \@datasets;
     $vars->{'products'} = \@myproducts;
 
     print $cgi->header();
-
-    $template->process('reports/old-charts.html.tmpl', $vars)
-      || ThrowTemplateError($template->error());
-    exit;
 }
 else {
-    my $product = $cgi->param('product');
-
     # For security and correctness, validate the value of the "product" form variable.
     # Valid values are those products for which the user has permissions which appear
     # in the "product" drop-down menu on the report generation form.
-    grep($_ eq $product, @myproducts)
-      || ThrowUserError("invalid_product_name", {product => $product});
+    my ($product) = grep { $_->name eq $product_name } @{$user->get_selectable_products};
+    ($product || $product_name eq '-All-')
+      || ThrowUserError('invalid_product_name', {product => $product_name});
 
-    # We've checked that the product exists, and that the user can see it
-    # This means that is OK to detaint
-    trick_taint($product);
+    # Product names can change over time. Their ID cannot; so use the ID
+    # to generate the filename.
+    my $prod_id = $product ? $product->id : 0;
 
-    defined($cgi->param('datasets')) || ThrowUserError('missing_datasets');
+    # Make sure there is something to plot.
+    my @datasets = $cgi->param('datasets');
+    scalar(@datasets) || ThrowUserError('missing_datasets');
 
-    my $datasets = join('', $cgi->param('datasets'));
+    if (grep { $_ !~ /^[A-Za-z0-9:_-]+$/ } @datasets) {
+        ThrowUserError('invalid_datasets', {'datasets' => \@datasets});
+    }
 
-    my $data_file = daily_stats_filename($product);
-    my $image_file = chart_image_name($data_file, $datasets);
-    my $url_image = correct_urlbase() . "$graph_url/$image_file";
+    # Filenames must not be guessable as they can point to products
+    # you are not allowed to see. Also, different projects can have
+    # the same product names.
+    my $key = Bugzilla->localconfig->{'site_wide_secret'};
+    my $project = bz_locations()->{'project'} || '';
+    my $image_file =  join(':', ($key, $project, $prod_id, @datasets));
+    # Wide characters cause md5_hex() to die.
+    if (Bugzilla->params->{'utf8'}) {
+        utf8::encode($image_file) if utf8::is_utf8($image_file);
+    }
+    $image_file = md5_hex($image_file) . '.png';
+    trick_taint($image_file);
 
     if (! -e "$graph_dir/$image_file") {
-        generate_chart("$dir/$data_file", "$graph_dir/$image_file", $product, $datasets);
+        generate_chart($dir, "$graph_dir/$image_file", $product, \@datasets);
     }
 
-    $vars->{'url_image'} = $url_image;
+    $vars->{'url_image'} = "$graph_url/$image_file";
 
     print $cgi->header(-Content_Disposition=>'inline; filename=bugzilla_report.html');
-
-    $template->process('reports/old-charts.html.tmpl', $vars)
-      || ThrowTemplateError($template->error());
-    exit;
 }
 
+$template->process('reports/old-charts.html.tmpl', $vars)
+  || ThrowTemplateError($template->error());
+
 #####################
 #    Subroutines    #
 #####################
@@ -131,9 +140,8 @@ sub get_data {
     my $dir = shift;
 
     my @datasets;
-    my $datafile = daily_stats_filename('-All-');
-    open(DATA, '<', "$dir/$datafile")
-      || ThrowCodeError('chart_file_open_fail', {filename => "$dir/$datafile"});
+    open(DATA, '<', "$dir/-All-")
+      || ThrowCodeError('chart_file_open_fail', {filename => "$dir/-All-"});
 
     while (<DATA>) {
         if (/^# fields?: (.+)\s*$/) {
@@ -145,38 +153,12 @@ sub get_data {
     return @datasets;
 }
 
-sub daily_stats_filename {
-    my ($prodname) = @_;
-    $prodname =~ s/\//-/gs;
-    return $prodname;
-}
-
-sub chart_image_name {
-    my ($data_file, $datasets) = @_;
-
-    # This routine generates a filename from the requested fields. The problem
-    # is that we have to check the safety of doing this. We can't just require
-    # that the fields exist, because what stats were collected could change
-    # over time (eg by changing the resolutions available)
-    # Instead, just require that each field name consists only of letters,
-    # numbers, underscores and hyphens.
-
-    if ($datasets !~ m/^[A-Za-z0-9:_-]+$/) {
-        ThrowUserError('invalid_datasets', {'datasets' => $datasets});
-    }
-
-    # Since we pass the tests, consider it OK
-    trick_taint($datasets);
-
-    # Cache charts by generating a unique filename based on what they
-    # show. Charts should be deleted by collectstats.pl nightly.
-    my $id = join ("_", split (":", $datasets));
-
-    return "${data_file}_${id}.png";
-}
-
 sub generate_chart {
-    my ($data_file, $image_file, $product, $datasets) = @_;
+    my ($dir, $image_file, $product, $datasets) = @_;
+    $product = $product ? $product->name : '-All-';
+    my $data_file = $product;
+    $data_file =~ s/\//-/gs;
+    $data_file = $dir . '/' . $data_file;
 
     if (! open FILE, $data_file) {
         if ($product eq '-All-') {
@@ -187,7 +169,7 @@ sub generate_chart {
 
     my @fields;
     my @labels = qw(DATE);
-    my %datasets = map { $_ => 1 } split /:/, $datasets;
+    my %datasets = map { $_ => 1 } @$datasets;
 
     my %data = ();
     while (<FILE>) {
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index a734435a1..5aa8955fd 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -907,7 +907,7 @@
 
   [% ELSIF error == "invalid_datasets" %]
     [% title = "Invalid Datasets" %]
-    Invalid datasets <em>[% datasets FILTER html %]</em>. Only digits,
+    Invalid datasets <em>[% datasets.join(":") FILTER html %]</em>. Only digits,
     letters and colons are allowed.
 
   [% ELSIF error == "invalid_format" %]
diff --git a/template/en/default/reports/old-charts.html.tmpl b/template/en/default/reports/old-charts.html.tmpl
index ca3ba6c7d..4bdc0cffa 100644
--- a/template/en/default/reports/old-charts.html.tmpl
+++ b/template/en/default/reports/old-charts.html.tmpl
@@ -51,7 +51,7 @@
               [%# We cannot use translated statuses and resolutions from field-descs.none.html
                 # because old charts do not distinguish statuses from resolutions. %]
               [% FOREACH dataset = datasets %]
-                <option value="[% dataset.value FILTER html %]:"
+                <option value="[% dataset.value FILTER html %]"
                   [% " selected=\"selected\"" IF dataset.selected %]>
                   [% dataset.value FILTER html %]</option>
               [% END %]