Bug 287109: [SECURITY] Names of private products/components can be exposed on certain CGIs

Patch By Frederic Buclin <LpSolit@gmail.com> r=myk, r=joel, a=justdave

6 files changed, 86 insertions, 41 deletions

diff --git a/enter_bug.cgi b/enter_bug.cgi
index 924977a3e..f3437b7ed 100755
--- a/enter_bug.cgi
+++ b/enter_bug.cgi
@@ -292,10 +292,7 @@ if ($cloned_bug_id) {
 
 # We need to check and make sure
 # that the user has permission to enter a bug against this product.
-if(!CanEnterProduct($product))
-{
-    ThrowUserError("entry_access_denied", { product => $product});         
-}
+CanEnterProductOrWarn($product);
 
 GetVersionTable();
 
diff --git a/globals.pl b/globals.pl
index d0e819f02..009f93ee9 100644
--- a/globals.pl
+++ b/globals.pl
@@ -436,12 +436,16 @@ sub IsInClassification {
     }
 }
 
-#
-# This function determines if a user can enter bugs in the named
-# product.
+# This function determines whether or not a user can enter
+# bugs into the named product.
 sub CanEnterProduct {
-    my ($productname) = @_;
+    my ($productname, $verbose) = @_;
     my $dbh = Bugzilla->dbh;
+
+    return unless defined($productname);
+    trick_taint($productname);
+
+    # First check whether or not the user has access to that product.
     my $query = "SELECT group_id IS NULL " .
                 "FROM products " .
                 "LEFT JOIN group_control_map " .
@@ -451,13 +455,53 @@ sub CanEnterProduct {
         $query .= "AND group_id NOT IN(" . 
                    join(',', values(%{Bugzilla->user->groups})) . ") ";
     }
-    $query .= "WHERE products.name = " . SqlQuote($productname) . " " .
+    $query .= "WHERE products.name = ? " .
               $dbh->sql_limit(1);
-    PushGlobalSQLState();
-    SendSQL($query);
-    my ($ret) = FetchSQLData();
-    PopGlobalSQLState();
-    return ($ret);
+
+    my $has_access = $dbh->selectrow_array($query, undef, $productname);
+    if (!$has_access) {
+        # Do we require the exact reason why we cannot enter
+        # bugs into that product? Returning -1 explicitely
+        # means the user has no access to the product or the
+        # product does not exist.
+        return (defined($verbose)) ? -1 : 0;
+    }
+
+    # Check if the product is open for new bugs and has
+    # at least one component.
+    my $allow_new_bugs =
+        $dbh->selectrow_array("SELECT CASE WHEN disallownew = 0 THEN 1 ELSE 0 END
+                               FROM products INNER JOIN components
+                               ON components.product_id = products.id
+                               WHERE products.name = ? " .
+                               $dbh->sql_limit(1),
+                               undef, $productname);
+
+    # Return 1 if the user can enter bugs into that product;
+    # return 0 if the product is closed for new bug entry;
+    # return undef if the product has no component.
+    return $allow_new_bugs;
+}
+
+# Call CanEnterProduct() and display an error message
+# if the user cannot enter bugs into that product.
+sub CanEnterProductOrWarn {
+    my ($product) = @_;
+
+    if (!defined($product)) {
+        ThrowUserError("no_products");
+    }
+    my $status = CanEnterProduct($product, 1);
+    trick_taint($product);
+
+    if (!defined($status)) {
+        ThrowUserError("no_components", { product => $product});
+    } elsif (!$status) {
+        ThrowUserError("product_disabled", { product => $product});
+    } elsif ($status < 0) {
+        ThrowUserError("entry_access_denied", { product => $product});
+    }
+    return $status;
 }
 
 sub GetEnterableProducts {
diff --git a/post_bug.cgi b/post_bug.cgi
index 9a4860409..b9d63b3fe 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -79,11 +79,10 @@ $template->process($format->{'template'}, $vars, \$comment)
 ValidateComment($comment);
 
 # Check that the product exists and that the user
-# is allowed to submit bugs in this product.
+# is allowed to enter bugs into this product.
 my $product = $cgi->param('product');
-if (!CanEnterProduct($product)) {
-    ThrowUserError("entry_access_denied", {product => $product});
-}
+CanEnterProductOrWarn($product);
+
 my $product_id = get_product_id($product);
 
 # Set cookies
diff --git a/process_bug.cgi b/process_bug.cgi
index 451613e29..c000e3a4a 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -60,7 +60,8 @@ use Bugzilla::FlagType;
 
 # Shut up misguided -w warnings about "used only once":
 
-use vars qw(%versions
+use vars qw(@legal_product
+          %versions
           %components
           %legal_opsys
           %legal_platform
@@ -268,9 +269,26 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
         $vars->{'privs'} = $PrivilegesRequired;
         ThrowUserError("illegal_change", $vars);
     }
- 
-    CheckFormField($cgi, 'product', \@::legal_product);
+
     my $prod = $cgi->param('product');
+    trick_taint($prod);
+
+    # If at least one bug does not belong to the product we are
+    # moving to, we have to check whether or not the user is
+    # allowed to enter bugs into that product.
+    # Note that this check must be done early to avoid the leakage
+    # of component, version and target milestone names.
+    my $check_can_enter =
+        $dbh->selectrow_array("SELECT 1 FROM bugs
+                               INNER JOIN products
+                               ON bugs.product_id = products.id
+                               WHERE products.name != ?
+                               AND bugs.bug_id IN
+                               (" . join(',', @idlist) . ") " .
+                               $dbh->sql_limit(1),
+                               undef, $prod);
+
+    if ($check_can_enter) { CanEnterProductOrWarn($prod) }
 
     # note that when this script is called from buglist.cgi (rather
     # than show_bug.cgi), it's possible that the product will be changed
@@ -755,6 +773,7 @@ if ($cgi->param('component') ne $cgi->param('dontchange')) {
                                {name => $cgi->param('component'),
                                 product => $cgi->param('product')});
     
+    $cgi->param('component_id', $comp_id);
     DoComma();
     $::query .= "component_id = $comp_id";
 }
@@ -1164,17 +1183,6 @@ foreach my $id (@idlist) {
             "group_control_map AS oldcontrolmap READ",
             "group_control_map AS newcontrolmap READ",
             "group_control_map READ", "email_setting READ");
-    # Fun hack.  @::log_columns only contains the component_id,
-    # not the name (since bug 43600 got fixed).  So, we need to have
-    # this id ready for the loop below, otherwise anybody can
-    # change the component of a bug (we checked product above).
-    # http://bugzilla.mozilla.org/show_bug.cgi?id=180545
-    my $product_id = get_product_id($cgi->param('product'));
-    
-    if ($cgi->param('component') ne $cgi->param('dontchange')) {
-        $cgi->param('component_id',
-                    get_component_id($product_id, $cgi->param('component')));
-    }
 
     # It may sound crazy to set %formhash for each bug as $cgi->param()
     # will not change, but %formhash is modified below and we prefer
@@ -1258,12 +1266,6 @@ foreach my $id (@idlist) {
                       { product => $oldhash{'product'} });
     }
 
-    if ($cgi->param('product') ne $cgi->param('dontchange')
-        && $cgi->param('product') ne $oldhash{'product'}
-        && !CanEnterProduct($cgi->param('product'))) {
-        ThrowUserError("entry_access_denied",
-                       { product => $cgi->param('product') });
-    }
     if ($requiremilestone) {
         # musthavemilestoneonaccept applies only if at least two
         # target milestones are defined for the current product.
diff --git a/reports.cgi b/reports.cgi
index a3e2c740e..c5314b33e 100755
--- a/reports.cgi
+++ b/reports.cgi
@@ -85,9 +85,7 @@ if (! defined $cgi->param('product')) {
 
     # We don't want people to be able to view
     # reports for products they don't have permissions for...
-    if (($product ne '-All-') && (!CanEnterProduct($product))) {
-        ThrowUserError("report_access_denied");
-    }
+    if ($product ne '-All-') { CanEnterProductOrWarn($product) }
           
     # We've checked that the product exists, and that the user can see it
     # This means that is OK to detaint
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index b7cefa9a3..f2ccb1497 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -920,6 +920,11 @@
     Patches cannot be more than [% Param('maxpatchsize') %] KB in size.
     Try breaking your patch into several pieces.
 
+  [% ELSIF error == "product_disabled" %]
+    [% title = BLOCK %]Product closed for [% terms.Bugs %] Entry[% END %]
+    Sorry, entering [% terms.bugs %] into
+    product <em>[% product FILTER html %]</em> has been disabled.
+
   [% ELSIF error == "product_edit_denied" %]
     [% title = "Product Edit Access Denied" %]
     You are not permitted to edit [% terms.bugs %] in product