Skip to content

bugzilla
bugzilla
Commit 5d70d16f authored by Frédéric Buclin's avatar Frédéric Buclin
Browse files
Options

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

r=mkanat a=LpSolit
parent b9c01561
Show whitespace changes
Inline Side-by-side
Showing with 128 additions and 63 deletions
Bugzilla/Bug.pm
View file @ 5d70d16f
......@@ -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;
}
......@@ -2514,14 +2514,12 @@ sub _set_product {
}
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);
}
}
......@@ -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);
}
}
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,8 +2823,7 @@ sub remove_group {
|| $controls->{othercontrol} == CONTROLMAPMANDATORY
|| $controls->{othercontrol} == CONTROLMAPNA)
{
ThrowUserError('group_change_denied',
{ bug => $self, group => $group });
ThrowUserError('group_invalid_removal', $args);
}
}
}
......@@ -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'};
......
Bugzilla/Group.pm
View file @ 5d70d16f
......@@ -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
......
Bugzilla/Product.pm
View file @ 5d70d16f
......@@ -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
......
Bugzilla/WebService/Constants.pm
View file @ 5d70d16f
......@@ -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,
......
process_bug.cgi
View file @ 5d70d16f
......@@ -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);
}
......
template/en/default/global/user-error.html.tmpl
View file @ 5d70d16f
......@@ -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" %]
......
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