Bug 25339 - Validate biblionumber on opac-basket.pl and opac-review.pl
Summary: Validate biblionumber on opac-basket.pl and opac-review.pl
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-01 05:19 UTC by David Cook
Modified: 2024-07-03 12:47 UTC (History)
8 users (show)

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


Attachments
Bug 25339: Validate biblionumber on opac-basket.pl and opac-review.pl (1.72 KB, patch)
2020-05-01 05:25 UTC, David Cook
Details | Diff | Splinter Review
Bug 25339: Validate biblionumber on opac-basket.pl and opac-review.pl (1.77 KB, patch)
2020-05-13 16:36 UTC, David Roberts
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-05-01 05:19:28 UTC
It's possible to inject bad data into the biblionumber parameter and for that data to appear (albeit html encoded) in the HTML page.

The template toolkit html filter prevents this being a real XSS vulnerability, but doesn't hurt to shore this up.
Comment 1 David Cook 2020-05-01 05:25:48 UTC
Created attachment 104064 [details] [review]
Bug 25339: Validate biblionumber on opac-basket.pl and opac-review.pl

This patch runs biblionumber through the int() function before
using it in the application.

Test Plan:
1) Apply patch
2) Go to /cgi-bin/koha/opac-basket.pl?bib_list=1/abcdefghi'-alert(1)-'j&verbose=1
3) Note that you can't find abcdefghi in the HTML. You only see biblionumber 1.
4) Go to /cgi-bin/koha/opac-review.pl?biblionumber=1'-alert(1)-'abcdefghi&reviewid=1&review=
5) Note that you can't find abcdefghi in the HTML. You only see biblionumber 1.
Comment 2 Jonathan Druart 2020-05-01 10:08:38 UTC
Why is this in the security area?
Comment 3 David Cook 2020-05-03 23:08:24 UTC
(In reply to Jonathan Druart from comment #2)
> Why is this in the security area?

Yeah, it's a good question. I debated where to put it. It can't really be exploited, but it does show a weak point in Koha.
Comment 4 David Roberts 2020-05-13 16:36:29 UTC
Created attachment 104850 [details] [review]
Bug 25339: Validate biblionumber on opac-basket.pl and opac-review.pl

This patch runs biblionumber through the int() function before
using it in the application.

Test Plan:
1) Apply patch
2) Go to /cgi-bin/koha/opac-basket.pl?bib_list=1/abcdefghi'-alert(1)-'j&verbose=1
3) Note that you can't find abcdefghi in the HTML. You only see biblionumber 1.
4) Go to /cgi-bin/koha/opac-review.pl?biblionumber=1'-alert(1)-'abcdefghi&reviewid=1&review=
5) Note that you can't find abcdefghi in the HTML. You only see biblionumber 1.

Signed-off-by: David Roberts <david@koha-ptfs.co.uk>
Comment 5 David Roberts 2020-05-13 16:37:57 UTC
Signed off, although I couldn't see the string in /cgi-bin/koha/opac-review.pl?biblionumber=1'-alert(1)-'abcdefghi&reviewid=1&review= even before the patch was applied. The first URL is certainly fixed.
Comment 6 Jonathan Druart 2020-05-14 11:07:30 UTC
Why did you pick those 2 scripts? And why only biblionumber?

If you want to fix this problem (it is not really an issue imo), you should do it all over the place, where we retrieve a variable that is supposed to be an id and we send it back to the template. I am pretty sure there are others.

That being said, I don't see biblionumber passed to the template from opac-review.pl.
Comment 7 David Cook 2020-05-14 23:17:59 UTC
(In reply to Jonathan Druart from comment #6)
> Why did you pick those 2 scripts? And why only biblionumber?
> 

Wise questions. They were flagged by a security consultant, so I fixed them locally, and posted the patches here to be upstreamed.

> If you want to fix this problem (it is not really an issue imo), you should
> do it all over the place, where we retrieve a variable that is supposed to
> be an id and we send it back to the template. I am pretty sure there are
> others.
> 

As you say, it's not really a (critical) issue, as we've got the XSS risk handled by the templates. Imho, it's just a bit embarrassing that we don't validate the data more - but not problematic per se. 

That said, I don't see an issue with patching it in some places and not all places. (After all, previous efforts by Jared seem to have only patched it in some places and not all places.)

I agree that there are certainly other places where this happens too. 

> That being said, I don't see biblionumber passed to the template from
> opac-review.pl.

Good catch! The reason is actually rather amusing. It appears Bug 25340 (a bug I reported the same day as this one but Owen fixed) removed the biblionumber issue from opac-review.pl as well.
Comment 8 Marcel de Rooy 2020-05-15 09:59:34 UTC
+    $biblionumber = int($biblionumber);

I think we have a few places where we do that. And it is really ugly imo.
If you dont pass a valid biblionumber, you should not get a valid response.

Furthermore int does not like strings. We will have warnings like:
Argument "3abc" isn't numeric in int 

I would rather get rid of the other places..

Changing status: ID or FQA ?
Comment 9 David Cook 2020-05-17 23:17:14 UTC
(In reply to Marcel de Rooy from comment #8)
> +    $biblionumber = int($biblionumber);
> 
> I think we have a few places where we do that. And it is really ugly imo.
> If you dont pass a valid biblionumber, you should not get a valid response.
> 
> Furthermore int does not like strings. We will have warnings like:
> Argument "3abc" isn't numeric in int 
> 
> I would rather get rid of the other places..

Yeah, that's a good point. 

> 
> Changing status: ID or FQA ?

Probably FQA for now, and maybe we should have a separate bug for returning a 404 or something when an invalid biblionumber is given. (And then this bug report would either become a duplicate or implement that new strategy.)
Comment 10 David Cook 2024-06-06 23:34:44 UTC
Sorry, Owen. I reset the Assignee a little while ago, as it seemed we'd hit a dead-end here and I didn't want folk thinking I was still working on it.