Closed
Bug 779709
Opened 12 years ago
Closed 12 years ago
It is possible to search on history for comments and attachments you cannot see
Categories
(Bugzilla :: Query/Bug List, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.2
People
(Reporter: mail, Assigned: mail)
References
()
Details
(Keywords: regression)
Attachments
(1 file, 5 obsolete files)
2.31 KB,
patch
|
glob
:
review+
|
Details | Diff | Splinter Review |
There is currently no restriction on the bugs_activity table in Bugzilla::Search that prevents you seeing information that you are not entitled too. For example, if you visit the below URL as a non privileged user, https://landfill.bugzilla.org/bugzilla-4.2-branch/buglist.cgi?f1=attachments.description&o1=changedafter&query_format=advanced&v1=2012-08-01&component=Salt&product=FoodReplicator You will see bug 17394 is listed. However, when you view that bug you cannot see the attachment (if you are not in the insiders group). However, Bugzilla::Bug::GetBugActivity does correctly handle this. -- simon
fwiw 4.0 handles this correctly: https://landfill.bugzilla.org/bugzilla-4.0-branch/buglist.cgi?query_format=advanced&list_id=14001&field0-0-0=attachments.description&type0-0-0=changedafter&value0-0-0=%202012-08-01&product=MyOwnBadSelf bug 17068 isn't listed when you're logged out.
Assignee | ||
Updated•12 years ago
|
Assignee: query-and-buglist → sgreen+mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
This seems to fix the issue. If you want me to change the patch so that I don't pass $field_id and $chart_id around (and get $chart_id from $args->{chart_id} and recalculate $field_id again from $args->{field}) let me know, and I will do so.
Attachment #648219 -
Flags: review?(glob)
Assignee | ||
Comment 3•12 years ago
|
||
Actually, passing in $args->field is going to help me for the thing I want to fix (a brc customisation), so this update patch does that.
Attachment #648219 -
Attachment is obsolete: true
Attachment #648219 -
Flags: review?(glob)
Attachment #648220 -
Flags: review?(glob)
Assignee | ||
Comment 4•12 years ago
|
||
BTW: Putting "AND COALESCE(attach_${field_id}_$chart_id.isprivate, 0) = 0" in $join->extra_sql or $join->then_to->extra_sql doesn't achieve the correct result (at least on MySQL). Should also mention that this has been tested on MySQL only. It would be good to give it a once over on PostgreSQL and Oracle too (both database seems to support COALESCE, and the rest (like using then_to should be handled as other parts of the code that do this work correctly.
Comment on attachment 648220 [details] [diff] [review] v2 patch Review of attachment 648220 [details] [diff] [review]: ----------------------------------------------------------------- private comments are also exposed by search, and need to be filtered too (in _long_desc_changedbefore_after). ::: Bugzilla/Search.pm @@ +2821,5 @@ > + > +sub _changed_security_check { > + my ($self, $args, $join) = @_; > + my ($chart_id, $field) = > + @$args{qw(chart_id field)}; nit: no need to wrap this line. @@ +2828,5 @@ > + || ThrowCodeError("invalid_field_name", { field => $field }); > + my $field_id = $field_object->id; > + > + # If the user is not part of the insiders group, they cannot see > + # changes to attachments that are private you should only do this when the field /^attachments\./
Attachment #648220 -
Flags: review?(glob) → review-
Comment 6•12 years ago
|
||
Does anyone know which bug regressed this? And is the Bug.search webservice also affected? I cannot mark it as blocking 4.2.3 for now because there is no such flag yet. :)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Frédéric Buclin from comment #6) > Does anyone know which bug regressed this? It was the big rewrite to Bugzilla::Search in 4.2 that caused this, as glob says it is not affected by 4.0 (and earlier) Bugzilla::Search. > And is the Bug.search webservice > also affected? It isn't affected for Bugzilla 4.2,as that uses Bugzilla::Bug->match, not Bugzilla::Search. It will be an issue for 4.4, which is going to use Bugzilla::Search for the Bug.search RPC call. > I cannot mark it as blocking 4.2.3 for now because there is no such flag > yet. :) I noticed that.
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Byron Jones ‹:glob› from comment #5) > private comments are also exposed by search, and need to be filtered too (in > _long_desc_changedbefore_after). > nit: no need to wrap this line. Ack for both of these. > you should only do this when the field /^attachments\./ That I think is asking for trouble. I imagine these is very little db cost for this change (although I haven't run it through an explain plan), and it is better to be safe than sorry for things like this. For example, there is a field 'attach_data.thedata', which we also want to filter. -- simon
Assignee | ||
Comment 9•12 years ago
|
||
Fixed the first two issues. Since I am disputing the last one, I have not done it, but am happy to do so if you think I should. -- simon
Attachment #648220 -
Attachment is obsolete: true
Attachment #648315 -
Flags: review?(glob)
Assignee | ||
Updated•12 years ago
|
Summary: It is possible to search on history for attachments you cannot see → It is possible to search on history for comments and attachments you cannot see
Updated•12 years ago
|
Flags: blocking4.2.3?
Updated•12 years ago
|
Flags: blocking4.2.3? → blocking4.2.3+
Comment 10•12 years ago
|
||
(In reply to Simon Green from comment #9) > Fixed the first two issues. Since I am disputing the last one, I have not > done it, but am happy to do so if you think I should. if you're searching for 'keywords' 'changed after' 'somedate', the sql generated with your patch has (fieldid 10 is 'keywords' on this db): LEFT JOIN bugs_activity AS act_10_2 ON bugs.bug_id = act_10_2.bug_id AND act_10_2.fieldid = 10 AND act_10_2.bug_when >= '2012-08-01 00:00:00' LEFT JOIN attachments AS attach_10_2 ON act_10_2.attach_id = attach_10_2.attach_id this doesn't make sense .. a change to a keyword is never going to have attach_id set. we shouldn't be joining on tables unless we're going to be using that data; there's no point taking a performance hit for no reason.
Comment 11•12 years ago
|
||
Comment on attachment 648315 [details] [diff] [review] v3 patch r-, as per comment 10. >+ if ( !$self->_user->is_insider ) { remove extra spaces after ( and before ) (both locations) aside from the extra join this patch looks good.
Attachment #648315 -
Flags: review?(glob) → review-
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Byron Jones ‹:glob› from comment #10) > (In reply to Simon Green from comment #9) > > Fixed the first two issues. Since I am disputing the last one, I have not > > done it, but am happy to do so if you think I should. > > if you're searching for 'keywords' 'changed after' 'somedate', the sql > generated with your patch has (fieldid 10 is 'keywords' on this db): > > LEFT JOIN bugs_activity AS act_10_2 ON bugs.bug_id = act_10_2.bug_id > AND act_10_2.fieldid = 10 AND act_10_2.bug_when >= '2012-08-01 > 00:00:00' > LEFT JOIN attachments AS attach_10_2 ON act_10_2.attach_id = > attach_10_2.attach_id > > this doesn't make sense .. a change to a keyword is never going to have > attach_id set. > > > we shouldn't be joining on tables unless we're going to be using that data; > there's no point taking a performance hit for no reason. Your solution isn't going to work either, as flags belonging to private attachments on viewable bugs would still be suitable. For example, on brc mysql> SELECT DISTINCT f.name FROM bugs_activity a JOIN fielddefs f ON a.fieldid = f.id WHERE attach_id IS NOT NULL; +-------------------------+ | name | +-------------------------+ | attachments.isobsolete | | attachments.mimetype | | attachments.ispatch | | attachments.filename | | attachments.isprivate | | attachments.description | | flagtypes.name | +-------------------------+ And I assume bmo are the same. So what is the correct solution? Only add this clause if the field name matches /^(attach|flagtypes.name$)/ . What happens when (if) we add keywords to attachments? -- simon
Assignee | ||
Comment 13•12 years ago
|
||
Includes suggested changes from last patch
Attachment #648315 -
Attachment is obsolete: true
Attachment #648629 -
Flags: review?(glob)
Assignee | ||
Comment 14•12 years ago
|
||
16:08 <%reed> simon: except you didn't remove the spaces 16:08 <%reed> ;) 16:09 < simon> Yeah I did. 16:09 < simon> (both locations) <-- oops. Enough said.
Attachment #648629 -
Attachment is obsolete: true
Attachment #648629 -
Flags: review?(glob)
Attachment #648630 -
Flags: review?(glob)
Comment 15•12 years ago
|
||
Comment on attachment 648630 [details] [diff] [review] v5 patch r=glob >+ if ($field =~ /^(flagtypes.name$|attach)/ and !$self->_user->is_insider) { on commit we need to escape that period .. /^(?:flagtypes\.name$|attach)/
Attachment #648630 -
Flags: review?(glob) → review+
Updated•12 years ago
|
Flags: approval4.2?
Comment 16•12 years ago
|
||
Comment on attachment 648630 [details] [diff] [review] v5 patch >+ if ($field =~ /^(flagtypes.name$|attach)/ and !$self->_user->is_insider) { How do you distinguish bug flags from attachment flags?
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Frédéric Buclin from comment #16) > Comment on attachment 648630 [details] [diff] [review] > v5 patch > > >+ if ($field =~ /^(flagtypes.name$|attach)/ and !$self->_user->is_insider) { > > How do you distinguish bug flags from attachment flags? I don't. Bug flags will have a bugs_activity.attach_id of NULL and the COALESCE clause means they will be treated as if they were publicly available.
Assignee | ||
Comment 18•12 years ago
|
||
Changes suggested for the regex line have been made
Attachment #648630 -
Attachment is obsolete: true
Attachment #649153 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #649153 -
Flags: review? → review?(glob)
Comment 19•12 years ago
|
||
Comment on attachment 649153 [details] [diff] [review] v6 patch r=glob
Attachment #649153 -
Flags: review?(glob) → review+
Comment 20•12 years ago
|
||
Per my discussion with glob, dkl and simon on IRC, the biggest leak is the attachment description, but this requires that you already know it (substrings do not work) and that the attachment description has been altered already (to be listed in the bugs_activity table). So this is not really an issue. Other issues are pretty minor (knowing that a comment has been marked as private or that a private attachment has been marked as obsolete is pretty harmless). So there is no need for a CVE number nor a security advisory. The patch can be checked in immediately and the security flag removed once the bug is marked as fixed.
Flags: approval?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
Comment 21•12 years ago
|
||
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Search.pm Committed revision 8331. Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/4.2/ modified Bugzilla/Search.pm Committed revision 8115.
Group: bugzilla-security
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•