From 3739e6bd6722af35a9f3f55af0e889036e56010e Mon Sep 17 00:00:00 2001 From: Jared Camins-Esakov Date: Mon, 15 Oct 2012 11:45:38 -0400 Subject: [PATCH 1/4] 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 Signed-off-by: Paul Poulain --- opac/opac-ISBDdetail.pl | 3 ++- opac/opac-MARCdetail.pl | 3 ++- opac/opac-authoritiesdetail.pl | 1 + opac/opac-detail.pl | 1 + opac/opac-showmarc.pl | 1 + 5 files changed, 7 insertions(+), 2 deletions(-) diff --git a/opac/opac-ISBDdetail.pl b/opac/opac-ISBDdetail.pl index 773ed51eb1..8c29936737 100755 --- a/opac/opac-ISBDdetail.pl +++ b/opac/opac-ISBDdetail.pl @@ -66,7 +66,8 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( } ); -my $biblionumber = $query->param('biblionumber'); +my $biblionumber = $query->param('biblionumber') || $query->param('bib'); +$biblionumber = int($biblionumber); # get biblionumbers stored in the cart my @cart_list; diff --git a/opac/opac-MARCdetail.pl b/opac/opac-MARCdetail.pl index 2d9dc25241..f39f2d2083 100755 --- a/opac/opac-MARCdetail.pl +++ b/opac/opac-MARCdetail.pl @@ -57,10 +57,11 @@ my $query = new CGI; my $dbh = C4::Context->dbh; -my $biblionumber = $query->param('biblionumber'); +my $biblionumber = $query->param('biblionumber') || $query->param('bib'); my $itemtype = &GetFrameworkCode($biblionumber); my $tagslib = &GetMarcStructure( 0, $itemtype ); my $biblio = GetBiblioData($biblionumber); +$biblionumber = $biblio->{biblionumber}; my $record = GetMarcBiblio($biblionumber, 1); if ( ! $record ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); diff --git a/opac/opac-authoritiesdetail.pl b/opac/opac-authoritiesdetail.pl index ffe9734bc8..195e244274 100755 --- a/opac/opac-authoritiesdetail.pl +++ b/opac/opac-authoritiesdetail.pl @@ -67,6 +67,7 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( ); my $authid = $query->param('authid'); +$authid = int($authid); my $record = GetAuthority( $authid ); if ( ! $record ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); # escape early diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index cdb4a5aecd..d9ecdfad0e 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -69,6 +69,7 @@ my ( $template, $borrowernumber, $cookie ) = get_template_and_user( ); my $biblionumber = $query->param('biblionumber') || $query->param('bib'); +$biblionumber = int($biblionumber); my $record = GetMarcBiblio($biblionumber); if ( ! $record ) { diff --git a/opac/opac-showmarc.pl b/opac/opac-showmarc.pl index 3638f8869d..f06d3cde0b 100755 --- a/opac/opac-showmarc.pl +++ b/opac/opac-showmarc.pl @@ -44,6 +44,7 @@ use XML::LibXML; my $input = new CGI; my $biblionumber = $input->param('id'); +$biblionumber = int($biblionumber); my $importid = $input->param('importid'); my $view = $input->param('viewas') || 'marc'; From 70f2b4bd0aeb1c09e988595df7da27279659f56d Mon Sep 17 00:00:00 2001 From: Paul Poulain Date: Wed, 24 Oct 2012 15:30:24 +0200 Subject: [PATCH 2/4] Bug 3652 follow-up reverting call to param('bib') could probably also be removed in opac-detail.pl, but it was still here before Jared patch. So, in case something is still using bib I haven't removed this call --- opac/opac-ISBDdetail.pl | 2 +- opac/opac-MARCdetail.pl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/opac/opac-ISBDdetail.pl b/opac/opac-ISBDdetail.pl index 8c29936737..be628795e1 100755 --- a/opac/opac-ISBDdetail.pl +++ b/opac/opac-ISBDdetail.pl @@ -66,7 +66,7 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( } ); -my $biblionumber = $query->param('biblionumber') || $query->param('bib'); +my $biblionumber = $query->param('biblionumber'); $biblionumber = int($biblionumber); # get biblionumbers stored in the cart diff --git a/opac/opac-MARCdetail.pl b/opac/opac-MARCdetail.pl index f39f2d2083..ac13a9d2c0 100755 --- a/opac/opac-MARCdetail.pl +++ b/opac/opac-MARCdetail.pl @@ -57,7 +57,7 @@ my $query = new CGI; my $dbh = C4::Context->dbh; -my $biblionumber = $query->param('biblionumber') || $query->param('bib'); +my $biblionumber = $query->param('biblionumber'); my $itemtype = &GetFrameworkCode($biblionumber); my $tagslib = &GetMarcStructure( 0, $itemtype ); my $biblio = GetBiblioData($biblionumber); From 35b6a5ea116f8cafc92c31b0879dccb1cbe23a6b Mon Sep 17 00:00:00 2001 From: Jared Camins-Esakov Date: Mon, 15 Oct 2012 11:58:30 -0400 Subject: [PATCH 3/4] 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=

evil

(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 Signed-off-by: Paul Poulain --- opac/opac-export.pl | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/opac/opac-export.pl b/opac/opac-export.pl index fb8dee7a1e..002c88e4ec 100755 --- a/opac/opac-export.pl +++ b/opac/opac-export.pl @@ -32,6 +32,7 @@ my $query = new CGI; my $op=$query->param("op")||''; #op=export is currently the only use my $format=$query->param("format")||'utf8'; my $biblionumber = $query->param("bib")||0; +$biblionumber = int($biblionumber); my ($marc, $error)= ('',''); $marc = GetMarcBiblio($biblionumber, 1) if $biblionumber; @@ -41,18 +42,23 @@ if(!$marc) { } elsif ($format =~ /endnote/) { $marc = marc2endnote($marc); + $format = 'endnote'; } elsif ($format =~ /marcxml/) { $marc = marc2marcxml($marc); + $format = 'marcxml'; } elsif ($format=~ /mods/) { $marc = marc2modsxml($marc); + $format = 'mods'; } elsif ($format =~ /ris/) { $marc = marc2ris($marc); + $format = 'ris'; } elsif ($format =~ /bibtex/) { $marc = marc2bibtex(C4::Biblio::GetMarcBiblio($biblionumber),$biblionumber); + $format = 'bibtex'; } elsif ($format =~ /dc/) { ($error,$marc) = marc2dcxml($marc,1); @@ -61,14 +67,17 @@ elsif ($format =~ /dc/) { elsif ($format =~ /marc8/) { ($error,$marc) = changeEncoding($marc,"MARC","MARC21","MARC-8"); $marc = $marc->as_usmarc() unless $error; + $format = 'marc8'; } elsif ($format =~ /utf8/) { C4::Charset::SetUTF8Flag($marc,1); $marc = $marc->as_usmarc(); + $format = 'utf8'; } elsif ($format =~ /marcstd/) { C4::Charset::SetUTF8Flag($marc,1); ($error,$marc) = marc2marc($marc, 'marcstd', C4::Context->preference('marcflavour')); + $format = 'marcstd'; } else { $error= "Format $format is not supported."; @@ -78,7 +87,7 @@ if ($error){ print $query->header(); print $query->start_html(); print "

An error occurred

"; - print $error; + print $query->escapeHTML("$error"); print $query->end_html(); } else { From d2de76d60d7369e26e8c3f806b9bdcdb6eeaa4fd Mon Sep 17 00:00:00 2001 From: Chris Hall Date: Wed, 17 Oct 2012 14:32:19 +1300 Subject: [PATCH 4/4] bug 3652 fixing XSS vulnerabilities in opac-search Signed-off-by: Mason James Signed-off-by: Paul Poulain --- koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt | 2 +- koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt | 2 +- koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt index f740488d94..39a4e95e88 100644 --- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt +++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt @@ -63,7 +63,7 @@ widgets : ['zebra'], sortList: [[0,0]] }); - [% IF ( query_desc ) %][% IF ( OpacHighlightedWords ) %]var query_desc = "[% query_desc |replace("'", "\'") |replace('"', '\"') |replace('\n', '\\n') |replace('\r', '\\r') %]"; + [% IF ( query_desc ) %][% IF ( OpacHighlightedWords ) %]var query_desc = "[% query_desc |replace("'", "\'") |replace('\n', '\\n') |replace('\r', '\\r') |html %]"; q_array = query_desc.split(" "); highlightOn(); $("#highlight_toggle_on" ).hide().click(function() {highlightOn() ; return false;}); diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt index 86e1a088be..222d841eb9 100644 --- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt +++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results-grouped.tt @@ -62,7 +62,7 @@ $(document).ready(function(){ return false; }); [% IF ( query_desc ) %] - var query_desc = "[% query_desc |replace("'", "\'") |replace('"', '\"') |replace('\n', '\\n') |replace('\r', '\\r') %]"; + var query_desc = "[% query_desc |replace("'", "\'") |replace('\n', '\\n') |replace('\r', '\\r') |html %]"; q_array = query_desc.split(" "); // ensure that we don't have "" at the end of the array, which can // break the highlighter diff --git a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt index a3c09e2966..95ed896c2d 100644 --- a/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt +++ b/koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt @@ -207,7 +207,7 @@ $(document).ready(function(){ [% END %] $("#holdDetails").hide(); -[% IF ( query_desc ) %][% IF ( OpacHighlightedWords ) %]var query_desc = "[% query_desc |replace("'", "\'") |replace('"', '\"') |replace('\n', '\\n') |replace('\r', '\\r') %]"; +[% IF ( query_desc ) %][% IF ( OpacHighlightedWords ) %]var query_desc = "[% query_desc |replace("'", "\'") |replace('\n', '\\n') |replace('\r', '\\r') |html %]"; q_array = query_desc.split(" "); // ensure that we don't have "" at the end of the array, which can // break the highlighter