Bug 18854 - DoS Offset
Summary: DoS Offset
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: unspecified
Hardware: All All
: P1 - high critical (vote)
Assignee: Bugs List
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-24 21:14 UTC by Vincent
Modified: 2017-12-07 22:24 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18854 - Protect from DOS (1.81 KB, patch)
2017-06-25 05:37 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 18854 - Protect from DOS (1.85 KB, patch)
2017-06-25 08:22 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 18854 - Protect from DOS (1.99 KB, patch)
2017-06-26 06:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 18854: Make sure offset will not be < 0 - protect from DoS (1.74 KB, patch)
2017-07-05 13:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18854: Make sure offset will not be < 0 - protect from DoS (1.78 KB, patch)
2017-07-05 13:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 18854 - Protect from DOS (2.02 KB, patch)
2017-07-07 07:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18854: Make sure offset will not be < 0 - protect from DoS (1.85 KB, patch)
2017-07-07 07:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18854: Protect few other occurrences of offset (3.53 KB, patch)
2017-07-10 14:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18854: Make sure offset will not be < 0 - protect from DoS (1.94 KB, patch)
2017-07-14 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18854: Protect few other occurrences of offset (3.60 KB, patch)
2017-07-14 06:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent 2017-06-24 21:14:55 UTC
Hi,

yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-1
will result in an 
Error:
Can't call method "raw" on an undefined value at /usr/share/koha/lib/C4/Search.pm line 490.

yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-999999999999999999
will also result in an
Error:
Can't call method "raw" on an undefined value at /usr/share/koha/lib/C4/Search.pm line 490.

but try
yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
I added one 9, and you won't have a response. 
*Seems to crash, lock overloaded (have no idea) the Koha or DB process. For most website running Koha, it will crash it, if not, it's likely that either DB or Koha is using multithread. Then just run the payload for the number of thread your DB or Koh is using and you will down the website.

*I'm guessing with 0 knowledge of Koha, I just want to watch another Netflix TV-Show
and don't have the courage to look at the code, or to run a Koha instance. Forgive me. Kuddos. 
I tried that on some of the test websites there https://koha-community.org/demo/ so they might be down :/ sorry.
Comment 1 Vincent 2017-06-24 22:57:13 UTC
Ok, I was a bit scared to not see the koha instance I tried getting back up so I set up my own instance.
So the search.pl process will get locked at 100%, and also the apache2 process. What was a bit scary was that lot of data were written on my disk. 
So 30 sec after running the query I had a koha error file of 1.5GB 
filled with :
[Sun Jun 25 00:49:13.475921 2017] [cgi:error] [pid XXX] [client XXXXXXX] AH01215: [Sun Jun 25 00:49:13 2017] search.pl: ERROR DECODING RECORD - Empty String at /XXXXXX/XXXXXX/perl5/MARC/File/XML.pm line 448.: /XXXXXX/XXXXXX/koha/intranet/cgi-bin/catalogue/search.pl

A bit scary.
Comment 2 Chris Cormack 2017-06-24 23:44:48 UTC
Thank you for reporting this, we'll hopefully have a fix shortly.
Comment 3 Chris Cormack 2017-06-25 05:37:48 UTC
Created attachment 64611 [details] [review]
Bug 18854 - Protect from DOS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client
Comment 4 Mirko Tietgen 2017-06-25 08:22:40 UTC
Created attachment 64612 [details] [review]
Bug 18854 - Protect from DOS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>
Comment 5 Marcel de Rooy 2017-06-26 06:37:45 UTC
Created attachment 64617 [details] [review]
Bug 18854 - Protect from DOS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: changed -2 to 0 in opac-search.pl.
Comment 6 Fridolin SOMERS 2017-06-26 07:29:51 UTC
I think this bug should also apply on opac/opac-showreviews.pl $offset.
No ?
Comment 7 Chris Cormack 2017-06-26 07:31:41 UTC
If they use zebra then yes
Comment 8 Fridolin SOMERS 2017-06-26 07:37:19 UTC
(In reply to Fridolin SOMERS from comment #6)
> I think this bug should also apply on opac/opac-showreviews.pl $offset.
> No ?

In fact no, looks like it is already protected :

my $offset = $query->param('offset') || 0;
my $page = $query->param('page') || 1;
$offset = ($page-1)*$results_per_page if $page>1;

Sorry its all good.
Comment 9 Chris Cormack 2017-06-26 07:38:54 UTC
Hmm that could still end up negative, i think safest probably to do it there too.
Comment 10 Marcel de Rooy 2017-06-26 07:49:53 UTC
opac-showreviews generates internal server error on large negative offsets, no dos problems afaics
Comment 11 Vincent 2017-06-27 08:03:49 UTC
Hi guys,

would might be cool to write a small fuzzer for KOHA in order to detect this kind of stuff in many more, it would then be easy to integrate in the CI.

Would love to do it but right now don't have the time.
Comment 12 Jonathan Druart 2017-07-05 13:40:28 UTC
IMO this check must be done in Koha::SearchEngine::[Elasticsearch|Zebra]::Search::search_compat to cover all calls.
Comment 13 Jonathan Druart 2017-07-05 13:45:53 UTC
Created attachment 64799 [details] [review]
[ALTERNATIVE-PATCH] Bug 18854: Make sure offset will not be < 0 - protect from DoS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client
Comment 14 Jonathan Druart 2017-07-05 13:46:49 UTC
QA, please provide feedback very quickly.
Comment 15 Tomás Cohen Arazi 2017-07-05 13:49:58 UTC
(In reply to Jonathan Druart from comment #14)
> QA, please provide feedback very quickly.

I agree the test could be moved into Search.pm.
Comment 16 Tomás Cohen Arazi 2017-07-05 13:53:22 UTC
Created attachment 64800 [details] [review]
Bug 18854: Make sure offset will not be < 0 - protect from DoS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 17 Julian Maurice 2017-07-07 07:23:20 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > QA, please provide feedback very quickly.
> 
> I agree the test could be moved into Search.pm.

I think both patches are useful here, because $offset is also used at other places in catalogue/search.pl and opac/opac-search.pl
QAing this now
Comment 18 Julian Maurice 2017-07-07 07:59:24 UTC
Created attachment 64888 [details] [review]
Bug 18854 - Protect from DOS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: changed -2 to 0 in opac-search.pl.
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 19 Julian Maurice 2017-07-07 07:59:28 UTC
Created attachment 64889 [details] [review]
Bug 18854: Make sure offset will not be < 0 - protect from DoS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 20 Jonathan Druart 2017-07-10 14:57:35 UTC
Created attachment 64969 [details] [review]
Bug 18854: Protect few other occurrences of offset
Comment 21 Jonathan Druart 2017-07-10 14:58:12 UTC
Can I get a QA stamp on the last patch?
Comment 22 Marcel de Rooy 2017-07-14 06:43:48 UTC
Created attachment 65039 [details] [review]
Bug 18854: Make sure offset will not be < 0 - protect from DoS

There was a bug that meant a very large offset in the search params
will cause the search script to run forever (or long enough to crash
the machine)

To test

1/ Get ready with sudo top so you can kill the thread before it causes
your machine to OOM
2/ Hit a page like yourdomain.com/cgi-bin/koha/opac-search.pl?q=1&offset=-9999999999999999999
3/ Notice the process runs for a long time
4/ Kill the process
5/ Apply the patch
6/ Hit the page again, notice the it loads (offset is set to zero)
7/ Do the same to search in the staff client

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2017-07-14 06:43:53 UTC
Created attachment 65040 [details] [review]
Bug 18854: Protect few other occurrences of offset

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Jonathan Druart 2017-07-14 15:47:31 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 25 Fridolin SOMERS 2017-07-18 13:30:14 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 26 Katrin Fischer 2017-07-19 21:18:40 UTC
These patches have been pushed to 16.11.x and will be in 16.11.10.