Description
Chris Cormack
2010-05-21 01:13:19 UTC
Restoring where "Previous assignee was mjr@ttllp.co.uk" Created attachment 2364 [details] [review] OPAC fixes - Setting default_escape => "HTML" in C4::Output - In the OPAC: - Removing redundant "ESCAPE='HTML'" from the templates - Adding "ESCAPE='0'" where necessary: -- Variables like OpacNav which output HTML or JS -- Instances of CGI::scrolling_list variables This is in new/bug3652 I have merged master in and pushed it up, please test this branch and if it works ok, let me know and I can merge it into master Chris N http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=f1d4d4d15c5038f2c668a232b3a169c20df31966 Is the commit you will want to cherry-pick if this tests ok Created attachment 2784 [details] [review] Revised patch for the OPAC Created attachment 2785 [details] [review] Proposed fix for the staff client This bug is mentioned in: Further fixes for Bug 3652 - XSS vulnerabilities http://lists.koha-community.org/pipermail/koha-patches/2010-November/013097.html Created attachment 2838 [details] [review] Revised fix Revised for current HEAD This bug is mentioned in: Further fixes for Bug 3652 - XSS vulnerabilities http://lists.koha-community.org/pipermail/koha-patches/2010-December/013162.html patch still not made his way into Koha, XSS vulnerabilities still here, but the patch won't apply anymore as we've now using T::T and this patch is based on H::T::P This will need a total rewrite for master Template::Toolkit I think we could do it with this https://metacpan.org/module/Template::AutoFilter mmm... reading comments I see: WARNING: This module is highly experimental. I have not done a lot of testing and things might blow up in unexpected ways. The API and behavior might change with any release (until 1.0). If you'd like to see any changes implemented, let me know via RT, email, IRC or by opening a pull request on github. Use at your own risk. What is the current status on this? Do we still have XSS vulnerabilities? XSS vulnerability when viewing shelves. To test 1/ Visit /cgi-bin/koha/opac-shelves.pl?viewshelf=<blink>fish</blink>&sortfield=1 After patch this should no longer be blinking Created attachment 12728 [details] [review] Bug 3652 : [SECURITY] XSS vulnerability Sorry forgot to mention this only shows up when you dont have permission to view the list Created attachment 12729 [details] [review] Bug 3652 : [SECURITY] XSS vulnerability Signed-off-by: Magnus Enger <magnus@enger.priv.no> Works as advertised. After applying the patch, <blink>fish</blink> is displayed on the page, but no blinking occurs. Created attachment 12730 [details] [review] Bug 3652 : [SIGNED-OFF] [SECURITY] XSS vulnerability Signed-off-by: Magnus Enger <magnus@enger.priv.no> Works as advertised. After applying the patch, <blink>fish</blink> is displayed on the page, but no blinking occurs. If you have a search that returns more than one page of results it is possible to craft an xss exploit. With page numbers turned on try /cgi-bin/koha/opac-search.pl?q=1&do=Search&limit-yr=1&limit=1&idx=kw&sort_by=relevance"></a><b>This%20shouldn't%20happen</b> Then try it again with the patch applied. Another patch to follow to fix facets Created attachment 12737 [details] [review] Bug 3652: XSS vulnerability in page numbering Created attachment 12738 [details] [review] Bug 3652 XSS vulnerability in facets Created attachment 12740 [details] [review] [SIGNED-OFF] Bug 3652: XSS vulnerability in page numbering Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Confirmed the problem with test plan on the bug and checked that paging works correctly after the patch. Created attachment 12741 [details] [review] [SIGNED-OFF] Bug 3652 XSS vulnerability in facets Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Confirmed bug with test plan on the bug and checked that problem is fixed after applying the patch and that facets still work. (In reply to comment #24) > Created attachment 12741 [details] [review] > [SIGNED-OFF] Bug 3652 XSS vulnerability in facets > > Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> > Confirmed bug with test plan on the bug and checked that > problem is fixed after applying the patch and that facets > still work. 3 patches look good, passing QA - well spotted! $ koha-qa.pl -c 3 testing 3 commit(s) (applied to commit 0c75d66) * 55c3117 Bug 3652 XSS vulnerability in facets koha-tmpl/opac-tmpl/prog/en/includes/opac-facets.inc * 210a889 Bug 3652: XSS vulnerability in page numbering koha-tmpl/opac-tmpl/prog/en/includes/page-numbers.inc * f2db538 Bug 3652 : [SIGNED-OFF] [SECURITY] XSS vulnerability koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt * koha-tmpl/opac-tmpl/prog/en/includes/opac-facets.inc OK * koha-tmpl/opac-tmpl/prog/en/includes/page-numbers.inc OK * koha-tmpl/opac-tmpl/prog/en/modules/opac-shelves.tt OK Patch pushed to master Pushed to 3.8.x, will be in 3.8.6, probably needed for 3.6.x too Created attachment 12807 [details] [review] Bug 3652: XSS fixes - follow up Fixes a typo in the html filter that causes a problem when using the paging in the OPAC result lists. (forgot to commit my change when testing the original XSS patches) Created attachment 12808 [details] [review] Bug 3652: XSS fixes - follow up Fixes a typo in the html filter that causes a problem when using the paging in the OPAC result lists. (forgot to commit my change when testing the original XSS patches) Created attachment 12809 [details] [review] Bug 3652: [SIGNED-OFF] XSS fixes - follow up Fixes a typo in the html filter that causes a problem when using the paging in the OPAC result lists. (forgot to commit my change when testing the original XSS patches) Signed-off-by: Magnus Enger <magnus@enger.priv.no> I triggered the error with this query string: /cgi-bin/koha/opac-search.pl?idx=kw&q=o'reilly&offset=40&sort_by=author_az After applying the patch the error is gone. Created attachment 12820 [details] [review] Bug 3652: close XSS vulnerabilities on biblionumber and authid Previously we did not sanitize biblionumber and authids passed in by the user. To test: 1) Go to /cgi-bin/koha/opac-detail.pl?biblionumber=2hi (substituting a valid biblionumber for the 2). 2) Notice the presence of "2hi" on this page, and also on the ISBD and MARC views. 3) Go to /cgi-bin/koha/opac-authoritiesdetail.pl?authid=2bye (substituting a valid authid for the 2). 4) Notice the presence of "2bye" on this page. 3) Apply patch. 4) Notice that "2hi" and "2bye" strings are gone. Created attachment 12823 [details] [review] Bug 3652: close XSS vulnerabilities in opac-export The opac-export.pl script had a number of XSS vulnerabilities relating to its error handling. To test: 1) Go to /cgi-bin/koha/opac-export.pl?op=export&bib=2&format=<h2>evil</h2> (substituting a valid biblionumber for the '2') 2) Notice that "evil" is rendered as an h2 heading. 3) Apply patch. 4) Notice that you now see the h2 tags, and they are not rendered by the browser. Created attachment 12835 [details] [review] Bug 3652: close XSS vulnerabilities on biblionumber and authid Previously we did not sanitize biblionumber and authids passed in by the user. To test: 1) Go to /cgi-bin/koha/opac-detail.pl?biblionumber=2hi (substituting a valid biblionumber for the 2). 2) Notice the presence of "2hi" on this page, and also on the ISBD and MARC views. 3) Go to /cgi-bin/koha/opac-authoritiesdetail.pl?authid=2bye (substituting a valid authid for the 2). 4) Notice the presence of "2bye" on this page. 3) Apply patch. 4) Notice that "2hi" and "2bye" strings are gone. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Created attachment 12836 [details] [review] Bug 3652: close XSS vulnerabilities in opac-export The opac-export.pl script had a number of XSS vulnerabilities relating to its error handling. To test: 1) Go to /cgi-bin/koha/opac-export.pl?op=export&bib=2&format=<h2>evil</h2> (substituting a valid biblionumber for the '2') 2) Notice that "evil" is rendered as an h2 heading. 3) Apply patch. 4) Notice that you now see the h2 tags, and they are not rendered by the browser. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> *** Bug 8930 has been marked as a duplicate of this bug. *** Created attachment 12876 [details] [review] Fixing XSS vulnerability in opac-search Fixes XSS vulnerabilites in opac-search Search in the opac for ';</script><script>alert(10);</alert>' (without the quotes around it), a pop up should appear. You could also visit /cgi-bin/koha/opac-search.pl?q=%3B%3C%2Fscript%3E%3Cscript%3Ealert%2810%29%3B%3C%2Fscript%3E Notes: The above XSS vuln will work in firefox 16.01 but will not work in chromium 18 as chromium detects that the string is user supplies, instead chromium will report an error message to the console. Arachni still believes that the opac-search is vulnerable despite this fix, I believe it is incorrect as the string it tries to inject is made up of alphanumeric and hyphens, (as far as I know) none of these are dangerous when rendered within a form or script element. Comment on attachment 12809 [details] [review] Bug 3652: [SIGNED-OFF] XSS fixes - follow up QA comment, trivial patch, passes QA & pushed Comment on attachment 12835 [details] [review] Bug 3652: close XSS vulnerabilities on biblionumber and authid About this patch, Jared, why do you add || $query->param('bib'); to opac-ISBD|MARCdetail.pl ? I see it's in opac-detail, but it's an oldies and not goodies (in early versions of Koha, biblionumber was sometimes written bib, bn, ... It has been fixed, and I favour removing || $query->param('bib'); from opac-detail.pl, because we must not have param('bib') (In reply to comment #38) > Comment on attachment 12835 [details] [review] > Bug 3652: close XSS vulnerabilities on biblionumber and authid > > About this patch, Jared, why do you add > || $query->param('bib'); > to opac-ISBD|MARCdetail.pl ? > I see it's in opac-detail, but it's an oldies and not goodies (in early > versions of Koha, biblionumber was sometimes written bib, bn, ... It has > been fixed, and I favour removing > || $query->param('bib'); > from opac-detail.pl, because we must not have param('bib') I wanted to make sure the behavior was identical, and I figured there must surely be a good reason for the $query->param('bib'). If you wanted to remove the || $query->param('bib') from all three files, I would not object at all. passing QA on 2 patches... $ koha-qa.pl -c 2 testing 2 commit(s) (applied to commit d91a4f8) * 09305f2 bug 3652 fixing XSS vulnerabilities in opac-search koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt * 005a1ea Bug 3652: close XSS vulnerabilities in opac-export opac/opac-export.pl * opac/opac-export.pl OK * koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt OK * koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt OK * koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt OK Created attachment 12926 [details] [review] bug 3652 fixing XSS vulnerabilities in opac-search Signed-off-by: Mason James <mtj@kohaaloha.com> (In reply to comment #41) > Created attachment 12926 [details] [review] > bug 3652 fixing XSS vulnerabilities in opac-search > > Signed-off-by: Mason James <mtj@kohaaloha.com> tested Chris' patch, works well (In reply to comment #40) > passing QA on 2 patches... oops, QA'ed the wrong patch $ koha-qa.pl -c 2 testing 2 commit(s) (applied to commit 32fab74) * 66a972f Bug 3652: close XSS vulnerabilities in opac-export opac/opac-export.pl * 2387688 Bug 3652: close XSS vulnerabilities on biblionumber and authid opac/opac-ISBDdetail.pl opac/opac-MARCdetail.pl opac/opac-authoritiesdetail.pl opac/opac-detail.pl opac/opac-showmarc.pl * opac/opac-export.pl OK * opac/opac-ISBDdetail.pl OK * opac/opac-MARCdetail.pl OK * opac/opac-authoritiesdetail.pl OK * opac/opac-detail.pl OK * opac/opac-showmarc.pl OK i actually tested these against master, ill flick the version to reflect that (hope thats ok) Pushed to 3.8.x will be in 3.8.6 The 3 patches Bug 3652: close XSS vulnerabilities on biblionumber and authid (3.40 KB, patch) Bug 3652: close XSS vulnerabilities in opac-export (2.62 KB, patch) bug 3652 fixing XSS vulnerabilities in opac-search (3.04 KB, patch) have been pushed QA comment for Bug 3652: close XSS vulnerabilities on biblionumber and authid (3.40 KB, patch) = I made a follow-up to remove the || $query->param('bib'); (see comment 38) I think opac-detail.pl could also be fixed, but in case there's an old reference to this, I won't do that without a specific patch. Comment for opac-search = the XSS did not work for me if I entered > Search in the opac for ';</script><script>alert(10);</alert>' If was exploitable only with > /cgi-bin/koha/opac-search. > pl?q=%3B%3C%2Fscript%3E%3Cscript%3Ealert%2810%29%3B%3C%2Fscript%3E but it's worth pushing it anyway status back to ASSIGNED if another XSS vulnerability is found & fixed No outstanding vulnerabilities at this time, a new bug can be opened if some are found |