Bug 25339

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.

Why is this in the security area?

(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.

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>

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.

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.

(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.

+    $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 ?

(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.)

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.