Try go to this URL on your site: /cgi-bin/koha/opac-MARCdetail.pl?biblionumber=2"><TEST>
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!
Kyle, opac-MARCdetail.tt includes opac-detail-sidebar.inc, which use the biblionumber variable. Moreover there is a typo: -<title>[...] +:title>[...]
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!
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.
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>
Sidenote: opac-detail.pl contains two use Koha::Biblios;
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?
Feedback please ^^
(In reply to Marcel de Rooy from comment #6) > Sidenote: opac-detail.pl contains two use Koha::Biblios; See bug 19576. :)
(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 )!
(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.
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?
Moving to In Discussion
I'd just replace biblio.id with biblio.biblionumber
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>
QA: Working here now
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>
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>
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
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>
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>
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>
Bug 4319 will benefit from the changes on this report.
Pushed to 16.11.x will be in 16.11.15
Pushed to stable for 17.11.01
Pushed to 17.05.x for 17.05.07
Pushed to master for 18.05, thanks to everybody involved!