Last Comment Bug 309952 - (CVE-2010-1204) [SECURITY] A boolean chart search with time tracking fields works for everybody
(CVE-2010-1204)
: [SECURITY] A boolean chart search with time tracking fields works for everybody
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.20
: All All
: -- minor (vote)
: Bugzilla 3.2
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
Depends on:
Blocks: 567670
  Show dependency treegraph
 
Reported: 2005-09-25 03:38 PDT by Teemu Mannermaa (:wicked)
Modified: 2010-06-24 16:03 PDT (History)
6 users (show)
mkanat: approval+
mkanat: approval3.6+
LpSolit: blocking3.6.1+
mkanat: approval3.4+
mkanat: approval3.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Eliminate criteria on time fields (2.86 KB, patch)
2005-10-15 07:15 PDT, Joel Peshkin
wicked: review-
Details | Diff | Review
Patch v2, 2.20 branch and tip (3.06 KB, patch)
2005-10-17 21:44 PDT, Joel Peshkin
wicked: review-
Details | Diff | Review
Backport to 2-18 (6.04 KB, patch)
2005-11-15 20:31 PST, Joel Peshkin
wicked: review-
Details | Diff | Review
Patch for 2-18 v2 (6.05 KB, patch)
2005-12-16 08:56 PST, Joel Peshkin
no flags Details | Diff | Review
Patch for 2-18 v2 (6.05 KB, patch)
2005-12-16 08:57 PST, Joel Peshkin
wicked: review-
Details | Diff | Review
v1 (3.6) (526 bytes, patch)
2010-04-16 20:34 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review
v1 (3.4) (706 bytes, patch)
2010-05-09 23:14 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review
v1 (3.2) (670 bytes, patch)
2010-05-12 06:52 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review
v1 (HEAD) (500 bytes, patch)
2010-06-24 10:05 PDT, Max Kanat-Alexander
mkanat: review+
Details | Diff | Review

Description Teemu Mannermaa (:wicked) 2005-09-25 03:38:50 PDT
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 2005-09-25 03:43:30 PDT
I'm not sure it's a security bug.
Comment 2 Joel Peshkin 2005-09-26 06:43:18 PDT
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 2005-09-26 10:22:00 PDT
joel, do you think we could remove the security flag?
Comment 4 Joel Peshkin 2005-09-26 10:27:43 PDT
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 2005-10-15 07:15:58 PDT
Created attachment 199652 [details] [diff] [review]
Eliminate criteria on time fields
Comment 6 Teemu Mannermaa (:wicked) 2005-10-17 04:54:44 PDT
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) 2005-10-17 04:56:23 PDT
2.16 branch is not affected because it doesn't have timetracking fields.
Comment 8 Frédéric Buclin 2005-10-17 06:29:04 PDT
(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 2005-10-17 21:44:23 PDT
Created attachment 199897 [details] [diff] [review]
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 2005-10-18 03:36:01 PDT
(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) 2005-10-24 08:28:34 PDT
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 2005-11-15 20:31:50 PST
Created attachment 203219 [details] [diff] [review]
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 2005-11-16 01:43:21 PST
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) 2005-11-20 13:16:30 PST
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 2005-12-11 10:03:52 PST
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 2005-12-16 08:56:20 PST
Created attachment 206094 [details] [diff] [review]
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 2005-12-16 08:57:38 PST
Created attachment 206096 [details] [diff] [review]
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) 2005-12-23 06:22:30 PST
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) 2005-12-23 06:31:41 PST
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 2005-12-23 10:03:00 PST
looks like this patch won't be ready for the next set of releases. :(
Comment 21 Frédéric Buclin 2008-12-07 06:25:26 PST
Bugzilla 2.20 is no longer supported. Retargetting to 2.22.
Comment 22 Max Kanat-Alexander 2009-02-02 12:21:33 PST
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 2009-07-28 13:08:14 PDT
Bugzilla 2.x is no longer supported. Retargetting to 3.0.
Comment 24 Frédéric Buclin 2010-04-13 00:05:42 PDT
Bugzilla 3.0 is EOL. We will retarget this bug when it's fixed.
Comment 25 Max Kanat-Alexander 2010-04-16 20:34:35 PDT
Created attachment 439667 [details] [diff] [review]
v1 (3.6)

This was remarkably easy to fix.
Comment 26 Frédéric Buclin 2010-04-29 10:40:46 PDT
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 2010-05-09 23:14:06 PDT
Created attachment 444353 [details] [diff] [review]
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 2010-05-12 06:46:21 PDT
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 2010-05-12 06:52:38 PDT
Created attachment 444873 [details] [diff] [review]
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 2010-05-12 07:17:02 PDT
Comment on attachment 444873 [details] [diff] [review]
v1 (3.2)

r=LpSolit
Comment 31 Frédéric Buclin 2010-05-12 07:17:19 PDT
ready to land.
Comment 32 Frédéric Buclin 2010-05-13 06:40:49 PDT
Let's put it in our radar for 3.6.1.
Comment 33 Reed Loden [:reed] (use needinfo?) 2010-05-14 13:03:03 PDT
This is CVE-2010-1204.
Comment 34 Max Kanat-Alexander 2010-06-24 09:57:11 PDT
Okay, I'm going to release today, so checkin will be soon.
Comment 35 Max Kanat-Alexander 2010-06-24 10:05:48 PDT
Created attachment 453763 [details] [diff] [review]
v1 (HEAD)

This required some very minor un-bitrotting for HEAD. Carrying forward review for un-bitrotted patch.
Comment 36 Max Kanat-Alexander 2010-06-24 10:11:11 PDT
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://mkanat%40bugzilla.org@bzr.mozilla.org/bugzilla/3.2/   
modified Bugzilla/Search.pm
Committed revision 6383.
Comment 37 Max Kanat-Alexander 2010-06-24 16:03:52 PDT
Security advisory sent, unlocking bug.

Note You need to log in before you can comment on or make changes to this bug.