788098 - Queries involving group substitution crash when usevisibilitygroups is enabled

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[shirphy]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.2; Trident/4.0; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)

Steps to reproduce:

I want to query all bug submitted by a group's members, and I use the advanced query, and set the Reporter is equal to %group.groupleader%.


Actual results:

I get a error:
Can't call method "id" without a package or object reference at Bugzilla/Group.pm line 192, <DATA> line 558.
For help, please send mail to the webmaster ([e-mail]), giving this error message and the time and date of the error.

But if I set usevisibilitygroups:Off, then query is oK.


Expected results:

According to the doc of bugzilla user guider, I should have get the query list.

Comment 1

[Frédéric Buclin]

Works fine in 4.0.8, but fails in 4.5. I guess the rewrite of Search.pm regressed this. Thanks for catching that!

Comment 2

[Frédéric Buclin]

It's indeed a regression due to bug 574556. This code in Bugzilla::Group->check_members_are_visible() is wrong:

 my $is_visible = grep { $_->id == $_ } @{ $user->visible_groups_inherited };

This should be { $self->id == $_ }.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

There were several bugs related to check_members_are_visible() besides the code in this method itself: first, we were disclosing if you were able to see members of the group before making sure you were allowed to know about the existence of the group. So CVE-2011-2979 was not fully fixed by bug 674497. Secondly, the regexp used before calling _contact_exact_group() was incomplete and didn't match the one used in this method, which could lead to empty group names (e.g. if you missed the trailing %).

Comment 4

[shirphy]

Thanks very much
It's worked.

Comment 5

[:glob ✱]

reopening; we need to get this fix into bugzilla itself, not just your installation :)

Comment 6

[shirphy]

I got a new error when I set the query in Advanced Query with the following
[Assignee] [is equal to] [%group.APPS%]
The Error is:
The group you specified, APPS, is not valid here.

But if I modify the APPS group with the setting:
Groups That Are a Member of This Group
("Users in X are automatically in APPS")
admin
(Means admin users are automatically in APPS)

Then the Query can run and get the result list.
It seems any group must contain admin group?

Since admin group is also in the APPS group, there will be a lot of 
Bugs not in the group APPS but in the admin group. 
So I have to add another additional query limit:
[Assignee] [is not equal to] [admin]
Then the result is what I want. 

So my question is if can I just use a query limit ([Assignee] [is equal to] [%group.APPS%])to get the result?

Thank you very much.

Comment 7

[Frédéric Buclin]

(In reply to shirphy from comment #6)
> So my question is if can I just use a query limit ([Assignee] [is equal to]
> [%group.APPS%])to get the result?

This is not the right place to ask for help. This bug-tracker is to track bugs only. For help, please visit http://www.bugzilla.org/support.

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 658043 [details] [diff] [review]
patch, v1

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

::: Bugzilla/Group.pm
@@ +173,2 @@
>      my $self = shift;
>      my $user = Bugzilla->user;

Just noticed when looking at this method, it would be nice if this would take $user as a parameter so it could be used outside of the context of the current user:

my ($self, $user) = @_;
$user ||= Bugzilla->user;

but another bug for that.

::: Bugzilla/Search.pm
@@ +2102,5 @@
>      my ($self, $args) = @_;
>      my $value = $args->{value};
>      my $user = $self->_user;
> +
> +    if ($value =~ /^\%group\.(?:[^%]+)%$/) {

Nit: Could this be simply /^%group\.[^%]+%$/ without the grouping parens? I don't see the need.

@@ +2124,5 @@
>      $value =~ /\%group\.([^%]+)%/;
> +    my $group_name = $1;
> +    my $group = Bugzilla::Group->check({ name => $group_name, _error => 'invalid_group_name' });
> +    # Pass $group_name instead of $group->name to the error message
> +    # to not leak the existence of the group.

This part needs explaining to me. Group->check will already complain if the group does not exist. So passing $group_name to ThrowUserError for $user->in_group in order to not give away the existence of the group seems redundant and unnecessary. So why not just pass $group->name as before?

I agree with the other change of moving in_group above check_members_are_visible().

Comment 9

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #8)
> Nit: Could this be simply /^%group\.[^%]+%$/ without the grouping parens?

Ah, sure, good point! :)


> This part needs explaining to me. Group->check will already complain if the
> group does not exist. So passing $group_name to ThrowUserError for
> $user->in_group in order to not give away the existence of the group seems
> redundant and unnecessary. So why not just pass $group->name as before?

Because if you pass "secret_group" and the group name is "Secret_Group" and we pass $group->name to the error message, then we would leak that the group exists, because the case of the output would be different than the input string. If the group doesn't exist OR the user doesn't belong to this group, then we don't want to leak the existence of the group, which is why we must display what the user typed.

Comment 10

[David Lawrence [:dkl]]

Ok r=dkl for bugzilla/trunk but patch doesn't apply cleanly to bugzilla/4.2 so holding off:

patching file Bugzilla/Group.pm
Hunk #1 succeeded at 189 (offset 16 lines).
patching file Bugzilla/Search.pm
Hunk #1 succeeded at 2050 (offset -38 lines).
Hunk #2 FAILED at 2106.
1 out of 2 hunks FAILED -- saving rejects to file Bugzilla/Search.pm.rej

dkl

Comment 11

[Frédéric Buclin]

Attachment: patch for 4.4 and trunk, v1.1

I fixed the regexp to remove the useless (?: ).

Comment 12

[Frédéric Buclin]

Attachment: patch for 4.2, v1

Same patch as for trunk/4.4. The only difference is that a line had some extra trailing whitespaces which were removed in 4.4.

Comment 13

[David Lawrence [:dkl]]

Comment on attachment 667590 [details] [diff] [review]
patch for 4.4 and trunk, v1.1

r=dkl from previous review

Comment 14

[David Lawrence [:dkl]]

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

r=dkl for 4.2 as well

Comment 15

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Group.pm
modified Bugzilla/Search.pm
Committed revision 8411.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Group.pm
modified Bugzilla/Search.pm
Committed revision 8406.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Group.pm
modified Bugzilla/Search.pm
Committed revision 8144.