Bug 19319

Summary: Reflected XSS Vulnerability in opac-MARCdetail.pl
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: chris, fridolin.somers, jonathan.druart, m.de.rooy, mtompset, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 4319    
Attachments: Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl
Bug 19319: (QA follow-up) Biblionumber leftovers in opac-detail.tt
Bug 19319: Reflected XSS Vulnerability in opac-MARCdetail.pl
Bug 19319: (QA follow-up) Biblionumber leftovers in opac-detail.tt
Bug 19319: Only fetch the record if it exists

Description Kyle M Hall 2017-09-14 15:51:51 UTC
Try go to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
Comment 1 Kyle M Hall 2017-09-14 15:53:24 UTC
Created attachment 67140 [details] [review]
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl

Try going to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>

Test Plan:
1) Go to /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
2) Note <TEST> is embedded all over the html
3) Apply this patch
4) Refresh the page, not the injection is gone!
Comment 2 Jonathan Druart 2017-09-26 18:10:38 UTC
Kyle, opac-MARCdetail.tt includes opac-detail-sidebar.inc, which use the biblionumber variable.

Moreover there is a typo:
  -<title>[...]
  +:title>[...]
Comment 3 Kyle M Hall 2017-10-24 16:43:21 UTC
Created attachment 68482 [details] [review]
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl

Try going to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>

Test Plan:
1) Go to /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
2) Note <TEST> is embedded all over the html
3) Apply this patch
4) Refresh the page, not the injection is gone!
Comment 4 Mark Tompsett 2017-10-24 17:06:50 UTC
While testing the href="#" items on the page confused me for a bit. However, I don't believe that actually does leak anything. Code reads well enough.
Comment 5 Mark Tompsett 2017-10-24 17:09:04 UTC
Created attachment 68486 [details] [review]
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl

Try going to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>

Test Plan:
1) Go to /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
2) Note <TEST> is embedded all over the html
3) Apply this patch
4) Refresh the page, note the injection is gone!
5) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Marcel de Rooy 2017-10-27 10:25:00 UTC
Sidenote: opac-detail.pl contains two use Koha::Biblios;
Comment 7 Marcel de Rooy 2017-10-27 10:30:26 UTC
Please explain why we should replace every biblionumber | html by biblio.id. I am not seeing trouble all over the place in opac-detail for instance (but there are a few ones).
In what cases is biblionumber | html not sufficient? This might have more impact than these changes?
Comment 8 Marcel de Rooy 2017-11-03 06:56:00 UTC
Feedback please ^^
Comment 9 Mark Tompsett 2017-11-03 14:07:40 UTC
(In reply to Marcel de Rooy from comment #6)
> Sidenote: opac-detail.pl contains two use Koha::Biblios;

See bug 19576. :)
Comment 10 Kyle M Hall 2017-11-13 18:31:52 UTC
(In reply to Marcel de Rooy from comment #7)
> Please explain why we should replace every biblionumber | html by biblio.id.
> I am not seeing trouble all over the place in opac-detail for instance (but
> there are a few ones).
> In what cases is biblionumber | html not sufficient? This might have more
> impact than these changes?

It's pretty simple, the value in biblionumber is being passed in via cgi, and we are outputting that same variable in the template. By using biblio.id we know we are outputting the real biblionumber, and we don't have to spend CPU cycles escaping it so it improves performance as well ( by pico-seconds I'm sure, but every bit helps )!
Comment 11 Marcel de Rooy 2017-11-17 10:36:02 UTC
(In reply to Kyle M Hall from comment #10)
> It's pretty simple, the value in biblionumber is being passed in via cgi,
> and we are outputting that same variable in the template. By using biblio.id
> we know we are outputting the real biblionumber, and we don't have to spend
> CPU cycles escaping it so it improves performance as well ( by pico-seconds
> I'm sure, but every bit helps )!

About performance:

Without patch (passing a normal biblionumber)
opac-detail 1039ms 1019 1057 1053 1009 1081 1128 1115 1044
opac-MARCdetail 342 362 341 330 389 345 398
opac-ISBDdetail 309 284 254 362 269 312 314

With patch (passing a normal biblionumber)
opac-detail 1015ms 1064 1063 1154 1103 1083 1039
opac-MARCdetail 398ms 379 285 414 420 304 327 326
opac-ISBDdetail 296ms 296 279 348 398 317

Hard to say if we gain performance here. 
ISBD-detail got an extra Koha::Biblios call. And opac-detail now always make the call instead of conditional.
If we pollute the passed biblionumber in the URL, we may win a tiny bit more from not escaping, but it will be the exception.
On the other hand, these scripts will need the call anyway some time. So it may not really matter.
Comment 12 Marcel de Rooy 2017-11-17 10:58:14 UTC
The proposed patch will fix the problem, but introduces biblio.id in a few important opac templates on a larger scale.
Git grepping on this construction before this patch only gives:
koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt:                    <h1>Request article from <a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.id %]">[% biblio.title | html %]</a></h1>
koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt:                                <input type="hidden" name="biblionumber" value="[% biblio.id %]" />
t/db_dependent/Letters/TemplateToolkit.t:$sth->execute( "TEST_BIBLIO", "[% biblio.id %]" );
So: request-article.tt and a test

I am hesitating to pass qa on this change. I would rather remove all |htmls from biblionumber in the template but keep biblionumber as a template var and perhaps add $template->param(biblionumber=>$biblio.id) in the script.
Or even leave the script alone, remove the |htmls and add [% SET biblionumber = biblionumber | html ] as a one time call in one of the main include files (doc-head-open?) or add a plugin that strips the html completely (HTML::Strip, parse) or picks the number with a regex or so?

Asking additional feedback from Jonathan or Tomas. What do you think?
Comment 13 Marcel de Rooy 2017-11-24 07:14:42 UTC
Moving to In Discussion
Comment 14 Jonathan Druart 2017-12-04 18:46:58 UTC
I'd just replace biblio.id with biblio.biblionumber
Comment 15 Kyle M Hall 2017-12-05 16:18:47 UTC
Created attachment 69513 [details] [review]
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl

Try going to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>

Test Plan:
1) Go to /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
2) Note <TEST> is embedded all over the html
3) Apply this patch
4) Refresh the page, note the injection is gone!
5) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 16 Marcel de Rooy 2017-12-08 08:44:01 UTC
QA: Working here now
Comment 17 Marcel de Rooy 2017-12-08 09:25:12 UTC
Created attachment 69634 [details] [review]
Bug 19319 - Reflected XSS Vulnerability in opac-MARCdetail.pl

Try going to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>

Test Plan:
1) Go to /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
2) Note <TEST> is embedded all over the html
3) Apply this patch
4) Refresh the page, note the injection is gone!
5) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-12-08 09:25:17 UTC
Created attachment 69635 [details] [review]
Bug 19319: (QA follow-up) Biblionumber leftovers in opac-detail.tt

Correcting a few biblionumber template var leftovers in opac-detail
template. They were combined with url filter instead of html.

Test plan:
[1] Look for [^.;?]biblionumber in opac-detail.tt and verify that these
    occurrences are fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2017-12-08 09:26:07 UTC
Note to RM

FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
   FAIL   forbidden patterns
                forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 415)

I have not corrected this line by changing class btn-small to 'btn-default btn-sm', because it negatively affects page styling (text box and Add button will become vertically misaligned/unaligned). And git grep -l btn-small reveals a few other occurrences like opac-results, opac-shelves, opac-user and opac-illrequests.
This design problem should be handled on another report imo.

Passed QA
Comment 20 Jonathan Druart 2017-12-08 18:03:50 UTC
Created attachment 69656 [details] [review]
Bug 19319: Reflected XSS Vulnerability in opac-MARCdetail.pl

Try going to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>

Test Plan:
1) Go to /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
2) Note <TEST> is embedded all over the html
3) Apply this patch
4) Refresh the page, note the injection is gone!
5) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2017-12-08 18:03:54 UTC
Created attachment 69657 [details] [review]
Bug 19319: (QA follow-up) Biblionumber leftovers in opac-detail.tt

Correcting a few biblionumber template var leftovers in opac-detail
template. They were combined with url filter instead of html.

Test plan:
[1] Look for [^.;?]biblionumber in opac-detail.tt and verify that these
    occurrences are fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2017-12-08 18:03:59 UTC
Created attachment 69658 [details] [review]
Bug 19319: Only fetch the record if it exists

We already know if the bibliographic record exists (404 redirect),
we can avoid unecessary fetches

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Marcel de Rooy 2017-12-15 10:02:27 UTC
Bug 4319 will benefit from the changes on this report.
Comment 24 Chris Cormack 2017-12-21 00:01:42 UTC
Pushed to 16.11.x will be in 16.11.15
Comment 25 Nick Clemens 2017-12-22 10:57:49 UTC
Pushed to stable for 17.11.01
Comment 26 Fridolin Somers 2018-01-04 09:14:42 UTC
Pushed to 17.05.x for 17.05.07
Comment 27 Jonathan Druart 2018-01-09 20:29:53 UTC
Pushed to master for 18.05, thanks to everybody involved!