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

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

Symptoms are a bit different in 3.4/3.6 and in 4.0/4.2, but it's one same problem:

3.4/3.6:
=======
When entering a new bug, you can pass bit-X=1 to list of parameters, where X is a group ID, and if the group exists but is not used for bugs, then you get:

"Attempted to add bug to the 'foo' group, which is not used for bugs."

If the group ID exists but is either a system group or a user group used for bugs, then you get no error. So only the name of user groups which aren't used for bugs can be disclosed.

For existing bugs, there is no name disclosed, because the error message displays the group ID instead.


4.0/4.2:
=======
When editing an existing bug, you can pass groups=foo to the list of parameters, where foo is the group name, and if the group doesn't exist, process_bug.cgi tells you so:

"There is no group named 'foo'."

https://bugzilla.mozilla.org/process_bug.cgi?id=653341&groups=I_love_LpSolit


If the group exists, you get:

"You tried to restrict bug X to to the 'foo' group, but bugs in the 'TestProduct' product can not be restricted to that group." (nit: note also the typo "to to")

https://bugzilla.mozilla.org/process_bug.cgi?id=653341&groups=bugzilla-approvers


When creating a new bug, illegal groups are silently ignored (bug 653341).

Comment 1

[Max Kanat-Alexander]

We could just change the default error message for Group->check by overriding check() in Bugzilla::Group. I think we want to be pretty secretive about this everywhere but the Groups control panel.

Comment 2

[Frédéric Buclin]

I will try to fix this bug before we release 4.0.2 and before we branch for 4.2, else it will be a pain to track as it will conflict with bug 658887. I prefer to have something which doesn't leak before starting to play with it in bug 658887.

Comment 3

[Frédéric Buclin]

Attachment: patch for 4.2, v1

With this patch applied, Bugzilla no longer leaks group names when it shouldn't. This means:

- If the bug is already restricted to group A, and a user tries to restrict the bug to group A again, add_group() does nothing as this is technically not a change. This behavior is consistent with how other fields behave: if you don't change a field value, then there is no reason to complain.

- If the bug is not yet restricted to group A, then either group A doesn't exist and we complain, or group A exists but the user is not allowed to restrict the bug to this group and we complain we *exactly* the same error message, i.e. "either the group doesn't exist, or you are not allowed to ...", or group A exists and the user is allowed to restrict the bug to this group and add_group() does it. This reasoning applies for both new bugs and existing bugs.

- If the bug is not restricted to group B and the user tries to remove the bug from group B, then remove_group() does nothing as this is technically not a change. This way, we don't leak if the group doesn't exist, or exists but the bug is not restricted to this group.

- If the bug is restricted to group B and the user tries to remove the bug from this group, then either group B cannot be removed by this user and remove_group() complains about it, or the user can remove this group and remove_group() does it.


About moving a bug into another product, _set_product() no longer removes inactive groups if the group exists in both products, because if products A and B both have group C as an inactive group (simply because you don't want to add new bugs to this group, but want to keep those already restricted to this group as is), then you can still move bugs between both products without disclosing them publicly. I also fixed the logic so that only a member of group C can remove the bug from this group, even if this inactive group C is mandatory (the old code also let non-members remove bugs from inactive groups, which could be a problem for archived bugs in an inactive group, if the non-member was CC'ed, for instance). Else this would mean that inactive groups could be removed even by non-members, which would make them less secure, and admins probably do not expect this behavior (because they don't read the source code!).

For mass-change into a new product, the behavior is the same as when moving a single bug.


The patch has a small bitrot in Bugzilla/WebService/Constants.pm for Bugzilla 4.0.2 and so doesn't apply there. I will upload backports once this one gets r+.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 549383 [details] [diff] [review]
patch for 4.2, v1

Review of attachment 549383 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good and works fine with my testing. Another unrelated issue I found that I can file a separate bug for is that when verifying a product change, if you do not have one or more items selected for component/version/milestone, and then hit submit, the form reloads but any group change verification is no longer displayed on the second try. This due to the check for !$verified on line 2470 in Bugzilla/Bug.pm only working for the initial product verification screen load. I feel if the user still needs to select component/version/milestone, they should still see the groups list as well. Anyway I will file that separately.

r=dkl

Comment 5

[Frédéric Buclin]

Attachment: patch for 4.0, v1

There is no code change compared to 4.2 (use interdiff to check that). The reason the patch for 4.2 didn't apply on the 4.0 branch is because a comment has been edited in 4.2 only, causing the conflict.

Comment 6

[Frédéric Buclin]

Attachment: patch for 4.2, v1.1

I found a small error while testing my patch for 4.0, and the bug also exists in my patch for 4.2: at two places, I passed the product object to the error message instead of passing its name only. No other changes.

Comment 7

[Frédéric Buclin]

Attachment: patch for 4.0, v1.1

Same patch as for 4.2, but with the same bitrot fix as before.

Comment 8

[Frédéric Buclin]

Attachment: patch for 3.4 and 3.6, v1

The same patch applies to both 3.4 and 3.6. The fix on these branches is much smaller as in Bugzilla 3.x, we only mentioned the group ID in error messages instead of the group name, except in this single place, which this patch fixes. This is the single place where we call the 'inactive_group' error message, so this change is safe.

Comment 9

[Daniel Veditz [:dveditz]]

Use CVE-2011-2380 for this bug

Comment 10

[Frédéric Buclin]

We disclosed group names for the first time in 2.23.3, due to bug 351332.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 549913 [details] [diff] [review]
patch for 4.2, v1.1

Review of attachment 549913 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry missed the earlier problem with my previous review. This new version looks good and passes my testing. r=dkl

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 549916 [details] [diff] [review]
patch for 4.0, v1.1

Review of attachment 549916 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and passes my testing. r=dkl

Comment 13

[Max Kanat-Alexander]

Those backport patches are *way* too much change to some *extremely* critical security code to accept on any branch.

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> Those backport patches are *way* too much change to some *extremely*
> critical security code to accept on any branch.

Well, feel free to review them. But I'm pretty confident with them. And the backport for 3.x is ultra-safe. I simply reword the error message.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 549913 [details] [diff] [review]
patch for 4.2, v1.1

Review of attachment 549913 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Bug.pm
@@ +2761,5 @@
> +    # 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;

Change that to blessed($group) to make the precedence clearer.

@@ +2766,5 @@
> +
> +    if (!blessed $group) {
> +        $group = Bugzilla::Group->check({ name => $group, bug_id => $self->id,
> +                                          product => $self->product,
> +                                         _error => 'group_restriction_not_allowed' });

I really would like a Bugzilla::Group->check_carefully or something like that instead of re-writing this everywhere. The _error parameter starts with an underscore because it's only supposed to be used by people who are overriding check() in a subclass.

@@ +2771,5 @@
> +    }
> +
> +    my $current_groups = $self->groups_in;
> +    # If the bug is already in this group, then there is nothing to do.
> +    return if grep { $_->id == $group->id } @$current_groups;

Wow, I'm surprised we don't have a $bug->in_group. Maybe you should add one for this?

@@ +2802,5 @@
> +
> +    if (!blessed $group) {
> +        $group = new Bugzilla::Group({ name => $group });
> +        # If the group doesn't exist, then the bug is not in this group.
> +        return unless $group;

I would really rather not be silent here, for the sake of the Bug.update webservice. There are going to be a lot of programs that depend on solid feedback from the WebService, and this will hide bugs and confuse API consumers if the removal is silent. It isn't a big security issue like add_group being silent, but it will still be very frustrating to downstream consumers.

@@ +2807,5 @@
> +    }
> +
> +    my $current_groups = $self->groups_in;
> +    # If the bug isn't in this group, then there is nothing to do.
> +    return unless grep { $_->id == $group->id } @$current_groups;

Since we do this twice, we really should have $bug->in_group.

@@ +3372,2 @@
>      return '' if $self->{error};
> +    $self->{product} ||= $self->product_obj->name;

This is a lot more work than just grabbing the name from the database; could we do this change in another bug? I've wanted to do it for a long time and it probably actually will *help* performance, but I'm concerned about making this change hidden here behind the security filter.

Also, you don't need to store it in $self->{product}, just return $self->product_obj->name.

::: Bugzilla/Product.pm
@@ +667,4 @@
>          "SELECT group_id 
>             FROM group_control_map
>                  INNER JOIN groups ON group_control_map.group_id = groups.id
> +          WHERE product_id = ? AND isactive = 1 AND isbuggroup = 1

Why add isbuggroup there? A group could not have gotten into this table without isbuggroup being true.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> I would really rather not be silent here, for the sake of the Bug.update
> webservice.

I really disagree here. That's the whole point of my code. Don't complain about a change which is not a change (as the bug is not in the group). As I explained on IRC, remove_group() is the better way to query for the existence of groups, because when you move a bug into another product and the group doesn't exist, you don't want to throw an error. That's the entry point to brute force the existence of groups.


 There are going to be a lot of programs that depend on solid
> feedback from the WebService, and this will hide bugs and confuse API
> consumers if the removal is silent.

And we don't throw an error if you try to set a field to a value which is already set, AFAIK. Behaving differently for groups doesn't make sense to me, even if we are talking about security here.


> Why add isbuggroup there? A group could not have gotten into this table
> without isbuggroup being true.

Exactly, that's true. But enforcing this here will prevent any "broken" installation from behaving in a weird way (custom code which did something incorrectly comes to mind). It's just an extra security enhancement which doesn't hurt.

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> And we don't throw an error if you try to set a field to a value which is
> already set, AFAIK. Behaving differently for groups doesn't make sense to
> me, even if we are talking about security here.

  But we do throw an error in every other field if you try to set *or* remove a value which doesn't exist at all in Bugzilla.

> > Why add isbuggroup there? A group could not have gotten into this table
> > without isbuggroup being true.
> 
> Exactly, that's true. But enforcing this here will prevent any "broken"
> installation from behaving in a weird way (custom code which did something
> incorrectly comes to mind). It's just an extra security enhancement which
> doesn't hurt.

  Okay, I'd rather skip adding it there, then. I would prefer to catch errors rather than hide them.

Comment 18

[Max Kanat-Alexander]

  Also, theoretically it could hurt; the MySQL query optimizer could choose to use that field's index, which would be just about the poorest-possible index to use (although to be fair, isactive may be equally bad in most installations).

Comment 19

[Max Kanat-Alexander]

(In reply to comment #14)
> Well, feel free to review them. But I'm pretty confident with them. And the
> backport for 3.x is ultra-safe. I simply reword the error message.

  Oh yeah, the backport for 3.x looks awesome. :-)

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 549929 [details] [diff] [review]
patch for 3.4 and 3.6, v1

Review of attachment 549929 [details] [diff] [review]:
-----------------------------------------------------------------

Testing went well for both 3.4 and 3.6. r=dkl

Comment 21

[Frédéric Buclin]

Attachment: patch for 4.2, v2

Now throws an error when remove_group() is called and the bug is not restricted to the given group. But I fixed process_bug.cgi to not call this method when unnecessary, i.e. it only calls it when a group is really added or removed. This way we are consistent with how Bugzilla 2.x and 3.x worked (and this makes more sense to me too).

Comment 22

[Max Kanat-Alexander]

Comment on attachment 550488 [details] [diff] [review]
patch for 4.2, v2

Review of attachment 550488 [details] [diff] [review]:
-----------------------------------------------------------------

Cool! I'm much happier with this one. :-) Quick question, though:

::: process_bug.cgi
@@ +356,5 @@
> +    my @remove_groups;
> +    foreach my $g (@current_groups) {
> +        push(@remove_groups, $g) if grep { $_ eq $g } @{$groups->{remove}};
> +    }
> +    $groups->{remove} = \@remove_groups;

So, why not move this logic into set_all? Having custom code inside of process_bug is difficult, because it means that extensions and other consumers (like the WebService) don't behave consistently with how the web interface works.

::: Bugzilla/WebService/Constants.pm
@@ +112,1 @@
>      group_restriction_not_allowed => 120,

Is the removal error already set to a number somewhere in here, or should we also add it?

Comment 23

[David Lawrence [:dkl]]

(In reply to comment #22)
> @@ +356,5 @@
> > +    my @remove_groups;
> > +    foreach my $g (@current_groups) {
> > +        push(@remove_groups, $g) if grep { $_ eq $g } @{$groups->{remove}};
> > +    }
> > +    $groups->{remove} = \@remove_groups;
> 
> So, why not move this logic into set_all? Having custom code inside of
> process_bug is difficult, because it means that extensions and other
> consumers (like the WebService) don't behave consistently with how the web
> interface works.

I agree that this would be a better fit in set_all. 

dkl

Comment 24

[Max Kanat-Alexander]

Comment on attachment 550488 [details] [diff] [review]
patch for 4.2, v2

Review of attachment 550488 [details] [diff] [review]:
-----------------------------------------------------------------

::: process_bug.cgi
@@ +356,5 @@
> +    my @remove_groups;
> +    foreach my $g (@current_groups) {
> +        push(@remove_groups, $g) if grep { $_ eq $g } @{$groups->{remove}};
> +    }
> +    $groups->{remove} = \@remove_groups;

Okay, LpSolit explained this to me--basically if we try to remove group boxes that are unchecked but not set, this will throw an error, which would be nonsensical from the user's perspective.

Let's only modify the removed groups, not the added groups, to keep this code simple. Also, let's clarify the comment to explain what LpSolit explained to me.

Comment 25

[Frédéric Buclin]

Attachment: patch for 4.2, v2.1

OK, here is my final patch for 4.2. I tested it as much as possible, with different privs. The same code applies to 4.0.2, modulo two conflicts (one in a comment, the other one in the POD).

Comment 26

[Max Kanat-Alexander]

Comment on attachment 550559 [details] [diff] [review]
patch for 4.2, v2.1

Review of attachment 550559 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, looks good from a code perspective. :-) I trust your testing.

::: process_bug.cgi
@@ +354,5 @@
> +    my @remove_groups;
> +    foreach my $g (@{$b->groups_in}) {
> +        push(@remove_groups, $g->name) if grep { $_ eq $g->name } @unchecked_groups;
> +    }
> +    $set_all_fields{groups}->{remove} = \@remove_groups;

For future-proofness, probably best to put "local" in front of that line (in case somebody wants to do something with set_all_fields later on in this file, even though they probably shouldn't).

Comment 27

[Frédéric Buclin]

Attachment: patch for 4.2, v2.2

Now with local ...

Comment 28

[Frédéric Buclin]

Attachment: patch for 4.0, v2

Same code as in the patch for 4.2, v2.2.

Comment 29

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified process_bug.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Group.pm
modified Bugzilla/Product.pm
modified Bugzilla/WebService/Constants.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7888.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified process_bug.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Group.pm
modified Bugzilla/Product.pm
modified Bugzilla/WebService/Constants.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7634.


Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified Bugzilla/Bug.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 7251.


Committing to: bzr+ssh://[e-mail]/bugzilla/3.4/
modified Bugzilla/Bug.pm
modified template/en/default/global/code-error.html.tmpl
Committed revision 6806.

Comment 30

[Frédéric Buclin]

I had to fix webservice_bug_update.t as the error message changed when you cannot add or remove a bug from a group:

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/webservice_bug_update.t
Committed revision 193.

Comment 31

[Max Kanat-Alexander]

Security advisory sent, unlocking this bug.