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)

defect
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: mail, Assigned: mail)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 5 obsolete files)

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
Assignee: query-and-buglist → sgreen+mozilla
Status: NEW → ASSIGNED
Attached patch v1 patch (obsolete) — Splinter Review
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)
Attached patch v2 patch (obsolete) — Splinter Review
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)
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-
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. :)
Flags: blocking4.4+
Keywords: regression
Target Milestone: --- → Bugzilla 4.2
(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.
(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
Attached patch v3 patch (obsolete) — Splinter Review
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)
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
Flags: blocking4.2.3?
Flags: blocking4.2.3? → blocking4.2.3+
(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 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-
(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
Attached patch v4 patch (obsolete) — Splinter Review
Includes suggested changes from last patch
Attachment #648315 - Attachment is obsolete: true
Attachment #648629 - Flags: review?(glob)
Attached patch v5 patch (obsolete) — Splinter Review
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 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+
Flags: approval?
Flags: approval4.2?
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?
(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.
Attached patch v6 patchSplinter Review
Changes suggested for the regex line have been made
Attachment #648630 - Attachment is obsolete: true
Attachment #649153 - Flags: review?
Attachment #649153 - Flags: review? → review?(glob)
Comment on attachment 649153 [details] [diff] [review]
v6 patch

r=glob
Attachment #649153 - Flags: review?(glob) → review+
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+
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.