Bug 28784 - DoS in opac-search.pl causes OOM situation and 100% CPU (doesn't require login!)
Summary: DoS in opac-search.pl causes OOM situation and 100% CPU (doesn't require login!)
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 13307
Blocks: 28821
  Show dependency treegraph
 
Reported: 2021-07-30 09:29 UTC by Joonas Kylmälä
Modified: 2021-08-26 08:08 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03,20.11.09,20.05.15,19.11.21


Attachments
Bug 28784: Remove code related to num_paragraph cookie (4.58 KB, patch)
2021-08-03 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28784: Remove code related to num_paragraph cookie (4.91 KB, patch)
2021-08-03 09:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28784: Remove code related to num_paragraph cookie (4.95 KB, patch)
2021-08-03 11:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28784: Remove code related to num_paragraph cookie (5.01 KB, patch)
2021-08-03 12:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28784: (follow-up) Always make three search boxes (1.95 KB, patch)
2021-08-03 12:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28784: (follow-up) Always make three search boxes (2.00 KB, patch)
2021-08-03 12:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28784: Remove code related to num_paragraph cookie (5.06 KB, patch)
2021-08-04 22:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28784: (follow-up) Always make three search boxes (2.06 KB, patch)
2021-08-04 22:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28784: Remove code related to num_paragraph cookie (5.16 KB, patch)
2021-08-06 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28784: (follow-up) Always make three search boxes (2.15 KB, patch)
2021-08-06 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28784: (QA follow-up) Remove num_paragraph cookie from include (1.16 KB, patch)
2021-08-06 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28784: (follow-up) Improve operation of button plus/less (4.19 KB, patch)
2021-08-06 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28784: [20.05.x] (follow-up) Always make three search boxes (1.62 KB, patch)
2021-08-17 07:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28784: [20.05.x] (follow-up) Always make three search boxes (1.94 KB, patch)
2021-08-17 07:59 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-07-30 09:29:39 UTC
WARNING: reproducing this bug makes server/computer UNRESPONSIVE.

To reproduce the Denial of Service go to

http://localhost:8080/cgi-bin/koha/opac-search.pl?advsearch=1&sort_by=relevance&do=Search

and using browser's developer console set a new cookie:

num_paragraph: 1000000000

After setting the cookie refresh the page and wait for the server to freeze.
Comment 1 Jonathan Druart 2021-08-03 08:58:45 UTC
I don't understand the point of this cookie.

        var numPar = $("#booleansearch fieldset p").size();
        if (numPar > [% search_boxes_count | html %]){
            jQuery.cookie("num_paragraph", numPar,{ path: '/'});
        }else{
            jQuery.removeCookie("num_paragraph", { path: '/'});
        }

But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements.

I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before.

From this comment:
    # determine what to display next to the search boxes (ie, boolean option
    # shouldn't appear on the first one, scan indexes should, adding a new
    # box should only appear on the last, etc.

The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug.
Comment 2 Jonathan Druart 2021-08-03 09:01:34 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2021-08-03 09:52:56 UTC
Created attachment 123409 [details] [review]
Bug 28784: Remove code related to num_paragraph cookie

It could lead to server freeze if set to a big value (we are pushing
into an array and so RAM is being fulfilled, and CPU is looping).

I don't understand the point of this cookie.

        var numPar = $("#booleansearch fieldset p").size();
        if (numPar > [% search_boxes_count | html %]){
            jQuery.cookie("num_paragraph", numPar,{ path: '/'});
        }else{
            jQuery.removeCookie("num_paragraph", { path: '/'});
        }

But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div' elements.

I've removed the code related to num_paragraph and the "Return to the last advanced search" feature still works as before.

From this comment:
    # determine what to display next to the search boxes (ie, boolean option
    # shouldn't appear on the first one, scan indexes should, adding a new
    # box should only appear on the last, etc.

The only bit that is not working as described is "adding a new box should only appear on the last", but it has been working this way for a long time already I think, and I don't see it as a bug.

Test plan:
Read the code, check that the above is correct.
Search for regression in this "return to last adv search" feature added
by bug 13307.
Comment 4 Owen Leonard 2021-08-03 11:50:57 UTC
Created attachment 123412 [details] [review]
Bug 28784: Remove code related to num_paragraph cookie

It could lead to server freeze if set to a big value (we are pushing
into an array and so RAM is being fulfilled, and CPU is looping).

I don't understand the point of this cookie.

        var numPar = $("#booleansearch fieldset p").size();
        if (numPar > [% search_boxes_count | html %]){
            jQuery.cookie("num_paragraph", numPar,{ path: '/'});
        }else{
            jQuery.removeCookie("num_paragraph", { path: '/'});
        }

But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div'
elements.

I've removed the code related to num_paragraph and the "Return to the
last advanced search" feature still works as before.

From this comment:
    # determine what to display next to the search boxes (ie, boolean option
    # shouldn't appear on the first one, scan indexes should, adding a new
    # box should only appear on the last, etc.

The only bit that is not working as described is "adding a new box
should only appear on the last", but it has been working this way for
a long time already I think, and I don't see it as a bug.

Test plan:
Read the code, check that the above is correct.
Search for regression in this "return to last adv search" feature added
by bug 13307.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Nick Clemens 2021-08-03 12:20:13 UTC
Created attachment 123413 [details] [review]
Bug 28784: Remove code related to num_paragraph cookie

It could lead to server freeze if set to a big value (we are pushing
into an array and so RAM is being fulfilled, and CPU is looping).

I don't understand the point of this cookie.

        var numPar = $("#booleansearch fieldset p").size();
        if (numPar > [% search_boxes_count | html %]){
            jQuery.cookie("num_paragraph", numPar,{ path: '/'});
        }else{
            jQuery.removeCookie("num_paragraph", { path: '/'});
        }

But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div'
elements.

I've removed the code related to num_paragraph and the "Return to the
last advanced search" feature still works as before.

From this comment:
    # determine what to display next to the search boxes (ie, boolean option
    # shouldn't appear on the first one, scan indexes should, adding a new
    # box should only appear on the last, etc.

The only bit that is not working as described is "adding a new box
should only appear on the last", but it has been working this way for
a long time already I think, and I don't see it as a bug.

Test plan:
Read the code, check that the above is correct.
Search for regression in this "return to last adv search" feature added
by bug 13307.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2021-08-03 12:20:17 UTC
Created attachment 123414 [details] [review]
Bug 28784: (follow-up) Always make three search boxes

The previous patch removed search_boxes_loop - that's okay, it was always
getting the same three values.

If we don't do something in the template though, we get no boxes

Ultimately this should be a include, and not a hardcoded loop, but keeping changes
small for backporting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Owen Leonard 2021-08-03 12:36:13 UTC
Created attachment 123416 [details] [review]
Bug 28784: (follow-up) Always make three search boxes

The previous patch removed search_boxes_loop - that's okay, it was always
getting the same three values.

If we don't do something in the template though, we get no boxes

Ultimately this should be a include, and not a hardcoded loop, but keeping changes
small for backporting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Tomás Cohen Arazi 2021-08-04 22:16:39 UTC
Created attachment 123467 [details] [review]
Bug 28784: Remove code related to num_paragraph cookie

It could lead to server freeze if set to a big value (we are pushing
into an array and so RAM is being fulfilled, and CPU is looping).

I don't understand the point of this cookie.

        var numPar = $("#booleansearch fieldset p").size();
        if (numPar > [% search_boxes_count | html %]){
            jQuery.cookie("num_paragraph", numPar,{ path: '/'});
        }else{
            jQuery.removeCookie("num_paragraph", { path: '/'});
        }

But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div'
elements.

I've removed the code related to num_paragraph and the "Return to the
last advanced search" feature still works as before.

From this comment:
    # determine what to display next to the search boxes (ie, boolean option
    # shouldn't appear on the first one, scan indexes should, adding a new
    # box should only appear on the last, etc.

The only bit that is not working as described is "adding a new box
should only appear on the last", but it has been working this way for
a long time already I think, and I don't see it as a bug.

Test plan:
Read the code, check that the above is correct.
Search for regression in this "return to last adv search" feature added
by bug 13307.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2021-08-04 22:16:45 UTC
Created attachment 123468 [details] [review]
Bug 28784: (follow-up) Always make three search boxes

The previous patch removed search_boxes_loop - that's okay, it was always
getting the same three values.

If we don't do something in the template though, we get no boxes

Ultimately this should be a include, and not a hardcoded loop, but keeping changes
small for backporting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Marcel de Rooy 2021-08-06 06:47:05 UTC
git grep num_para
koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-bottom.inc:            jQuery.removeCookie("num_paragraph", { path: '/'});
Comment 11 Marcel de Rooy 2021-08-06 06:47:46 UTC
Just looking a little bit more to the +/- buttons ..
Comment 12 Marcel de Rooy 2021-08-06 08:06:27 UTC
Created attachment 123558 [details] [review]
Bug 28784: Remove code related to num_paragraph cookie

It could lead to server freeze if set to a big value (we are pushing
into an array and so RAM is being fulfilled, and CPU is looping).

I don't understand the point of this cookie.

        var numPar = $("#booleansearch fieldset p").size();
        if (numPar > [% search_boxes_count | html %]){
            jQuery.cookie("num_paragraph", numPar,{ path: '/'});
        }else{
            jQuery.removeCookie("num_paragraph", { path: '/'});
        }

But "#booleansearch fieldset p" does not exist, it's not 'p' but 'div'
elements.

I've removed the code related to num_paragraph and the "Return to the
last advanced search" feature still works as before.

From this comment:
    # determine what to display next to the search boxes (ie, boolean option
    # shouldn't appear on the first one, scan indexes should, adding a new
    # box should only appear on the last, etc.

The only bit that is not working as described is "adding a new box
should only appear on the last", but it has been working this way for
a long time already I think, and I don't see it as a bug.

Test plan:
Read the code, check that the above is correct.
Search for regression in this "return to last adv search" feature added
by bug 13307.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2021-08-06 08:06:32 UTC
Created attachment 123559 [details] [review]
Bug 28784: (follow-up) Always make three search boxes

The previous patch removed search_boxes_loop - that's okay, it was always
getting the same three values.

If we don't do something in the template though, we get no boxes

Ultimately this should be a include, and not a hardcoded loop, but keeping changes
small for backporting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2021-08-06 08:06:37 UTC
Created attachment 123560 [details] [review]
Bug 28784: (QA follow-up) Remove num_paragraph cookie from include

Still found in opac-bottom.inc.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2021-08-06 08:06:42 UTC
Created attachment 123561 [details] [review]
Bug 28784: (follow-up) Improve operation of button plus/less

Do not remove the op select on the first row.
Allow plus/less on all lines. Allow less until we have one line left.
Make sure that first op is disabled and hidden with plus/less.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2021-08-06 08:07:10 UTC
Last follow-up might go on its own?
Comment 17 Jonathan Druart 2021-08-06 08:49:29 UTC
(In reply to Marcel de Rooy from comment #16)
> Last follow-up might go on its own?

Yes please, we will have to backport and publish security releases. The fewer changes the better :)
Comment 18 Marcel de Rooy 2021-08-06 09:07:36 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Marcel de Rooy from comment #16)
> > Last follow-up might go on its own?
> 
> Yes please, we will have to backport and publish security releases. The
> fewer changes the better :)
Comment 19 Marcel de Rooy 2021-08-06 09:11:02 UTC
Comment on attachment 123561 [details] [review]
Bug 28784: (follow-up) Improve operation of button plus/less

Follow-up moved to bug 28821
Comment 20 Victor Grousset/tuxayo 2021-08-12 19:27:35 UTC
Rebase needed for 20.05.xx

> Patch failed at 0001 Bug 28784: (follow-up) Always make three search boxes
Comment 21 Jonathan Druart 2021-08-16 10:40:59 UTC
(In reply to Victor Grousset/tuxayo from comment #20)
> Rebase needed for 20.05.xx
> 
> > Patch failed at 0001 Bug 28784: (follow-up) Always make three search boxes

I guess you need to keep this and drop the second chunk:

-                            [% FOREACH search_box IN search_boxes_loop %]
+                            [% FOREACH search_box IN [ 1, 2, 3] %]
Comment 22 Victor Grousset/tuxayo 2021-08-16 18:37:27 UTC
What is a clean way to do this?

Git doesn't even apply and conflict.

So my plan is to download the patch file, and edit it directly with git am.
Comment 23 wainuiwitikapark 2021-08-17 01:38:33 UTC
This doesn't apply cleanly to 19.11.x
Comment 24 Victor Grousset/tuxayo 2021-08-17 03:37:42 UTC
Somehow now git managed to apply & conflict. So I'll be able to try to solve.
Maybe I got lucky and my last backports brought a missing piece.

Clocking off, will continue tomorrow.

(note: don't forget the remove the test cookie, otherwise it will blow up latter when not being ready to kill the process...)
Comment 25 Jonathan Druart 2021-08-17 07:41:44 UTC
Created attachment 123866 [details] [review]
Bug 28784: [20.05.x] (follow-up) Always make three search boxes

The previous patch removed search_boxes_loop - that's okay, it was always
getting the same three values.

If we don't do something in the template though, we get no boxes

Ultimately this should be a include, and not a hardcoded loop, but keeping changes
small for backporting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Jonathan Druart 2021-08-17 07:59:50 UTC
Created attachment 123867 [details] [review]
Bug 28784: [20.05.x] (follow-up) Always make three search boxes

The previous patch removed search_boxes_loop - that's okay, it was always
getting the same three values.

If we don't do something in the template though, we get no boxes

Ultimately this should be a include, and not a hardcoded loop, but keeping changes
small for backporting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Jonathan Druart 2021-08-17 08:00:52 UTC
For 20.05.x

Bug 28784: (QA follow-up) Remove num_paragraph cookie from include
Bug 28784: [20.05.x] (follow-up) Always make three search boxes
Bug 28784: Remove code related to num_paragraph cookie

And you also need:
Bug 28518: Display missing inputs for "Return to the last advanced search"
Comment 28 Jonathan Druart 2021-08-17 08:06:58 UTC
(In reply to Jonathan Druart from comment #27)
> For 20.05.x
> 
> Bug 28784: (QA follow-up) Remove num_paragraph cookie from include
> Bug 28784: [20.05.x] (follow-up) Always make three search boxes
> Bug 28784: Remove code related to num_paragraph cookie
> 
> And you also need:
> Bug 28518: Display missing inputs for "Return to the last advanced search"

Same for 19.11.x
Comment 29 wainuiwitikapark 2021-08-18 03:04:18 UTC
On 19.11.x after doing git bz apply 28784 I get:

Bug 28784 - DoS in opac-search.pl causes OOM situation and 100% CPU (doesn't require login!)

123558 - Bug 28784: Remove code related to num_paragraph cookie
123559 - Bug 28784: (follow-up) Always make three search boxes
123560 - Bug 28784: (QA follow-up) Remove num_paragraph cookie from include
123867 - Bug 28784: [20.05.x] (follow-up) Always make three search boxes

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28784: Remove code related to num_paragraph cookie
Applying: Bug 28784: (follow-up) Always make three search boxes
error: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-advsearch.tt).
error: could not build fake ancestor
Patch failed at 0001 Bug 28784: (follow-up) Always make three search boxes
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-28784-follow-up-Always-make-three-search-boxes-1CY3s7.patch
Comment 30 Jonathan Druart 2021-08-18 12:41:33 UTC
(In reply to wainuiwitikapark from comment #29)
> On 19.11.x after doing git bz apply 28784 I get:
> 
> Bug 28784 - DoS in opac-search.pl causes OOM situation and 100% CPU (doesn't
> require login!)
> 
> 123558 - Bug 28784: Remove code related to num_paragraph cookie
> 123559 - Bug 28784: (follow-up) Always make three search boxes
> 123560 - Bug 28784: (QA follow-up) Remove num_paragraph cookie from include
> 123867 - Bug 28784: [20.05.x] (follow-up) Always make three search boxes

See comment 27, you don't need the second patch.
Comment 31 Victor Grousset/tuxayo 2021-08-18 22:41:11 UTC
Thanks Jonathan, it works. Pushed on the security repo on branch 20.05.x-security
Comment 32 Victor Grousset/tuxayo 2021-08-18 23:35:21 UTC
The question came about whether or not should we do a synced release. (non-urgent, just synced)

So, does this bug warrants it?
Comment 33 wainuiwitikapark 2021-08-19 02:26:49 UTC
(In reply to Victor Grousset/tuxayo from comment #31)
> Thanks Jonathan, it works. Pushed on the security repo on branch
> 20.05.x-security

I have now also pushed to security repo on 19.11.x-security branch
Comment 34 Fridolin Somers 2021-08-25 02:50:31 UTC
Pushed to 20.11.x for 20.11.09
Comment 35 Jonathan Druart 2021-08-25 13:00:17 UTC
Pushed to 21.05.x for 21.05.03

Pushed to 20.11.x for 20.11.09

Pushed to 20.05.x for 20.05.15

Pushed to 19.11.x for 19.11.21
Comment 36 Jonathan Druart 2021-08-26 08:08:25 UTC
Pushed to master for 21.11.00.