From a1e0768ceb728f0019086050837884d29e498dfe Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 19 Jun 2015 10:12:45 +0200 Subject: [PATCH 01/18] Bug 14408 Path traversal vulnerability /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Signed-off-by: Katrin Fischer Signed-off-by: Kyle M Hall --- C4/Auth.pm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/C4/Auth.pm b/C4/Auth.pm index 06525c135b..a2aa8aa132 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -157,6 +157,10 @@ sub get_template_and_user { C4::Context->interface( $in->{type} ); + # Sanitize template path to avoid path traversal + $in->{template_name} =~ s|^/||; + $in->{template_name} =~ s|\.\.||g; + $in->{'authnotrequired'} ||= 0; my $template = C4::Templates::gettemplate( $in->{'template_name'}, From 0cba81194f86b1b7fbea9d2ab48fe8c995a3c247 Mon Sep 17 00:00:00 2001 From: Chris Cormack Date: Fri, 19 Jun 2015 08:35:07 +1200 Subject: [PATCH 02/18] Bug 14412 : SQL injection possible There is a SQL Injection vulnerability in the /cgi-bin/koha/opac-tags_subject.pl script. By manipulating the variable 'number', the database can be accessed via time-based blind injections. The following string serves as an example: /cgi-bin/koha/opac-tags_subject.pl?number=1+PROCEDURE+ANALYSE+(EXTRACTVALUE(9743,CONCAT(0x5c,(BENCHMARK(5000000,MD5('evil'))))),1) To exploit the vulnerability, no authentication is needed To test 1/ Turn on mysql query logging 2/ Hit /cgi-bin/koha/opac-tags_subject.pl?number=1+PROCEDURE+ANALYSE+(EXTRACTVALUE(9743,CONCAT(0x5c,(BENCHMARK(5000000,MD5('evil'))))),1) 3/ Check the logs notice something like SELECT entry,weight FROM tags ORDER BY weight DESC LIMIT 1 PROCEDURE ANALYSE (EXTRACTVALUE(9743,CONCAT(0x5c,(BENCHMARK(5000000,MD5('evil'))))),1) 4/ Apply patch 5/ Hit the url again 6/ Notice the log now only has SELECT entry,weight FROM tags ORDER BY weight DESC LIMIT 1 Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Confirmed the problem and the fix for it. Signed-off-by: Liz Rea --- opac/opac-tags_subject.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opac/opac-tags_subject.pl b/opac/opac-tags_subject.pl index 7cf75afc2b..a410be2ea6 100755 --- a/opac/opac-tags_subject.pl +++ b/opac/opac-tags_subject.pl @@ -51,8 +51,8 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( my $number = $query->param('number') || 100; -my $sth = $dbh->prepare("SELECT entry,weight FROM tags ORDER BY weight DESC LIMIT $number"); -$sth->execute; +my $sth = $dbh->prepare("SELECT entry,weight FROM tags ORDER BY weight DESC LIMIT ?"); +$sth->execute($number); my %result; my $max=0; From 611df7517a2f1fa58c6780463ff56253d908a23d Mon Sep 17 00:00:00 2001 From: Chris Cormack Date: Fri, 19 Jun 2015 09:25:22 +1200 Subject: [PATCH 03/18] Bug 14418 XSS Vulnerabilities Fix for /cgi-bin/koha/opac-search.pl To test 1/ Hit /cgi-bin/koha/opac-search.pl?tag="> Where the id is a valid shelf id 2/ Notice the js is executed 3/ Apply patch 4/ Reload page 5/ Notice input is now escaped on display Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Tested in Debian, couldn't reproduce the alert in Iceweasel, but in Chromium. Patch fixes it. --- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt index 4aef3de416..d0274c3720 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt @@ -527,7 +527,7 @@ [% IF ( edit ) %]
- +
Editing [% shelfname |html %] From f462209e86e30e8ea23da67fb367c77c6d33be88 Mon Sep 17 00:00:00 2001 From: Liz Rea Date: Tue, 23 Jun 2015 09:57:18 +1200 Subject: [PATCH 06/18] Revert "Bug 14408 Path traversal vulnerability" This reverts commit a1e0768ceb728f0019086050837884d29e498dfe. --- C4/Auth.pm | 4 ---- 1 file changed, 4 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index a2aa8aa132..06525c135b 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -157,10 +157,6 @@ sub get_template_and_user { C4::Context->interface( $in->{type} ); - # Sanitize template path to avoid path traversal - $in->{template_name} =~ s|^/||; - $in->{template_name} =~ s|\.\.||g; - $in->{'authnotrequired'} ||= 0; my $template = C4::Templates::gettemplate( $in->{'template_name'}, From 1cc1a9588a26eca84dd4014fde8454107598eb8e Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 22 Jun 2015 05:23:52 +0000 Subject: [PATCH 07/18] Bug 14408 Path Traversal error Counter counter patch Please test well, including with the null byte %00, this uses a whitelisting to only allow files ending with .tt and not allowing ../etc Note the previous patch tries to protect against /etc/passwd but //etc/passwd is now vulnerable. I do think a whitelist is safer than trying to do a blacklist /cgi-bin/koha/svc/virtualshelves/search /cgi-bin/koha/svc/members/search Are vulnerable To test: 1/ Hit /cgi-bin/koha/svc/members/search?template_path=members/tables/members_results.tt Notice you get a valid JSON response 2/ Hit /search?template_path=..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd (You may have add more ..%2f or remove them to get the correct path) Notice you can see the contents of the /etc/passwd file 3/ Hit /cgi-bin/koha/svc/members/search?template_path=test%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd 4/ Apply patch 5/ Hit the first url again, notice it still works 6/ Hit the second url notice it now errors with a file not found 7/ Hit the third url notice it now errors with a file not found Repeat for the other script also Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Signed-off-by: Liz Rea --- C4/Auth.pm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/C4/Auth.pm b/C4/Auth.pm index 06525c135b..25452892d8 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -157,6 +157,9 @@ sub get_template_and_user { C4::Context->interface( $in->{type} ); + my $safe_chars = 'a-zA-Z_\-\/'; + die "bad template path" unless $in->{'template_name'} =~ m/^[$safe_chars]+.tt?$/ig; #sanitize input + $in->{'authnotrequired'} ||= 0; my $template = C4::Templates::gettemplate( $in->{'template_name'}, From 96047dba2c8f97e5582277b88e047534babe1761 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 22 Jun 2015 10:56:26 +0200 Subject: [PATCH 08/18] Bug 14426: Escape or use placeholders for sql parameters Does this patch enough to prevent sql injection in borrowers_out.pl? ==================================================================== 1. "Criteria" Parameter, Payload: ELT(1=1,'evil') / ELT(1=2,'evil') ==================================================================== echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 186\r\n\r\nFilter=P_COM&Filter=&Limit=&output=file&basename=Export&MIME=CSV&sep=%3B&report_name=&do_it=1&userid=&password=&branch=&koha_login_context=intranet&Criteria=ELT(1=2,'evil')" | nc testbox 9002 echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 186\r\n\r\nFilter=P_COM&Filter=&Limit=&output=file&basename=Export&MIME=CSV&sep=%3B&report_name=&do_it=1&userid=&password=&branch=&koha_login_context=intranet&Criteria=ELT(1=1,'evil')" | nc testbox 9002 ==================================================================== 2. "Filter" Parameter, Payload: P_COM'+AND+'a'='a / P_COM'+AND+'a'='b ==================================================================== echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 183\r\n\r\nkoha_login_context=intranet&Limit=&Criteria=branchcode&output=file&basename=Export&MIME=CSV&sep=;&report_name=&do_it=1&userid=&password=&branch=&Filter=P_COM'+AND+'a'='a" | nc testbox 9002 echo -ne "POST /cgi-bin/koha/reports/borrowers_out.pl HTTP/1.1\r\nHost: testbox:9002\r\nContent-Length: 183\r\n\r\nkoha_login_context=intranet&Limit=&Criteria=branchcode&output=file&basename=Export&MIME=CSV&sep=;&report_name=&do_it=1&userid=&password=&branch=&Filter=P_COM'+AND+'a'='b" | nc testbox 9002 ==================================================================== Signed-off-by: Chris Cormack Signed-off-by: Katrin Fischer Signed-off-by: Liz Rea --- reports/borrowers_out.pl | 41 +++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/reports/borrowers_out.pl b/reports/borrowers_out.pl index 5831eecddd..027b8fa361 100755 --- a/reports/borrowers_out.pl +++ b/reports/borrowers_out.pl @@ -174,17 +174,19 @@ sub calculate { $colorder .= $column; my $strsth2; - $strsth2 .= "select distinct $colfield FROM borrowers WHERE 1"; - if ($colfilter[0]) { + $strsth2 .= "select distinct " . $dbh->quote($colfield) . " FROM borrowers WHERE 1"; + my @query_args; + if ( $colfilter[0] ) { $colfilter[0] =~ s/\*/%/g; - $strsth2 .= " and $column LIKE '$colfilter[0]' " ; + $strsth2 .= " and " . $dbh->quote($column) . "LIKE ?" ; + push @query_args, $colfilter[0]; } - $strsth2 .=" group by $colfield"; - $strsth2 .=" order by $colorder"; + $strsth2 .=" group by " . $dbh->quote($colfield); + $strsth2 .=" order by " . $dbh->quote($colorder); # warn "". $strsth2; my $sth2 = $dbh->prepare( $strsth2 ); - $sth2->execute; + $sth2->execute( @query_args ); while (my ($celvalue) = $sth2->fetchrow) { my %cell; # my %ft; @@ -217,21 +219,30 @@ sub calculate { # Processing calculation $strcalc .= "SELECT CONCAT( borrowers.surname , \"\\t\",borrowers.firstname, \"\\t\", borrowers.cardnumber)"; - $strcalc .= " , $colfield " if ($colfield); + $strcalc .= " , " . $dbh->quote($colfield) if ($colfield); $strcalc .= " FROM borrowers "; $strcalc .= "WHERE 1 "; - @$filters[0]=~ s/\*/%/g if (@$filters[0]); - $strcalc .= " AND borrowers.categorycode like '" . @$filters[0] ."'" if ( @$filters[0] ); - + my @query_args; + if ( @$filters[0] ) { + @$filters[0]=~ s/\*/%/g; + $strcalc .= " AND borrowers.categorycode like ?"; + push @query_args, @$filters[0]; + } $strcalc .= " AND NOT EXISTS (SELECT * FROM issues WHERE issues.borrowernumber=borrowers.borrowernumber "; - $strcalc .= " AND issues.timestamp> '" . @$filters[1] . "'" if (@$filters[1]); + if ( @$filters[1] ) { + $strcalc .= " AND issues.timestamap > ?"; + push @query_args, @$filters[1]; + } $strcalc .= ") "; $strcalc .= " AND NOT EXISTS (SELECT * FROM old_issues WHERE old_issues.borrowernumber=borrowers.borrowernumber "; - $strcalc .= " AND old_issues.timestamp> '" . @$filters[1] . "'" if (@$filters[1]); + if ( @$filters[1] ) { + $strcalc .= " AND old_issues.timestamp > ?"; + push @query_args, @$filters[1]; + } $strcalc .= ") "; $strcalc .= " group by borrowers.borrowernumber"; - $strcalc .= ", $colfield" if ($column); - $strcalc .= " order by $colfield " if ($colfield); + $strcalc .= ", " . $dbh->quote($colfield) if ($column); + $strcalc .= " order by " . $dbh->quote($colfield) if ($colfield); my $max; if ($line) { if (@loopcol) { @@ -241,7 +252,7 @@ sub calculate { } my $dbcalc = $dbh->prepare($strcalc); - $dbcalc->execute; + $dbcalc->execute( @query_args ); # warn "filling table"; my $previous_col; $i=1; From 794fb09fac40408e12504fb67337299e0b30abe9 Mon Sep 17 00:00:00 2001 From: Chris Date: Sun, 21 Jun 2015 08:10:20 +0000 Subject: [PATCH 09/18] Bug 14423 : XSS in authorities-home To test: 1/ Hit a url like http://localhost:8081/cgi-bin/koha/authorities/authorities-home.pl?op=do_search&type=intranet&marclist=mainentry&and_or=and&operator=contains&value=%22/%3E%3Cscript%3Ealert%28%27oh%20noes%27%29%3C/script%3E 2/ Notice you get 3 alert boxes 3/ Apply patch 4/ Hit the url again, no js Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Signed-off-by: Liz Rea --- .../intranet-tmpl/prog/en/includes/authorities-search.inc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search.inc index cbdc939355..433c92703d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/authorities-search.inc @@ -58,7 +58,7 @@ [% END %] - + - + - + - + [% IF ( letters ) %]

days ago +

  • days ago
  • [% INCLUDE 'date-format.inc' %]
    From eb964ffc9c9974169b4056a8fa937c65a35d8a1d Mon Sep 17 00:00:00 2001 From: Chris Date: Sun, 21 Jun 2015 08:33:13 +0000 Subject: [PATCH 11/18] Bug 14423 XSS bug in auth_subfields_structure 1/ Hit a url like http://localhost:8081/cgi-bin/koha/admin/auth_subfields_structure.pl?op=add_form&authtypecode=%27%3Cscript%3Ealert%28%27oh%20noes%27%29%3C/script%3E&tagfield=%22/%3E%3Cscript%3Ealert%28%27oh%20noes%27%29%3C/script%3E 2/ Notice a ton of alert boxes pop up 3/ Apply patch 4/ Reload url, no longer get any alerts 5/ Test fuctionality still works Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Signed-off-by: Liz Rea --- .../modules/admin/auth_subfields_structure.tt | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_subfields_structure.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_subfields_structure.tt index f7b1d52e0f..faaa5fb222 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_subfields_structure.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/auth_subfields_structure.tt @@ -27,11 +27,11 @@ function displayMoreConstraint(numlayer){ [% INCLUDE 'cat-search.inc' %]
    • + Cancel [% IF ( previous ) %] From 308c4c4d4685e05f6d6e763bfa785d78f230010b Mon Sep 17 00:00:00 2001 From: Chris Date: Sun, 21 Jun 2015 08:46:40 +0000 Subject: [PATCH 12/18] Bug 14423 : XSS issues in marc_subfields_structure 1/ Hit a url like http://localhost:8081/cgi-bin/koha/admin/marc_subfields_structure.pl?op=add_form&tagfield=%22/%3E%3Cscript%3Ealert%28%27oh%20noes%27%29%3C/script%3E 2/ Notice all the alert boxes 3/ Apply patch 4/ Reload page, no more alerts 5/ Test functionality still works Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Signed-off-by: Liz Rea --- .../modules/admin/marc_subfields_structure.tt | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt index bfa88158fe..c0a2863cb6 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt @@ -167,19 +167,19 @@ function populateHiddenCheckboxes(tab) { [% INCLUDE 'cat-search.inc' %]
    @@ -191,14 +191,14 @@ function populateHiddenCheckboxes(tab) { [% IF ( add_form ) %]

    [% IF ( use_heading_flags_p ) %] - [% IF ( heading_edit_subfields_p ) %]Tag [% tagfield %] Subfield constraints[% END %] + [% IF ( heading_edit_subfields_p ) %]Tag [% tagfield | html %] Subfield constraints[% END %] [% ELSE %] [% action %] [% END %]

    - +
      @@ -339,7 +339,7 @@ function populateHiddenCheckboxes(tab) { [% END %]
    - Cancel + Cancel
    [% END %] @@ -352,7 +352,7 @@ function populateHiddenCheckboxes(tab) {
    - + @@ -360,7 +360,7 @@ function populateHiddenCheckboxes(tab) { - + @@ -371,14 +371,14 @@ function populateHiddenCheckboxes(tab) {

    Data deleted

    - +
    [% END %] [% IF ( else ) %] -

    MARC subfield structure admin for [% tagfield %] [% IF ( frameworkcode ) %](framework [% frameworkcode %])[% ELSE %](default framework)[% END %]

    +

    MARC subfield structure admin for [% tagfield | html %] [% IF ( frameworkcode ) %](framework [% frameworkcode %])[% ELSE %](default framework)[% END %]

    This screen shows the subfields associated with the selected tag. You can edit subfields or add a new one by clicking on edit.

    The column Koha field shows that the subfield is linked with a Koha field. Koha can manage a MARC interface, or a Koha interface. This link ensures that both DB are synchronized, thus you can change from a MARC to a Koha interface easily.

    @@ -428,7 +428,7 @@ function populateHiddenCheckboxes(tab) { - Cancel + Cancel From e21bf78b8b073d568998da32acb64cae9d4f4edd Mon Sep 17 00:00:00 2001 From: Chris Date: Sun, 21 Jun 2015 09:01:32 +0000 Subject: [PATCH 13/18] Bug 14423 : XSS bugs in catalogue search To test 1/ hit a url like http://localhost:8081/cgi-bin/koha/catalogue/search.pl?limit=%3Cscript%3Ealert%28%27oh%20noes%27%29%3C/script%3E 2/ Notice alert boxes 3/ Apply patch 4/ Reload url, no alerts 5/ Check search still works Signed-off-by: Jonathan Druart Signed-off-by: Katrin Fischer Signed-off-by: Liz Rea --- .../intranet-tmpl/prog/en/modules/catalogue/results.tt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt index d8ccc2dd1d..3e43b23836 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt @@ -259,7 +259,7 @@ var holdForPatron = function () {
    @@ -304,7 +304,7 @@ var holdForPatron = function () {

    - [% total %] result(s) found [% IF ( query_desc ) %]for '[% query_desc |html %]'[% END %][% IF ( limit_desc ) %] with limit(s): '[% limit_desc %]'[% END %][% IF ( LibraryName ) %] in [% LibraryName %] Catalog[% END %].
      + [% total %] result(s) found [% IF ( query_desc ) %]for '[% query_desc |html %]'[% END %][% IF limit_desc %] with limit(s): '[% limit_desc | html %]'[% END %][% IF ( LibraryName ) %] in [% LibraryName %] Catalog[% END %].