summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorFrédéric Buclin <LpSolit@gmail.com>2011-08-04 22:08:32 +0200
committerFrédéric Buclin <LpSolit@gmail.com>2011-08-04 22:08:32 +0200
commit5d70d16f37a866852e6a48ec9fefe3664a6a9a55 (patch)
treeb193cb8a52a93619d408869931126777d8c82bb0
parentb9c01561118c42514055b218f81cb82fa76dbb05 (diff)
downloadbugzilla-5d70d16f37a866852e6a48ec9fefe3664a6a9a55.tar.gz
bugzilla-5d70d16f37a866852e6a48ec9fefe3664a6a9a55.tar.xz
Bug 653477: (CVE-2011-2380) [SECURITY] Group names can be guessed when creating or editing a bug
r=mkanat a=LpSolit
-rw-r--r--Bugzilla/Bug.pm94
-rw-r--r--Bugzilla/Group.pm56
-rw-r--r--Bugzilla/Product.pm10
-rw-r--r--Bugzilla/WebService/Constants.pm3
-rwxr-xr-xprocess_bug.cgi10
-rw-r--r--template/en/default/global/user-error.html.tmpl28
6 files changed, 133 insertions, 68 deletions
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index bf4529f6f..1f0034ff7 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -1622,6 +1622,8 @@ sub _check_dup_id {
sub _check_groups {
my ($invocant, $group_names, undef, $params) = @_;
+
+ my $bug_id = blessed($invocant) ? $invocant->id : undef;
my $product = blessed($invocant) ? $invocant->product_obj
: $params->{product};
my %add_groups;
@@ -1640,14 +1642,12 @@ sub _check_groups {
if !ref $group_names;
# First check all the groups they chose to set.
+ my %args = ( product => $product->name, bug_id => $bug_id, action => 'add' );
foreach my $name (@$group_names) {
- my $group = Bugzilla::Group->check(
- { name => $name, product => $product,
- _error => 'group_restriction_not_allowed' });
+ my $group = Bugzilla::Group->check_no_disclose({ %args, name => $name });
if (!$product->group_is_settable($group)) {
- ThrowUserError('group_restriction_not_allowed',
- { name => $name, product => $product });
+ ThrowUserError('group_restriction_not_allowed', { %args, name => $name });
}
$add_groups{$group->id} = $group;
}
@@ -2512,20 +2512,18 @@ sub _set_product {
$self->set_target_milestone($tm_name);
}
}
-
+
if ($product_changed) {
- # Remove groups that can't be set in the new product, or that aren't
- # active.
- #
+ # Remove groups that can't be set in the new product.
# We copy this array because the original array is modified while we're
# working, and that confuses "foreach".
my @current_groups = @{$self->groups_in};
foreach my $group (@current_groups) {
- if (!$group->is_active or !$product->group_is_valid($group)) {
+ if (!$product->group_is_valid($group)) {
$self->remove_group($group);
}
}
-
+
# Make sure the bug is in all the mandatory groups for the new product.
foreach my $group (@{$product->groups_mandatory}) {
$self->add_group($group);
@@ -2757,18 +2755,25 @@ sub modify_keywords {
sub add_group {
my ($self, $group) = @_;
- # Invalid ids are silently ignored. (We can't tell people whether
- # or not a group exists.)
- $group = Bugzilla::Group->check($group) if !blessed $group;
- return if !$group->is_active or !$group->is_bug_group;
+ # If the user enters "FoO" but the DB has "Foo", $group->name would
+ # return "Foo" and thus revealing the existence of the group name.
+ # So we have to store and pass the name as entered by the user to
+ # the error message, if we have it.
+ my $group_name = blessed($group) ? $group->name : $group;
+ my $args = { name => $group_name, product => $self->product,
+ bug_id => $self->id, action => 'add' };
+
+ $group = Bugzilla::Group->check_no_disclose($args) if !blessed $group;
+
+ # If the bug is already in this group, then there is nothing to do.
+ return if $self->in_group($group);
+
# Make sure that bugs in this product can actually be restricted
# to this group by the current user.
$self->product_obj->group_is_settable($group)
- || ThrowUserError('group_invalid_restriction',
- { product => $self->product, group => $group,
- bug => $self });
+ || ThrowUserError('group_restriction_not_allowed', $args);
# OtherControl people can add groups only during a product change,
# and only when the group is not NA for them.
@@ -2777,37 +2782,38 @@ sub add_group {
if (!$self->{_old_product_name}
|| $controls->{othercontrol} == CONTROLMAPNA)
{
- ThrowUserError('group_change_denied',
- { bug => $self, group => $group });
+ ThrowUserError('group_restriction_not_allowed', $args);
}
}
my $current_groups = $self->groups_in;
- if (!grep($group->id == $_->id, @$current_groups)) {
- push(@$current_groups, $group);
- }
+ push(@$current_groups, $group);
}
sub remove_group {
my ($self, $group) = @_;
- $group = Bugzilla::Group->check($group) if !blessed $group;
-
- # First, check if this is a valid group for this product.
- # You can *always* remove a group that is not valid for this product
- # or that is not active, so we don't do any other checks if either of
- # those are the case. (Users might remove inactive groups, and set_product
- # removes groups that aren't valid for this product.)
- #
- # This particularly happens when isbuggroup is no longer 1, and we're
- # moving a bug to a new product.
- if ($group->is_active and $self->product_obj->group_is_valid($group)) {
+
+ # See add_group() for the reason why we store the user input.
+ my $group_name = blessed($group) ? $group->name : $group;
+ my $args = { name => $group_name, product => $self->product,
+ bug_id => $self->id, action => 'remove' };
+
+ $group = Bugzilla::Group->check_no_disclose($args) if !blessed $group;
+
+ # If the bug isn't in this group, then either the name is misspelled,
+ # or the group really doesn't exist. Let the user know about this problem.
+ $self->in_group($group) || ThrowUserError('group_invalid_removal', $args);
+
+ # Check if this is a valid group for this product. You can *always*
+ # remove a group that is not valid for this product (set_product does this).
+ # This particularly happens when we're moving a bug to a new product.
+ # You still have to be a member of an inactive group to remove it.
+ if ($self->product_obj->group_is_valid($group)) {
my $controls = $self->product_obj->group_controls->{$group->id};
- # Nobody can ever remove a Mandatory group.
- if ($controls->{membercontrol} == CONTROLMAPMANDATORY) {
- ThrowUserError('group_invalid_removal',
- { product => $self->product, group => $group,
- bug => $self });
+ # Nobody can ever remove a Mandatory group, unless it became inactive.
+ if ($controls->{membercontrol} == CONTROLMAPMANDATORY && $group->is_active) {
+ ThrowUserError('group_invalid_removal', $args);
}
# OtherControl people can remove groups only during a product change,
@@ -2817,12 +2823,11 @@ sub remove_group {
|| $controls->{othercontrol} == CONTROLMAPMANDATORY
|| $controls->{othercontrol} == CONTROLMAPNA)
{
- ThrowUserError('group_change_denied',
- { bug => $self, group => $group });
+ ThrowUserError('group_invalid_removal', $args);
}
}
}
-
+
my $current_groups = $self->groups_in;
@$current_groups = grep { $_->id != $group->id } @$current_groups;
}
@@ -3529,6 +3534,11 @@ sub groups_in {
return $self->{'groups_in'};
}
+sub in_group {
+ my ($self, $group) = @_;
+ return grep($_->id == $group->id, @{$self->groups_in}) ? 1 : 0;
+}
+
sub user {
my $self = shift;
return $self->{'user'} if exists $self->{'user'};
diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm
index 11a27e2e6..b7532fe09 100644
--- a/Bugzilla/Group.pm
+++ b/Bugzilla/Group.pm
@@ -439,6 +439,21 @@ sub ValidateGroupName {
return $ret;
}
+sub check_no_disclose {
+ my ($class, $params) = @_;
+ my $action = delete $params->{action};
+
+ $action =~ /^(?:add|remove)$/
+ or ThrowCodeError('bad_arg', { argument => $action,
+ function => "${class}::check_no_disclose" });
+
+ $params->{_error} = ($action eq 'add') ? 'group_restriction_not_allowed'
+ : 'group_invalid_removal';
+
+ my $group = $class->check($params);
+ return $group;
+}
+
###############################
### Validators ###
###############################
@@ -538,6 +553,47 @@ Returns: It returns the group id if successful
=over
+=item C<check_no_disclose>
+
+=over
+
+=item B<Description>
+
+Throws an error if the user cannot add or remove this group to/from a given
+bug, but doesn't specify if this is because the group doesn't exist, or the
+user is not allowed to edit this group restriction.
+
+=item B<Params>
+
+This method takes a single hashref as argument, with the following keys:
+
+=over
+
+=item C<name>
+
+C<string> The name of the group to add or remove.
+
+=item C<bug_id>
+
+C<integer> The ID of the bug to which the group change applies.
+
+=item C<product>
+
+C<string> The name of the product the bug belongs to.
+
+=item C<action>
+
+C<string> Must be either C<add> or C<remove>, depending on whether the group
+must be added or removed from the bug. Any other value will generate an error.
+
+=back
+
+=item C<Returns>
+
+A C<Bugzilla::Group> object on success, else an error is thrown.
+
+=back
+
=item C<check_members_are_visible>
Throws an error if this group is not visible (according to
diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm
index b9443e9e6..85524ac47 100644
--- a/Bugzilla/Product.pm
+++ b/Bugzilla/Product.pm
@@ -680,10 +680,12 @@ sub groups_mandatory {
# if this group can be validly set by the currently-logged-in user.
sub group_is_settable {
my ($self, $group) = @_;
- my $group_id = blessed($group) ? $group->id : $group;
- my $is_mandatory = grep { $group_id == $_->id }
+
+ return 0 unless ($group->is_active && $group->is_bug_group);
+
+ my $is_mandatory = grep { $group->id == $_->id }
@{ $self->groups_mandatory };
- my $is_available = grep { $group_id == $_->id }
+ my $is_available = grep { $group->id == $_->id }
@{ $self->groups_available };
return ($is_mandatory or $is_available) ? 1 : 0;
}
@@ -948,7 +950,7 @@ a bug. (In fact, the user I<must> set the Mandatory group on the bug.)
=over
-=item C<$group> - Either a numeric group id or a L<Bugzilla::Group> object.
+=item C<$group> - A L<Bugzilla::Group> object.
=back
diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm
index cb518b0bd..123b14d68 100644
--- a/Bugzilla/WebService/Constants.pm
+++ b/Bugzilla/WebService/Constants.pm
@@ -109,8 +109,7 @@ use constant WS_ERROR_CODE => {
dupe_loop_detected => 118,
dupe_id_required => 119,
# Bug-related group errors
- group_change_denied => 120,
- group_invalid_restriction => 120,
+ group_invalid_removal => 120,
group_restriction_not_allowed => 120,
# Status/Resolution errors
missing_resolution => 121,
diff --git a/process_bug.cgi b/process_bug.cgi
index 0348424fa..acb359f63 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -345,7 +345,17 @@ foreach my $field (@custom_fields) {
}
}
+# We are going to alter the list of removed groups, so we keep a copy here.
+my @unchecked_groups = @$removed_groups;
foreach my $b (@bug_objects) {
+ # Don't blindly ask to remove unchecked groups available in the UI.
+ # A group can be already unchecked, and the user didn't try to remove it.
+ # In this case, we don't want remove_group() to complain.
+ my @remove_groups;
+ foreach my $g (@{$b->groups_in}) {
+ push(@remove_groups, $g->name) if grep { $_ eq $g->name } @unchecked_groups;
+ }
+ local $set_all_fields{groups}->{remove} = \@remove_groups;
$b->set_all(\%set_all_fields);
}
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 52ac64ddd..3e1b8748e 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -735,12 +735,6 @@
in the database which refer to it. All references to this group must
be removed before you can remove it.
- [% ELSIF error == "group_change_denied" %]
- [% title = "Cannot Add/Remove That Group" %]
- You tried to add or remove the '[% group.name FILTER html %]' group
- from [% terms.bug %] [%+ bug.id FILTER html %], but you do not
- have permissions to do so.
-
[% ELSIF error == "group_exists" %]
[% title = "The group already exists" %]
The group [% name FILTER html %] already exists.
@@ -761,23 +755,17 @@
[% ELSIF error == "group_invalid_removal" %]
- You tried to remove [% terms.bug %] [%+ bug.id FILTER html %]
- from the '[% group.name FILTER html %]' group, but [% terms.bugs %]
- in the '[% product FILTER html %]' product can not be removed from that
- group.
-
- [% ELSIF error == "group_invalid_restriction" %]
- You tried to restrict [% terms.bug %] [%+ bug.id FILTER html %] to
- to the '[% group.name FILTER html %]' group, but [% terms.bugs %] in the
- '[% product FILTER html %]' product can not be restricted to
- that group.
+ You tried to remove [% terms.bug %] [%+ bug_id FILTER html %]
+ from the '[% name FILTER html %]' group, but either this group does not exist,
+ or you are not allowed to remove [% terms.bugs %] from this group in the
+ '[% product FILTER html %]' product.
[% ELSIF error == "group_restriction_not_allowed" %]
[% title = "Group Restriction Not Allowed" %]
- You tried to restrict [% terms.abug %] to the "[% name FILTER html %]"
- group, but either this group does not exist, or you are not allowed
- to restrict [% terms.bugs %] to this group in the "[% product.name FILTER html %]"
- product.
+ You tried to restrict [% bug_id ? "$terms.bug $bug_id" : terms.abug FILTER html %]
+ to the '[% name FILTER html %]' group, but either this group does not exist,
+ or you are not allowed to restrict [% terms.bugs %] to this group in the
+ '[% product FILTER html %]' product.
[% ELSIF error == "group_not_specified" %]
[% title = "Group not specified" %]