309952 - (CVE-2010-1204) [SECURITY] A boolean chart search with time tracking fields works for everybody

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Teemu Mannermaa (:wicked)]

A user can execute a boolean chart search that has a time tracking field even
though the user isn't in the group defined by timetrackinggroup param. This way
one could search for all bugs that have worked time equal to 2 and get a list of
all bugs that have taken 2 hours to finish. Time tracking fields are not shown
on the drop-down for these users so URL hacking or a previously saved query
needs to be used to trigger this.

Comment 1

[Frédéric Buclin]

I'm not sure it's a security bug.

Comment 2

[Joel Peshkin]

Well, it is not a very serious one... but it is something we should fix.
 I suggest just adding a check to Search.pm to throw an error when the time
fields are invoked.

Comment 3

[Frédéric Buclin]

joel, do you think we could remove the security flag?

Comment 4

[Joel Peshkin]

I'd leave it set until we have a patch deployed.  It is not a large leak, but it
is one that can be exploited for its little bit of information.

Comment 5

[Joel Peshkin]

Attachment: Eliminate criteria on time fields

Comment 6

[Teemu Mannermaa (:wicked)]

Comment on attachment 199652 [details] [diff] [review]
Eliminate criteria on time fields

>Index: Bugzilla/Search.pm
>===================================================================
>+             unless (Bugzilla->user->in_group(Param('timetrackinggroup'))){
>+                 $term = "1";
>+                 return;
>+             }

You should use $user variable for the current user because whine.pl and
collectstats.pl can pass a user that Bugzilla::Search should use for permission
checking. Now neither of those can use timetracking even if the user they use
have necessary permission.

Also, I don't like the way time tracking fields are silently ignored when a
user has no permission to use them. Now the search seems to succeed but it only
uses non-timetracking criterias. Would it be possible to throw an error
instead?

Comment 7

[Teemu Mannermaa (:wicked)]

2.16 branch is not affected because it doesn't have timetracking fields.

Comment 8

[Frédéric Buclin]

(In reply to comment #6)
> Also, I don't like the way time tracking fields are silently ignored when a
> user has no permission to use them. Now the search seems to succeed but it only
> uses non-timetracking criterias. Would it be possible to throw an error
> instead?

What should be done, IMO, is to ignore those fields, but display a message at
the top of the buglist that this list has been obtained ignoring time-related
fields. This way, your saved searches which used time-related fields in the past
(for instance when you were in the time-tracking group) still return some
results instead of throwing an error.

Comment 9

[Joel Peshkin]

Attachment: Patch v2, 2.20 branch and tip

This fixes the user object.

I did look at the other question.  While it would be possible to return a
warning to buglist for these particular changes, there are many cases where
search silently ignores fields it is not supposed to care about.  It would not
amke sense to warn for this one case only.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> search silently ignores fields it is not supposed to care about.  It would not
> amke sense to warn for this one case only.

ok!

Comment 11

[Teemu Mannermaa (:wicked)]

Comment on attachment 199897 [details] [diff] [review]
Patch v2, 2.20 branch and tip

Looks good on 2.20 branch on tip. Now the timetracking fields are simply ignored in search if the user is not in time tracking group.

Branch 2.18 needs a backport.

Comment 12

[Joel Peshkin]

Attachment: Backport to 2-18

I was only able to do rudimentary testing for this, but it should be the ticket.

Comment 13

[Frédéric Buclin]

Comment on attachment 203219 [details] [diff] [review]
Backport to 2-18

wicked reported the bug, and he reviewed the patches for the tip and 2.20... I let him review this one too. :)

Comment 14

[Teemu Mannermaa (:wicked)]

Comment on attachment 203219 [details] [diff] [review]
Backport to 2-18

>Index: Bugzilla/Search.pm
>===================================================================
>+             if (Param("timetrackinggroup") 
>+                 && &::UserInGroup(Param("timetrackinggroup"))) {

You need to use the possibly passed $user parameter here too or new chart queries can't use time tracking fields. This would mean using the in_group User.pm function. I wouldn't mind if you change this also to return immediately when a user is not in the timetracking group. That way there's no need to change intendation. All this means this patch would be almost like the newer patch.

Comment 15

[Frédéric Buclin]

joel, ping? Could you please update your patch for 2.18? We cannot freeze checkins and start the QA cycle till the backport is ok (which should be in a week or so).

Comment 16

[Joel Peshkin]

Attachment: Patch for 2-18 v2

Here's the backport.  I did only the most rudimentary testing on this.  Wicked:  Can you still test this?

Comment 17

[Joel Peshkin]

Attachment: Patch for 2-18 v2

Here's the backport.  I did only the most rudimentary testing on this.  Wicked:  Can you still test this?

Comment 18

[Teemu Mannermaa (:wicked)]

Comment on attachment 206096 [details] [diff] [review]
Patch for 2-18 v2

This only protects some specific boolean query types but still allows, for example, "estimated_time is greater than zero" query. This is because there's bunch of regexps with no specific field name.

Also, I'd still like to see an immediate return instead of intendation change in 2.18 patch too. Just like on the tip/2.20 patch.

Comment 19

[Teemu Mannermaa (:wicked)]

Comment on attachment 199897 [details] [diff] [review]
Patch v2, 2.20 branch and tip

Looks like same problem also affects tip patch. Sorry for wrong review earlier. :(

Comment 20

[Frédéric Buclin]

looks like this patch won't be ready for the next set of releases. :(

Comment 21

[Frédéric Buclin]

Bugzilla 2.20 is no longer supported. Retargetting to 2.22.

Comment 22

[Max Kanat-Alexander]

Any chance of somebody fixing this? It should be easier now with is_timetracker and the Search.pm refactoring.

Comment 23

[Frédéric Buclin]

Bugzilla 2.x is no longer supported. Retargetting to 3.0.

Comment 24

[Frédéric Buclin]

Bugzilla 3.0 is EOL. We will retarget this bug when it's fixed.

Comment 25

[Max Kanat-Alexander]

Attachment: v1 (3.6)

This was remarkably easy to fix.

Comment 26

[Frédéric Buclin]

Comment on attachment 439667 [details] [diff] [review]
v1 (3.6)

Works fine in 3.7. But the patch needs to be backported as 3.4 and 3.2 do not have TIMETRACKING_FIELDS defined.

Comment 27

[Max Kanat-Alexander]

Attachment: v1 (3.4)

This is a backport. I've tested it only on 3.4, but it should also work on 3.2.

Comment 28

[Frédéric Buclin]

Comment on attachment 444353 [details] [diff] [review]
v1 (3.4)

Works with 3.4 only. is_timetracker() doesn't exist in 3.2, so another backport is required.

Comment 29

[Max Kanat-Alexander]

Attachment: v1 (3.2)

Okay, here's the 3.2 backport. I haven't tested it, because I don't have a live 3.2 installation handy, but it compiles and it's basically identical to the other patches, except that we check timetrackinggroup membership directly, which is also what we do in other parts of Search.pm in 3.2.

Comment 30

[Frédéric Buclin]

Comment on attachment 444873 [details] [diff] [review]
v1 (3.2)

r=LpSolit

Comment 31

[Frédéric Buclin]

ready to land.

Comment 32

[Frédéric Buclin]

Let's put it in our radar for 3.6.1.

Comment 33

[Reed Loden [:reed]]

This is CVE-2010-1204.

Comment 34

[Max Kanat-Alexander]

Okay, I'm going to release today, so checkin will be soon.

Comment 35

[Max Kanat-Alexander]

Attachment: v1 (HEAD)

This required some very minor un-bitrotting for HEAD. Carrying forward review for un-bitrotted patch.

Comment 36

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Search.pm
Committed revision 7246.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified Bugzilla/Search.pm
Committed revision 7112. 

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.4/                         
modified Bugzilla/Search.pm
Committed revision 6761.

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

Comment 37

[Max Kanat-Alexander]

Security advisory sent, unlocking bug.