255606 - It is possible to get all bugs from buglist.cgi by passing URL parameters to buglist.cgi that create no Search.pm criteria

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[till busch]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.2; Linux) (KHTML, like Gecko)
Build Identifier: 

when querying for bug lists there should be a limit on returned bugs. 
 
querying buglist.cgi without any parameters in the url will show the following 
message: "You may not search, or create saved searches, without any search 
terms." 
 
querying buglist.cgi?format=simple will try to display ALL bugs in the 
database. (for large databases this will virtually never finish). 
 
check the following url to see what happens: 
http://landfill.bugzilla.org/bugzilla-tip/buglist.cgi?format=simple 
 
i've tried the same query on http://bugzilla.mozilla.org by accident. and just 
got stuck until giving up. 
 
imho, either buglist.cgi should check this, or (less safe) the default 
template for list/list-simple.html.tmpl should be fixed. 

Reproducible: Always
Steps to Reproduce:
1. got to http://bugzilla.mozilla.org/buglist.cgi?format=simple 
2. wait forever 
 
Actual Results:

Comment 1

[till busch]

i just checked that again in some more detail. buglist.cgi with any non-empty  
query string will return all bugs in the database.  
  
best solution to me would be an adjustable limit in params. 

Comment 2

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 3

[Dave Miller [:justdave]]

buglist.cgi without any params currently spits out an error that you need to provide some search terms.

However, the test case here still works, and I've run into it a few times myself chasing DoS issues with bmo.  Our check for "no parameters passed" needs to exclude the non-search-criteria parameters before checking if any params were passed.

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to buglist.cgi to disregard format/query_format when checking for no params (v1)

This is still an issue on current trunk. The code change should at least remove format before checking for zero params. This patch removes format and query_format before doing the check and then puts them back for use later.

Dave

Comment 5

[Max Kanat-Alexander]

Comment on attachment 501469 [details] [diff] [review]
Patch to buglist.cgi to disregard format/query_format when checking for no params (v1)

There are too many things like this to start picking out individual ones and fixing them. There could be an almost infinite number of bad or extra parameters.

Instead, the fix that I'm going to do for 4.2 is to have Search.pm do this check itself--if it gets no criteria at all, deny the query.

Comment 6

[Max Kanat-Alexander]

I'm morphing this bug to be a generally tracker for the fact that searches can be done with no criteria.

This *can* be fixed in 4.2, so we should fix it.

Note that 4.2 somewhat mitigates this problem already by limiting searches to a maximum of 10,000 bugs.

Comment 8

[Frédéric Buclin]

Attachment: patch, v2

When asking for the SQL query via $search->sql, this method asks for the WHERE condition by calling _sql_where(). If this method gets no data from the clause object and the specific_search_allow_empty_words parameter is false, then it throws an error.

Comment 9

[Frédéric Buclin]

If that's not the right approach, feel free to take the bug.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 549661 [details] [diff] [review]
patch, v2

This is an interesting idea. I think this will work. We should rename the parameter and its description, though, because this will prevent you from doing this *anywhere*, including reports, duplicates, graphs, charts, and advanced search.

Comment 11

[Max Kanat-Alexander]

By the way, I think you will also have to fix xt/search.t to expect this error on certain tests.

Comment 12

[Frédéric Buclin]

Attachment: patch, v3

I didn't update the tests in xt/ because 1) it's not part of the core code (in the same way QA tests aren't), 2) good luck to understand what tests there try to do, and 3) prove xt/search.t immediately fails in Bugzilla::Test::Search::_create_one_bug(), complaining that the user it not allowed to change the priority of the bug. I don't want to debug it.

Comment 13

[Frédéric Buclin]

Note that I wrote this patch on top of the one from bug 676200, so the other one must be applied first.

Comment 14

[Frédéric Buclin]

Comment on attachment 550346 [details] [diff] [review]
patch, v3

This patch breaks (tabular) reports where you want e.g. all bugs, but triaged by some given fields (for instance: status vs product). In this case, the parameter shouldn't apply as it's a valid report. Only requests from the search page should be affected by this parameter. Does this mean the check should be moved out of Search.pm and into buglist.cgi? The problem is that it would have to access some internals of the Search object, which is not desired. Another solution could be to pass an optional argument to Bugzilla::Search->new(), and if true, does the check I proposed. By defaut, no check would be done. This would also be safer as we could control when to throw an error.

Comment 15

[Frédéric Buclin]

Attachment: patch, v4

We now pass check_empty => 1 to ask Search.pm to do the check for us. This option is only effective if the parameter forbids empty searches.

Comment 16

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #15)
> Created attachment 551016 [details] [diff] [review] [diff] [details] [review]
> patch, v4
> 
> We now pass check_empty => 1 to ask Search.pm to do the check for us. This
> option is only effective if the parameter forbids empty searches.

  Cool, the parameter was what I was going to suggest. However, instead I'd like it to be reversed--I'd like to have an "allow_all_bugs" parameter.

Comment 17

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #16)
>   Cool, the parameter was what I was going to suggest. However, instead I'd
> like it to be reversed--I'd like to have an "allow_all_bugs" parameter.

Hum, this is inconsistent with how the 'search_allow_no_criteria' parameter works. The goal of this parameter is to know whether we should check if some criteria has been passed or not, so it should at least start with "check_". "allow_all_bugs" sounds like it could bypass the parameter defined by the admin.

Comment 18

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #17)
> Hum, this is inconsistent with how the 'search_allow_no_criteria' parameter
> works. The goal of this parameter is to know whether we should check if some
> criteria has been passed or not, so it should at least start with "check_".
> "allow_all_bugs" sounds like it could bypass the parameter defined by the
> admin.

  I do understand that it's opposite from how the admin Parameter works, but what I'm mostly concerned about is consistency of behavior. That is, I want Search.pm to deny people grabbing all the bugs in the DB by default. Developers should have to explicitly tell Search.pm "it's okay to do this crazy thing, here."

Comment 19

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #18)
> what I'm mostly concerned about is consistency of behavior. That is, I want
> Search.pm to deny people grabbing all the bugs in the DB by default.

Ah no, this won't work. There are tons of calls to Bugzilla::Search which do not expect this restriction by default (old and new charts, the whining system, etc...). I'm pretty sure reversing the logic would break a lot of stuff. And there are probably several tens of places to fix.

Comment 20

[Max Kanat-Alexander]

Hmm, okay. Could we actually do an analysis of how many places would need fixing? Whines should respect the preference and break. Old Charts should allow finding all bugs, New Charts should probably not.

Comment 21

[Frédéric Buclin]

The problem is that you can define a saved search to get all bugs while the parameter is disabled, and when the parameter is enabled, you suddenly have an unallowed query in hands. This happened to me when running whine.pl, because one of my queries was returning all bugs. In that case, whine.pl stopped and prevented all other whines from being executed. And comment 14 is another good example of where this could be a problem.

Comment 22

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #21)
> In that case, whine.pl
> stopped and prevented all other whines from being executed.

  Yeah. In that case, we need to fix that problem first. whine.pl should send you a message saying that there was an error, and it should not stop the other searches from happening. This wouldn't be too hard, I'm pretty sure.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 551016 [details] [diff] [review]
patch, v4

So yeah, r- with my above comments.

Comment 24

[Frédéric Buclin]

All 6 places which call 'new Bugzilla::Search':

report.cgi,128:
    my $search = new Bugzilla::Search(

query.cgi, line 129:
    my $search = new Bugzilla::Search(params => scalar $buf->Vars);

collectstats.pl, line 494:
    my $search = new Bugzilla::Search('params' => scalar $cgi->Vars,

buglist.cgi, line 773:
    my $search = new Bugzilla::Search('fields' => \@selectcolumns,

xt/lib/Bugzilla/Test/Search/FieldTest.pm, line 564:
    lives_ok { $search = new Bugzilla::Search(@args) }

whine.pl, line 449:
    my $search = new Bugzilla::Search(


report.cgi must be able to get all bugs, always.
query.cgi only triggers this code if a param matching "field-?\d+" is found and then calls $search->boolean_charts_to_custom_search. This code is not affected by this bug, AFAIK, as $search->url is never called.
collectstats.pl simply counts the number of bugs matching the query. IMO, it makes total sense to let it count the total number of bugs in the DB.
buglist.cgi must respect what the parameter says.
whine.pl should respect what the parameter says, I agree. Else you could use it to get all bugs from the DB to bypass restrictions on buglist.cgi.
xt/lib/Bugzilla/Test/Search/FieldTest.pm probably should be independent of the parameter and so should be allowed to get all bugs if necessary.

To summarize:

Should be allowed to get all bugs, always: report.cgi, collectstats.pl, xt/lib/Bugzilla/Test/Search/FieldTest.pm

Doesn't matter (unaffected): query.cgi

Should not be allowed to get all bugs, unless otherwise specified: buglist.cgi, whine.pl

Comment 25

[Max Kanat-Alexander]

> report.cgi must be able to get all bugs, always.

  I wonder about that, actually. Does bmo hang forever if you try to do reporting with no criteria? If it can have the same server-side effect, perhaps we should also forbid it.

Comment 26

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #25)
>   I wonder about that, actually. Does bmo hang forever if you try to do
> reporting with no criteria?

It takes only a few seconds when passing no criteria.

Comment 27

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #26)
> (In reply to Max Kanat-Alexander from comment #25)
> >   I wonder about that, actually. Does bmo hang forever if you try to do
> > reporting with no criteria?
> 
> It takes only a few seconds when passing no criteria.

  Okay. So report.cgi can get all bugs safely, then, that's fine.

Comment 28

[Frédéric Buclin]

Attachment: patch, v5

It appeared that the only two places which should be allowed to run queries with no criteria, report.cgi and collectstats.pl, are also the only two to use allow_unlimited. This coincidence makes sense as on large DB, no criteria involves a large number of bugs being returned. So I reused this argument in collectstats.pl. This also fixes a regression in 4.2 due to bug 632717 where collecstats.pl could suddently stop reporting a number of bugs larger than max_search_results. On installations like bmo, it's very likely than the number of bugs returned by collectstats.pl is larger than max_search_results, e.g. RESOLVED bugs in the Bugzilla product (currently at 12,000 bugs).

Comment 29

[Max Kanat-Alexander]

Comment on attachment 575904 [details] [diff] [review]
patch, v5

Review of attachment 575904 [details] [diff] [review]:
-----------------------------------------------------------------

This looks awesome! :-)

::: whine.pl
@@ +446,5 @@
>              'user'   => $args->{'recipient'}, # the search runs as the recipient
>          );
> +        # If a query fails for whatever reason, it shouldn't kill the script.
> +        my $sqlquery = eval { $search->sql };
> +        next if $@;

Yessss. This should also be backported to 4.0.

But we should probably print out a warning? So at least the admin can have some idea why the user's whine isn't working, when they ask.

Comment 30

[Frédéric Buclin]

Attachment: patch, v6

I added a message when a query failed, so that the admin has a chance to know what's going on. Carrying forward mkanat's r+.

Comment 31

[Frédéric Buclin]

Attachment: patch, v6

Oops, the previous patch was malformed.

Comment 32

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified buglist.cgi
modified collectstats.pl
modified whine.pl
modified Bugzilla/Config.pm
modified Bugzilla/Search.pm
modified Bugzilla/Config/Query.pm
modified template/en/default/admin/params/query.html.tmpl
modified template/en/default/global/messages.html.tmpl
modified template/en/default/search/search-specific.html.tmpl
Committed revision 8012.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified buglist.cgi
modified collectstats.pl
modified whine.pl
modified Bugzilla/Config.pm
modified Bugzilla/Search.pm
modified Bugzilla/Config/Query.pm
modified template/en/default/admin/params/query.html.tmpl
modified template/en/default/global/messages.html.tmpl
modified template/en/default/search/search-specific.html.tmpl
Committed revision 7961.

Comment 33

[Max Kanat-Alexander]

Comment on attachment 577015 [details] [diff] [review]
patch, v6

Review of attachment 577015 [details] [diff] [review]:
-----------------------------------------------------------------

::: whine.pl
@@ +449,5 @@
> +        my $sqlquery = eval { $search->sql };
> +        if ($@) {
> +            say get_text('whine_query_failed', { query_name => $thisquery->{'name'},
> +                                                 author => $args->{'author'},
> +                                                 reason => $@ });

That really needs to be a "warn" instead of a "say". Most people redirect output to /dev/null for cron scripts. Also, errors should not be on STDOUT. (Feel free to fix this without filing a bug and without review.)

Comment 34

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #33)
> Also, errors should not be on STDOUT.

Yeah, wicked already suggested that. This is fixed.

Comment 35

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #34)
> (In reply to Max Kanat-Alexander from comment #33)
> > Also, errors should not be on STDOUT.
> 
> Yeah, wicked already suggested that. This is fixed.

  Oh, okay. Awesome.

Comment 36

[Frédéric Buclin]

Added to relnotes in bug 713346.