455099 - Some methods in Search.pm use the wrong user object to check privs

Status: RESOLVED FIXED

(Bugzilla :: Whining, defect)

Description

[Frédéric Buclin]

Attachment: patch for tip, v1

You can pass {user => $some_user} to |new Bugzilla::Search| to get a buglist for that user, but some methods in Search.pm have Bugzilla->user hardcoded, instead of $self->{'user'}, which means some validation checks are made against the wrong user. This is currently problematic when executing whine.pl and collectstats.pl as Bugzilla->user is the default user object, which has no privs at all. This means they miss everything which is restricted to the insider group and the timetracking group.

The other way could also happen, despite nothing in our core code is affected (but custom code could be): if the one calling Bugzilla::Search has more privs than the user who will receive whine mail, he would get data he shouldn't normally get.

Comment 1

[Max Kanat-Alexander]

Comment on attachment 338412 [details] [diff] [review]
patch for tip, v1

Why don't we just call set_user in whine.pl?

Comment 2

[Frédéric Buclin]

Because the code in Search.pm, collectstats.pl and whine.pl is much older than the implementation of set_user, which appeared with email_in.pl in 3.0.

Comment 3

[Frédéric Buclin]

Moreover, I suppose you don't want to use Bugzilla->set_user when already running the script as "you" (or you would call ->set_user all the time to reset the user back to you).

Comment 4

[Max Kanat-Alexander]

Well, it's just that there's lots of code in Bugzilla that uses Bugzilla->user, and who *knows* what kind of subroutines or methods we might call from inside Bugzilla::Search now or in the future.

I think for HEAD, the best solution is to use Bugzilla->set_user.

For the branch, we should do what you're doing here, which is make it use $self->{'user'}.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 338412 [details] [diff] [review]
patch for tip, v1

This looks fine, though.

Comment 6

[Frédéric Buclin]

(In reply to comment #4)
> I think for HEAD, the best solution is to use Bugzilla->set_user.

No. If you do that, I think you can sudo everybody. All you have to do is to create a whine which uses a query with an invalid date, and ThrowUserError will throw an error before Bugzilla->set_user has been set back to its initial value, and you are now logged in as the other user.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> No. If you do that, I think you can sudo everybody. All you have to do is to
> create a whine which uses a query with an invalid date, and ThrowUserError will
> throw an error before Bugzilla->set_user has been set back to its initial
> value, and you are now logged in as the other user.

  Oh, I see, we have whines that send to other users, right.

Comment 8

[Frédéric Buclin]

Attachment: patch for 3.2, v1

Backport for 3.2. $user->is_timetracker doesn't exist in 3.2. No other change.

Comment 9

[Frédéric Buclin]

tip:

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.163; previous revision: 1.162
done

3.2rc1:

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.159.2.3; previous revision: 1.159.2.2
done