Last Comment Bug 779709 - It is possible to search on history for comments and attachments you cannot see
: It is possible to search on history for comments and attachments you cannot see
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 4.2
: All All
: -- major (vote)
: Bugzilla 4.2
Assigned To: mail
: default-qa
Mentors:
https://landfill.bugzilla.org/bugzill...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 19:05 PDT by mail
Modified: 2013-07-22 20:23 PDT (History)
5 users (show)
LpSolit: approval+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 patch (1.78 KB, patch)
2012-08-01 22:17 PDT, mail
no flags Details | Diff | Splinter Review
v2 patch (1.92 KB, patch)
2012-08-01 22:21 PDT, mail
glob: review-
Details | Diff | Splinter Review
v3 patch (2.24 KB, patch)
2012-08-02 05:02 PDT, mail
glob: review-
Details | Diff | Splinter Review
v4 patch (2.31 KB, patch)
2012-08-02 23:05 PDT, mail
no flags Details | Diff | Splinter Review
v5 patch (2.31 KB, patch)
2012-08-02 23:12 PDT, mail
glob: review+
Details | Diff | Splinter Review
v6 patch (2.31 KB, patch)
2012-08-05 16:41 PDT, mail
glob: review+
Details | Diff | Splinter Review

Description mail 2012-08-01 19:05:40 PDT
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
Comment 2 mail 2012-08-01 22:17:39 PDT
Created attachment 648219 [details] [diff] [review]
v1 patch

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.
Comment 3 mail 2012-08-01 22:21:51 PDT
Created attachment 648220 [details] [diff] [review]
v2 patch

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.
Comment 4 mail 2012-08-01 22:41:22 PDT
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 5 Byron Jones ‹:glob› 2012-08-01 23:46:26 PDT
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\./
Comment 6 Frédéric Buclin 2012-08-02 03:20:45 PDT
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. :)
Comment 7 mail 2012-08-02 03:39:13 PDT
(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.
Comment 8 mail 2012-08-02 03:45:32 PDT
(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
Comment 9 mail 2012-08-02 05:02:14 PDT
Created attachment 648315 [details] [diff] [review]
v3 patch

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
Comment 10 Byron Jones ‹:glob› 2012-08-02 08:17:10 PDT
(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 Byron Jones ‹:glob› 2012-08-02 08:21:56 PDT
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.
Comment 12 mail 2012-08-02 16:22:56 PDT
(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
Comment 13 mail 2012-08-02 23:05:26 PDT
Created attachment 648629 [details] [diff] [review]
v4 patch

Includes suggested changes from last patch
Comment 14 mail 2012-08-02 23:12:20 PDT
Created attachment 648630 [details] [diff] [review]
v5 patch

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.
Comment 15 Byron Jones ‹:glob› 2012-08-02 23:35:57 PDT
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)/
Comment 16 Frédéric Buclin 2012-08-03 02:43:48 PDT
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?
Comment 17 mail 2012-08-03 05:29:42 PDT
(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.
Comment 18 mail 2012-08-05 16:41:18 PDT
Created attachment 649153 [details] [diff] [review]
v6 patch

Changes suggested for the regex line have been made
Comment 19 Byron Jones ‹:glob› 2012-08-05 22:19:35 PDT
Comment on attachment 649153 [details] [diff] [review]
v6 patch

r=glob
Comment 20 Frédéric Buclin 2012-08-06 17:07:07 PDT
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.
Comment 21 Byron Jones ‹:glob› 2012-08-06 22:00:01 PDT
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.

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