[Bug 10590] New: in opac-topissues limit param is not protected
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Bug ID: 10590 Summary: in opac-topissues limit param is not protected Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: OPAC Assignee: oleonard@myacpl.org Reporter: fridolyn.somers@biblibre.com QA Contact: testopia@bugs.koha-community.org In opac-topissues page, the limit URL argument is directly added to SQL query. This argument should be protected. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|oleonard@myacpl.org |fridolyn.somers@biblibre.co | |m --- Comment #1 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 19646 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19646&action=edit Proposed patch See commit message -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #2 from Robin Sheat <robin@catalyst.net.nz> --- I'm unable to reproduce this, anything following ';' is stripped off. However, if you replace ; with %3B, then it gets through. However, adding e.g. '%3Btruncate+test%3B' to the end of limit does give me a query that finishes like: GROUP BY biblio.biblionumber HAVING tot >0 ORDER BY tot DESC LIMIT 15;truncate test; which isn't ideal. Running this gives me: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'truncate test' at line 14 at /mnt/catalyst/koha/opac/opac-topissues.pl line 117. and I don't fully know why. This said, this shouldn't be possible. I don't think your patch goes far enough though: the $limit should be replaced by a '?' as well as being filtered. I'll test your patch and make a followup with that. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #3 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 19659 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19659&action=edit Bug 10590 - in opac-topissues limit param is not protected In opac-topissues page, the limit URL argument is directly added to SQL query. This patch adds protections : limit must only contain digits and must be lower than 100. Test plan : - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=9999&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 100 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=WHERE&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time Signed-off-by: Robin Sheat <robin@catalyst.net.nz> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #4 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 19660 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19660&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. --- Comment #5 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- (In reply to Robin Sheat from comment #2) I did not took the time to hack the system with that but nevertheless it is dangerous to keep it as it is.
I don't think your patch goes far enough though: the $limit should be replaced by a '?' as well as being filtered You mean ending query with "limit ?" and using execute($limit) ? I thought it would not work because limit will be a string : "limit '10'".
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #4 from Robin Sheat <robin@catalyst.net.nz> --- Created attachment 19660 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19660&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #6 from Robin Sheat <robin@catalyst.net.nz> --- (In reply to Fridolyn SOMERS from comment #5)
(In reply to Robin Sheat from comment #2) I did not took the time to hack the system with that but nevertheless it is dangerous to keep it as it is.
It is.
I don't think your patch goes far enough though: the $limit should be replaced by a '?' as well as being filtered You mean ending query with "limit ?" and using execute($limit) ? I thought it would not work because limit will be a string : "limit '10'".
It works fine, SQL doesn't really care about the difference between strings and numbers when working with parameters. I'm marking this signed off as it's /vital/ that one of these patches goes in, it'd be best if they both did. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gmcharlt@gmail.com --- Comment #7 from Galen Charlton <gmcharlt@gmail.com> --- (In reply to Robin Sheat from comment #2)
Running this gives me: DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'truncate test' at line 14 at /mnt/catalyst/koha/opac/opac-topissues.pl line 117.
and I don't fully know why.
Looks like one might have to enable multiple statement execution support [1] to get the canonical exploit to work. [1] http://dev.mysql.com/doc/refman/5.0/en/c-api-multiple-queries.html -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #8 from Galen Charlton <gmcharlt@gmail.com> --- I'm testing and doing a second signoff now, after which I've enlisted Katrin's help to QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19646|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #9 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 19661 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19661&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #10 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 19662 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19662&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 19663 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19663&action=edit Bug 10590 - in opac-topissues limit param is not protected In opac-topissues page, the limit URL argument is directly added to SQL query. This patch adds protections : limit must only contain digits and must be lower than 100. Test plan : - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=9999&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 100 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=WHERE&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #12 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 19664 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19664&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19659|0 |1 is obsolete| | Attachment #19660|0 |1 is obsolete| | Attachment #19661|0 |1 is obsolete| | Attachment #19662|0 |1 is obsolete| | Attachment #19663|0 |1 is obsolete| | Attachment #19664|0 |1 is obsolete| | --- Comment #13 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 19665 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19665&action=edit Bug 10590 - in opac-topissues limit param is not protected In opac-topissues page, the limit URL argument is directly added to SQL query. This patch adds protections : limit must only contain digits and must be lower than 100. Test plan : - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=9999&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 100 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=WHERE&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #14 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 19666 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19666&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P1 - high -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19666|0 |1 is obsolete| | --- Comment #15 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 19667 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19667&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19665|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 19668 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19668&action=edit Bug 10590 - in opac-topissues limit param is not protected In opac-topissues page, the limit URL argument is directly added to SQL query. This patch adds protections : limit must only contain digits and must be lower than 100. Test plan : - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=9999&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 100 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=WHERE&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19667|0 |1 is obsolete| | --- Comment #17 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 19669 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19669&action=edit Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #18 from Jonathan Druart <jonathan.druart@biblibre.com> --- Fixes sound good to me. Marked as Passed QA. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19668|0 |1 is obsolete| | Attachment #19669|0 |1 is obsolete| | --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 19670 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19670&action=edit [PASSED QA] Bug 10590 - in opac-topissues limit param is not protected In opac-topissues page, the limit URL argument is directly added to SQL query. This patch adds protections : limit must only contain digits and must be lower than 100. Test plan : - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=10&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=9999&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 100 most cheched-out of all time - Edit URL to : /cgi-bin/koha/opac-topissues.pl?limit=WHERE&branch=&itemtype=&timeLimit=999&do_it=1 => You get the results of 10 most cheched-out of all time Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 19671 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19671&action=edit [PASSED QA] Bug 10590 - parameterise the limit option The limit option was previously substituted directly into the query. The previous patch on bug 10590 filters it on input, but there's no reason not to have it made to work properly in the query for added safety. To test: [1] Go to the top checkouts report (http://OPAC/cgi-bin/koha/opac-topissues.pl) [2] Run the report several times, varying the filters on number of results and item type. [3] Verify the the list of top checkouts appears to be correct. Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #21 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master. Thanks, everybody! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |tomascohen@gmail.com --- Comment #22 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This patch has been pushed to 3.12.x, will be in 3.12.2. Thanks Fridolyn and Robin! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bgkriegel@gmail.com --- Comment #23 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Pushed to 3.10.x, will be in 3.10.8 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10590 Chris Hall <chrish@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chrish@catalyst.net.nz --- Comment #24 from Chris Hall <chrish@catalyst.net.nz> --- Pushed to 3.8.x, will be in 3.8.15 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org