859118 - Bug.search called with no arguments returns all visible bugs, ignoring max_search_results and search_allow_no_criteria

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Frédéric Buclin]

While tracking why landfill was running out of memory, it appeared that someone was calling Bug.search with no arguments, making Bugzilla to return all bugs. I tried locally, and Bug.search totally ignores max_search_results (implemented in Bugzilla 4.2, see bug 632717) and search_allow_no_criteria (also implemented in Bugzilla 4.2, see bug 255606). So there is no way to prevent Bug.search from collecting all bugs, making the server to run out of memory or uses all its CPU in some cases. This could be used as a DoS attack against a server.

This is not exactly a security bug, so we can remove the sec flag once we have a patch ready for checkin.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (v1

Comment 2

[Frédéric Buclin]

Comment on attachment 738748 [details] [diff] [review]
Patch to disallow empty search params and unlimited results for Bug.search (v1

>+    my $allow_unlimited = delete $params->{allow_unlimited};

allow_unlimited is internal to Bugzilla search code and shouldn't be accessible from our API. If an admin limits the number of bugs to return, then it must be followed. Else it's too easy to bypass this restriction.


>+    if (!$params->{WHERE}
>+        && !grep(!/(offset|limit|where)/i, keys %$params)

I don't see why you look at offset, limit or where. If there is no WHERE part, then the query has no criteria and it should be rejected.


>+        ThrowUserError('buglist_parameters_required');

It must be listed under the Errors section.


>+In addition to the standard errors that all webservice functions can throw,
>+this function will throw an error if no search parameters are passed in and
>+the administrator has disallowed that. Otherwise if you specify an invalid
>+value for a particular field, you just won't get any results for that value.

This is not the right place to mention the new error. Please give it an ID and list it as we usually do.


Otherwise looks good.

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (v2)

>> +    my $allow_unlimited = delete $params->{allow_unlimited};
>
>allow_unlimited is internal to Bugzilla search code and shouldn't be accessible
>from our API. If an admin limits the number of bugs to return, then it must be
>followed. Else it's too easy to bypass this restriction.

I changed it to use $params->{limit} = 0 to mean allowing unlimited results. We do this currently for buglist.cgi and did not require a new param to be added.

>> +    if (!$params->{WHERE}
>> +	  && !grep(!/(offset|limit|where)/i, keys %$params)
>
>I don't see why you look at offset, limit or where. If there is no WHERE part,
>then the query has no criteria and it should be rejected.

WHERE is created inside Bug.search by looking for special field names. If those field names are not passed in, then WHERE does not exist.

Most fields are passed to Bugzilla::Bug::matched exactly as they are passed to Bug.search so the code above makes sure we passed in one or more of these. Examples, component, product, status, etc. which can be single values or list of values. 

We do not check for no params in Bugzilla::Bug::match and no params in Bugzilla::Object::match means get_all() so we have to check here.

dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (v3)

Mistake in my logic with previous patch. This one is better.

dkl

Comment 5

[Frédéric Buclin]

Comment on attachment 741644 [details] [diff] [review]
Patch to disallow empty search params and unlimited results for Bug.search (v3)

>=== modified file 'Bugzilla/WebService/Bug.pm'

>+    if (!$params->{WHERE}
>+        && !grep(!/(offset|limit|where)/i, keys %$params)

The first check is redundant as you are testing WHERE twice (case-insensitive in the regexp). One of them can go away.


>+C<int> Limit the number of results returned to C<int> records. The limit, if
>+more than zero, must be less than the maximum limit set by the administrator.

The user probably has no idea what the maximum limit is set to. Maybe you should reword it to tell that if the limit is higher than the maximum limit set by the administrator, then the maximum limit will be used instead.


>+=item In Bugzilla B<4.4>, added the ability to return all results if

This change will be applied to 4.2.6 as well. This comment must be very clear about this change.

r=LpSolit


Unrelated to this bug/patch: if I set limit to N, I may get less than N bugs if some of them are not visible by me, i.e. the SQL query first limits the list to N bugs, and then we exclude invisible bugs from the list which means the user gets less than N bugs. That's a pity.

Comment 6

[Frédéric Buclin]

The patch doesn't apply cleanly to the 4.2 branch. A backport is needed.

patching file Bugzilla/WebService/Bug.pm
Hunk #1 FAILED at 406.

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (trunk) (v4)

Moving forward r+ to this version which will be committed.

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (4.2) (v1)

Patch for 4.2. Moving forward r+. 

Since this bug is marked private, should I wait to commit these two patches and go ahead?

dkl

Comment 9

[Frédéric Buclin]

Comment on attachment 745347 [details] [diff] [review]
Patch to disallow empty search params and unlimited results for Bug.search (trunk) (v4)

>=== modified file 'Bugzilla/WebService/Bug.pm'

>+    if (!$params->{WHERE} && !grep($_ ne 'LIMIT', keys %$params)

This code is now wrong. You must also exclude offset from the list, else I can pass {limit => 1000000, offset => 0} and get the whole buglist despite I passed no search criteria.

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (trunk) (v5)

Fixed issue with not properly accounting for offset when determining no params.

dkl

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to disallow empty search params and unlimited results for Bug.search (4.2) (v2)

Comment 12

[Frédéric Buclin]

Comment on attachment 745363 [details] [diff] [review]
Patch to disallow empty search params and unlimited results for Bug.search (trunk) (v5)

>+    if (!$params->{WHERE} 

Nit: remove the trailing whitespace on checkin.

Comment 13

[Frédéric Buclin]

Comment on attachment 745366 [details] [diff] [review]
Patch to disallow empty search params and unlimited results for Bug.search (4.2) (v2)

>+    # If no other parameters have been passed other than limit and offset
>+    # and a WHERE parameter was not created earlier, then we throw error
>+    # if system is configured to do so.
>+    if (!$params->{WHERE} 
>+        && !grep(!/(limit|offset)/i, keys %$params)
>+        && !Bugzilla->params->{search_allow_no_criteria})
>+    {
>+        ThrowUserError('buglist_parameters_required');
>+    }
>+
>+    if (!$params->{WHERE} && !grep($_ ne 'LIMIT', keys %$params)
>+        && !Bugzilla->params->{search_allow_no_criteria})
>+    {
>+        ThrowUserError('buglist_parameters_required');
>+    }

Err... you are mixing two revisions of the patch here.

Comment 14

[David Lawrence [:dkl]]

Attachment: Patch to display prod/comp description in show_bug.cgi for 4.2 (v3)

Sigh. Having an off day with patches. Here is a better one.

dkl

Comment 15

[Frédéric Buclin]

Comment on attachment 745376 [details] [diff] [review]
Patch to display prod/comp description in show_bug.cgi for 4.2 (v3)

>+    if (!$params->{WHERE} 

Nit: remove the trailing whitespace.


>+=item In B<4.2> and newer, added the ability to return all results if

You should say 4.2.6 as we didn't enforce this restriction before.

Comment 16

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk    
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 8618.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4             
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 8551.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2             
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
Committed revision 8211.