From c793bb323f1c58c1fe3e82b09068e88c380f5c21 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Wed, 3 Nov 2010 00:06:15 +0100 Subject: Bug 419014: (CVE-2010-3764) [SECURITY] Old charts are not project specific, and product names are viewable in graphs/ r=wurblzap a=LpSolit --- reports.cgi | 122 ++++++++++++++++++++++++++---------------------------------- 1 file changed, 52 insertions(+), 70 deletions(-) (limited to 'reports.cgi') 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 () { 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 () { -- cgit v1.2.3-24-g4f1b