Bug 309952 (CVE-2010-1204)

[SECURITY] A boolean chart search with time tracking fields works for everybody

RESOLVED FIXED in Bugzilla 3.2

Status

()

Bugzilla
Creating/Changing Bugs
--
minor
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: wicked, Assigned: Max Kanat-Alexander)

Tracking

2.20
Bugzilla 3.2
Bug Flags:
approval +
approval3.6 +
blocking3.6.1 +
approval3.4 +
approval3.2 +

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
I'm not sure it's a security bug.

Comment 2

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

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

Comment 4

12 years ago
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

12 years ago
Created attachment 199652 [details] [diff] [review]
Eliminate criteria on time fields

Updated

12 years ago
Attachment #199652 - Flags: review?(wicked)
(Reporter)

Comment 6

12 years ago
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?
Attachment #199652 - Flags: review?(wicked) → review-
(Reporter)

Comment 7

12 years ago
2.16 branch is not affected because it doesn't have timetracking fields.
Assignee: create-and-change → bugreport
Target Milestone: --- → Bugzilla 2.18

Comment 8

12 years ago
(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

12 years ago
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.
Attachment #199652 - Attachment is obsolete: true
Attachment #199897 - Flags: review?(wicked)

Comment 10

12 years ago
(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!
Status: NEW → ASSIGNED
Whiteboard: [doesn't affect 2.16]
(Reporter)

Comment 11

12 years ago
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.
Attachment #199897 - Attachment description: Patch v2 → Patch v2, 2.20 branch and tip
Attachment #199897 - Flags: review?(wicked) → review+

Updated

12 years ago
Flags: approval?
Flags: approval2.20?
Whiteboard: [doesn't affect 2.16] → [doesn't affect 2.16][patch needed for 2.18.5][ready for 2.20.1][ready for 2.21.2]
(Reporter)

Updated

12 years ago
Blocks: 312700

Comment 12

12 years ago
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.
Attachment #203219 - Flags: review?(LpSolit)

Comment 13

12 years ago
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. :)
Attachment #203219 - Flags: review?(LpSolit) → review?(wicked)
(Reporter)

Comment 14

12 years ago
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.
Attachment #203219 - Flags: review?(wicked) → review-

Comment 15

12 years ago
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).

Updated

12 years ago
Blocks: 320312

Comment 16

12 years ago
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?
Attachment #203219 - Attachment is obsolete: true
Attachment #206094 - Flags: review?

Comment 17

12 years ago
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?
Attachment #206096 - Flags: review?(wicked)

Updated

12 years ago
Attachment #206094 - Attachment is obsolete: true
Attachment #206094 - Flags: review?

Updated

12 years ago
Whiteboard: [doesn't affect 2.16][patch needed for 2.18.5][ready for 2.20.1][ready for 2.21.2] → [doesn't affect 2.16][patch for 2.18.5 awaiting review][ready for 2.20.1][ready for 2.21.2]
(Reporter)

Comment 18

12 years ago
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.
Attachment #206096 - Flags: review?(wicked) → review-
(Reporter)

Comment 19

12 years ago
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. :(
Attachment #199897 - Flags: review+ → review-

Comment 20

12 years ago
looks like this patch won't be ready for the next set of releases. :(
Flags: approval?
Flags: approval2.20?
Whiteboard: [doesn't affect 2.16][patch for 2.18.5 awaiting review][ready for 2.20.1][ready for 2.21.2] → [doesn't affect 2.16]

Updated

12 years ago
No longer blocks: 320312

Updated

10 years ago
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Group: webtools-security → bugzilla-security
Group: bugzilla-security → webtools-security
Group: webtools-security → bugzilla-security

Comment 21

9 years ago
Bugzilla 2.20 is no longer supported. Retargetting to 2.22.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
(Assignee)

Comment 22

8 years ago
Any chance of somebody fixing this? It should be easier now with is_timetracker and the Search.pm refactoring.
Assignee: bugreport → create-and-change
(Reporter)

Updated

8 years ago
No longer blocks: 312700

Comment 23

8 years ago
Bugzilla 2.x is no longer supported. Retargetting to 3.0.
Target Milestone: Bugzilla 2.22 → Bugzilla 3.0

Updated

8 years ago
Status: ASSIGNED → NEW
(Reporter)

Updated

7 years ago
Assignee: create-and-change → wicked

Comment 24

7 years ago
Bugzilla 3.0 is EOL. We will retarget this bug when it's fixed.
Target Milestone: Bugzilla 3.0 → ---
(Assignee)

Comment 25

7 years ago
Created attachment 439667 [details] [diff] [review]
v1 (3.6)

This was remarkably easy to fix.
Assignee: wicked → mkanat
Attachment #199897 - Attachment is obsolete: true
Attachment #206096 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #439667 - Flags: review?(wicked)
Attachment #439667 - Flags: review?(LpSolit)
(Assignee)

Updated

7 years ago
Whiteboard: [doesn't affect 2.16]
Target Milestone: --- → Bugzilla 3.2

Updated

7 years ago
Attachment #439667 - Flags: review?(wicked)
Attachment #439667 - Flags: review?(LpSolit)
Attachment #439667 - Flags: review+

Comment 26

7 years ago
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.

Updated

7 years ago
Flags: approval?
Flags: approval3.6?
Summary: A boolean chart search with time tracking fields works for everybody → [SECURITY] A boolean chart search with time tracking fields works for everybody
Whiteboard: [backports for 3.4 and 3.2 needed]
(Assignee)

Comment 27

7 years ago
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.
Attachment #444353 - Flags: review?(LpSolit)

Comment 28

7 years ago
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.
Attachment #444353 - Attachment description: v1 (3.4 and 3.2) → v1 (3.4)
Attachment #444353 - Flags: review?(LpSolit) → review+

Updated

7 years ago
Flags: approval3.4?
Whiteboard: [backports for 3.4 and 3.2 needed] → [backports for 3.2 needed]
(Assignee)

Comment 29

7 years ago
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.
Attachment #444873 - Flags: review?(LpSolit)

Comment 30

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

r=LpSolit
Attachment #444873 - Flags: review?(LpSolit) → review+

Comment 31

7 years ago
ready to land.
Flags: approval3.2?
Whiteboard: [backports for 3.2 needed]

Comment 32

7 years ago
Let's put it in our radar for 3.6.1.
Flags: blocking3.6.1+
This is CVE-2010-1204.
Alias: CVE-2010-1204

Updated

7 years ago
Blocks: 567670
(Assignee)

Comment 34

7 years ago
Okay, I'm going to release today, so checkin will be soon.
Flags: approval?
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval3.2?
Flags: approval3.2+
Flags: approval+
(Assignee)

Comment 35

7 years ago
Created attachment 453763 [details] [diff] [review]
v1 (HEAD)

This required some very minor un-bitrotting for HEAD. Carrying forward review for un-bitrotted patch.
Attachment #453763 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #439667 - Attachment description: v1 (HEAD) → v1 (3.6)
(Assignee)

Comment 36

7 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

7 years ago
Security advisory sent, unlocking bug.
Group: bugzilla-security
You need to log in before you can comment on or make changes to this bug.