Commit e2252835 authored by mkanat%kerio.com's avatar mkanat%kerio.com

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
parent 8f2bc1b0
...@@ -292,10 +292,7 @@ if ($cloned_bug_id) { ...@@ -292,10 +292,7 @@ if ($cloned_bug_id) {
# We need to check and make sure # We need to check and make sure
# that the user has permission to enter a bug against this product. # that the user has permission to enter a bug against this product.
if(!CanEnterProduct($product)) CanEnterProductOrWarn($product);
{
ThrowUserError("entry_access_denied", { product => $product});
}
GetVersionTable(); GetVersionTable();
......
...@@ -436,12 +436,16 @@ sub IsInClassification { ...@@ -436,12 +436,16 @@ sub IsInClassification {
} }
} }
# # This function determines whether or not a user can enter
# This function determines if a user can enter bugs in the named # bugs into the named product.
# product.
sub CanEnterProduct { sub CanEnterProduct {
my ($productname) = @_; my ($productname, $verbose) = @_;
my $dbh = Bugzilla->dbh; 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 " . my $query = "SELECT group_id IS NULL " .
"FROM products " . "FROM products " .
"LEFT JOIN group_control_map " . "LEFT JOIN group_control_map " .
...@@ -451,13 +455,53 @@ sub CanEnterProduct { ...@@ -451,13 +455,53 @@ sub CanEnterProduct {
$query .= "AND group_id NOT IN(" . $query .= "AND group_id NOT IN(" .
join(',', values(%{Bugzilla->user->groups})) . ") "; join(',', values(%{Bugzilla->user->groups})) . ") ";
} }
$query .= "WHERE products.name = " . SqlQuote($productname) . " " . $query .= "WHERE products.name = ? " .
$dbh->sql_limit(1); $dbh->sql_limit(1);
PushGlobalSQLState();
SendSQL($query); my $has_access = $dbh->selectrow_array($query, undef, $productname);
my ($ret) = FetchSQLData(); if (!$has_access) {
PopGlobalSQLState(); # Do we require the exact reason why we cannot enter
return ($ret); # 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 { sub GetEnterableProducts {
......
...@@ -79,11 +79,10 @@ $template->process($format->{'template'}, $vars, \$comment) ...@@ -79,11 +79,10 @@ $template->process($format->{'template'}, $vars, \$comment)
ValidateComment($comment); ValidateComment($comment);
# Check that the product exists and that the user # 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'); my $product = $cgi->param('product');
if (!CanEnterProduct($product)) { CanEnterProductOrWarn($product);
ThrowUserError("entry_access_denied", {product => $product});
}
my $product_id = get_product_id($product); my $product_id = get_product_id($product);
# Set cookies # Set cookies
......
...@@ -60,7 +60,8 @@ use Bugzilla::FlagType; ...@@ -60,7 +60,8 @@ use Bugzilla::FlagType;
# Shut up misguided -w warnings about "used only once": # Shut up misguided -w warnings about "used only once":
use vars qw(%versions use vars qw(@legal_product
%versions
%components %components
%legal_opsys %legal_opsys
%legal_platform %legal_platform
...@@ -268,9 +269,26 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct) ...@@ -268,9 +269,26 @@ if (((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
$vars->{'privs'} = $PrivilegesRequired; $vars->{'privs'} = $PrivilegesRequired;
ThrowUserError("illegal_change", $vars); ThrowUserError("illegal_change", $vars);
} }
CheckFormField($cgi, 'product', \@::legal_product);
my $prod = $cgi->param('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 # note that when this script is called from buglist.cgi (rather
# than show_bug.cgi), it's possible that the product will be changed # 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')) { ...@@ -755,6 +773,7 @@ if ($cgi->param('component') ne $cgi->param('dontchange')) {
{name => $cgi->param('component'), {name => $cgi->param('component'),
product => $cgi->param('product')}); product => $cgi->param('product')});
$cgi->param('component_id', $comp_id);
DoComma(); DoComma();
$::query .= "component_id = $comp_id"; $::query .= "component_id = $comp_id";
} }
...@@ -1164,17 +1183,6 @@ foreach my $id (@idlist) { ...@@ -1164,17 +1183,6 @@ foreach my $id (@idlist) {
"group_control_map AS oldcontrolmap READ", "group_control_map AS oldcontrolmap READ",
"group_control_map AS newcontrolmap READ", "group_control_map AS newcontrolmap READ",
"group_control_map READ", "email_setting 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() # It may sound crazy to set %formhash for each bug as $cgi->param()
# will not change, but %formhash is modified below and we prefer # will not change, but %formhash is modified below and we prefer
...@@ -1258,12 +1266,6 @@ foreach my $id (@idlist) { ...@@ -1258,12 +1266,6 @@ foreach my $id (@idlist) {
{ product => $oldhash{'product'} }); { 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) { if ($requiremilestone) {
# musthavemilestoneonaccept applies only if at least two # musthavemilestoneonaccept applies only if at least two
# target milestones are defined for the current product. # target milestones are defined for the current product.
......
...@@ -85,9 +85,7 @@ if (! defined $cgi->param('product')) { ...@@ -85,9 +85,7 @@ if (! defined $cgi->param('product')) {
# We don't want people to be able to view # We don't want people to be able to view
# reports for products they don't have permissions for... # reports for products they don't have permissions for...
if (($product ne '-All-') && (!CanEnterProduct($product))) { if ($product ne '-All-') { CanEnterProductOrWarn($product) }
ThrowUserError("report_access_denied");
}
# We've checked that the product exists, and that the user can see it # We've checked that the product exists, and that the user can see it
# This means that is OK to detaint # This means that is OK to detaint
......
...@@ -920,6 +920,11 @@ ...@@ -920,6 +920,11 @@
Patches cannot be more than [% Param('maxpatchsize') %] KB in size. Patches cannot be more than [% Param('maxpatchsize') %] KB in size.
Try breaking your patch into several pieces. 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" %] [% ELSIF error == "product_edit_denied" %]
[% title = "Product Edit Access Denied" %] [% title = "Product Edit Access Denied" %]
You are not permitted to edit [% terms.bugs %] in product You are not permitted to edit [% terms.bugs %] in product
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment