Bug 653477: (CVE-2011-2380) [SECURITY] Group names can be guessed when creating or editing a bug

r=mkanat a=LpSolit

4 files changed, 115 insertions, 48 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,