Bug 10590 - in opac-topissues limit param is not protected
Summary: in opac-topissues limit param is not protected
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P1 - high critical (vote)
Assignee: Fridolin Somers
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-15 10:59 UTC by Fridolin Somers
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (1.72 KB, patch)
2013-07-15 11:05 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 10590 - in opac-topissues limit param is not protected (1.76 KB, patch)
2013-07-15 14:03 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.44 KB, patch)
2013-07-15 14:09 UTC, Robin Sheat
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.74 KB, patch)
2013-07-15 14:17 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.74 KB, patch)
2013-07-15 14:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10590 - in opac-topissues limit param is not protected (1.81 KB, patch)
2013-07-15 14:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.74 KB, patch)
2013-07-15 14:18 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10590 - in opac-topissues limit param is not protected (1.81 KB, patch)
2013-07-15 14:19 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.74 KB, patch)
2013-07-15 14:19 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.80 KB, patch)
2013-07-15 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10590 - in opac-topissues limit param is not protected (1.88 KB, patch)
2013-07-15 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10590 - parameterise the limit option (1.80 KB, patch)
2013-07-15 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10590 - in opac-topissues limit param is not protected (1.94 KB, patch)
2013-07-15 14:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10590 - parameterise the limit option (1.86 KB, patch)
2013-07-15 14:57 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2013-07-15 10:59:47 UTC
In opac-topissues page, the limit URL argument is directly added to SQL query.
This argument should be protected.
Comment 1 Fridolin Somers 2013-07-15 11:05:26 UTC Comment hidden (obsolete)
Comment 2 Robin Sheat 2013-07-15 13:59:15 UTC
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.
Comment 3 Robin Sheat 2013-07-15 14:03:32 UTC Comment hidden (obsolete)
Comment 4 Robin Sheat 2013-07-15 14:09:10 UTC Comment hidden (obsolete)
Comment 5 Fridolin Somers 2013-07-15 14:09:10 UTC
(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'".
Comment 6 Robin Sheat 2013-07-15 14:11:20 UTC
(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.
Comment 7 Galen Charlton 2013-07-15 14:11:46 UTC
(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
Comment 8 Galen Charlton 2013-07-15 14:12:24 UTC
I'm testing and doing a second signoff now, after which I've enlisted Katrin's help to QA.
Comment 9 Galen Charlton 2013-07-15 14:17:46 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2013-07-15 14:18:04 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-07-15 14:18:23 UTC Comment hidden (obsolete)
Comment 12 Galen Charlton 2013-07-15 14:18:34 UTC Comment hidden (obsolete)
Comment 13 Galen Charlton 2013-07-15 14:19:01 UTC Comment hidden (obsolete)
Comment 14 Galen Charlton 2013-07-15 14:19:18 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-07-15 14:30:53 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-07-15 14:31:13 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-07-15 14:31:20 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2013-07-15 14:32:53 UTC
Fixes sound good to me. Marked as Passed QA.
Comment 19 Katrin Fischer 2013-07-15 14:55:51 UTC
Created attachment 19670 [details] [review]
[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>
Comment 20 Katrin Fischer 2013-07-15 14:57:14 UTC
Created attachment 19671 [details] [review]
[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>
Comment 21 Galen Charlton 2013-07-15 15:19:16 UTC
Pushed to master.  Thanks, everybody!
Comment 22 Tomás Cohen Arazi 2013-07-15 16:43:58 UTC
This patch has been pushed to 3.12.x, will be in 3.12.2.

Thanks Fridolyn and Robin!
Comment 23 Bernardo Gonzalez Kriegel 2013-07-16 11:45:52 UTC
Pushed to 3.10.x, will be in 3.10.8
Comment 24 Chris Hall 2013-07-18 08:59:10 UTC
Pushed to 3.8.x, will be in 3.8.15