diff options
author | Frédéric Buclin <LpSolit@gmail.com> | 2010-11-03 00:12:13 +0100 |
---|---|---|
committer | Frédéric Buclin <LpSolit@gmail.com> | 2010-11-03 00:12:13 +0100 |
commit | 25ee1bf1a3d7ab82d127ec7fa71e7b9fef1f1cd9 (patch) | |
tree | bb73f2baed5e43dab587e20c0b1904f841c22d0d | |
parent | Bug 608645: Release Notes for Bugzilla 3.4.9 (diff) | |
download | bugzilla-25ee1bf1a3d7ab82d127ec7fa71e7b9fef1f1cd9.tar.gz bugzilla-25ee1bf1a3d7ab82d127ec7fa71e7b9fef1f1cd9.tar.bz2 bugzilla-25ee1bf1a3d7ab82d127ec7fa71e7b9fef1f1cd9.zip |
Bug 419014: (CVE-2010-3764) [SECURITY] Old charts are not project specific, and product names are viewable in graphs/
r=wurblzap a=LpSolit
-rw-r--r-- | Bugzilla/Constants.pm | 1 | ||||
-rw-r--r-- | Bugzilla/Install/Filesystem.pm | 17 | ||||
-rwxr-xr-x | collectstats.pl | 7 | ||||
-rwxr-xr-x | reports.cgi | 123 | ||||
-rw-r--r-- | template/en/default/global/user-error.html.tmpl | 2 | ||||
-rw-r--r-- | template/en/default/reports/old-charts.html.tmpl | 2 |
6 files changed, 73 insertions, 79 deletions
diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index de8f943ab..c45225740 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -515,6 +515,7 @@ sub bz_locations { 'datadir' => "$libpath/$datadir", 'attachdir' => "$libpath/$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 b8f07e7a3..38927ad2f 100644 --- a/Bugzilla/Install/Filesystem.pm +++ b/Bugzilla/Install/Filesystem.pm @@ -72,6 +72,7 @@ sub FILESYSTEM { my $extlib = bz_locations()->{'ext_libpath'}; my $skinsdir = bz_locations()->{'skinsdir'}; my $localconfig = bz_locations()->{'localconfig'}; + my $graphsdir = bz_locations()->{'graphsdir'}; my $ws_group = Bugzilla->localconfig->{'webservergroup'}; @@ -158,7 +159,7 @@ sub FILESYSTEM { dirs => $ws_dir_writeable }, $webdotdir => { files => $ws_writeable, dirs => $ws_dir_writeable }, - graphs => { files => $ws_writeable, + $graphsdir => { files => $ws_writeable, dirs => $ws_dir_writeable }, # Readable directories @@ -210,7 +211,7 @@ sub FILESYSTEM { "$datadir/duplicates" => $ws_dir_readable, $attachdir => $ws_dir_writeable, $extensionsdir => $ws_dir_readable, - graphs => $ws_dir_writeable, + $graphsdir => $ws_dir_writeable, $webdotdir => $ws_dir_writeable, "$skinsdir/custom" => $ws_dir_readable, "$skinsdir/contrib" => $ws_dir_readable, @@ -311,8 +312,17 @@ EOT # in a subdirectory. deny from all EOT + }, + "$graphsdir/.htaccess" => { perms => $ws_readable, 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 }, ); @@ -338,10 +348,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 e550c1613..9be3dc391 100755 --- a/collectstats.pl +++ b/collectstats.pl @@ -52,9 +52,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 @@ -71,8 +74,6 @@ if ($#ARGV >= 0 && $ARGV[0] eq "--regenerate") { $regenerate = 1; } -my $datadir = bz_locations()->{'datadir'}; - my @myproducts = map {$_->name} Bugzilla::Product->get_all; unshift(@myproducts, "-All-"); diff --git a/reports.cgi b/reports.cgi index 6eb4496cc..6a630fabb 100755 --- a/reports.cgi +++ b/reports.cgi @@ -45,32 +45,29 @@ use Bugzilla::Util; use Bugzilla::Error; use Bugzilla::Status; +use File::Basename; +use Digest::MD5 qw(md5_hex); + eval "use GD"; $@ && ThrowCodeError("gd_not_installed"); eval "use Chart::Lines"; $@ && ThrowCodeError("chart_lines_not_installed"); -my $dir = bz_locations()->{'datadir'} . "/mining"; -my $graph_url = 'graphs'; -my $graph_dir = bz_locations()->{'libpath'} . '/' .$graph_url; - # 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(); - -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} ); +my $dir = bz_locations()->{'datadir'} . "/mining"; +my $graph_dir = bz_locations()->{'graphsdir'}; +my $graph_url = basename($graph_dir); +my $product_name = $cgi->param('product') || ''; -if (! defined $cgi->param('product')) { +Bugzilla->switch_to_shadow_db(); + +if (!$product_name) { # Can we do bug charts? (-d $dir && -d $graph_dir) || ThrowCodeError('chart_dir_nonexistent', @@ -88,51 +85,62 @@ 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}); + } + # 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); + } my $type = chart_image_type(); - my $data_file = daily_stats_filename($product); - my $image_file = chart_image_name($data_file, $type, $datasets); - my $url_image = correct_urlbase() . "$graph_url/$image_file"; + $image_file = md5_hex($image_file) . ".$type"; + trick_taint($image_file); if (! -e "$graph_dir/$image_file") { - generate_chart("$dir/$data_file", "$graph_dir/$image_file", $type, - $product, $datasets); + generate_chart($dir, "$graph_dir/$image_file", $type, $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 # ##################### @@ -141,9 +149,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*$/) { @@ -155,12 +162,6 @@ sub get_data { return @datasets; } -sub daily_stats_filename { - my ($prodname) = @_; - $prodname =~ s/\//-/gs; - return $prodname; -} - sub chart_image_type { # what chart type should we be generating? my $testimg = Chart::Lines->new(2,2); @@ -170,32 +171,12 @@ sub chart_image_type { return $type; } -sub chart_image_name { - my ($data_file, $type, $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}.$type"; -} - sub generate_chart { - my ($data_file, $image_file, $type, $product, $datasets) = @_; + my ($dir, $image_file, $type, $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-') { @@ -206,7 +187,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 beea23914..67d0d9acf 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -887,7 +887,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 %] |