Description
Vincent
2017-06-24 21:14:55 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. Thank you for reporting this, we'll hopefully have a fix shortly. 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 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> 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. I think this bug should also apply on opac/opac-showreviews.pl $offset. No ? If they use zebra then yes (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. Hmm that could still end up negative, i think safest probably to do it there too. opac-showreviews generates internal server error on large negative offsets, no dos problems afaics 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. IMO this check must be done in Koha::SearchEngine::[Elasticsearch|Zebra]::Search::search_compat to cover all calls. 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 QA, please provide feedback very quickly. (In reply to Jonathan Druart from comment #14) > QA, please provide feedback very quickly. I agree the test could be moved into Search.pm. 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> (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 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> 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> Created attachment 64969 [details] [review] Bug 18854: Protect few other occurrences of offset Can I get a QA stamp on the last patch? 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> Created attachment 65040 [details] [review] Bug 18854: Protect few other occurrences of offset Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 17.11, thanks to everybody involved! Pushed to 17.05.x, will be in 17.05.02 These patches have been pushed to 16.11.x and will be in 16.11.10. |