Bug 27715: Use $dbh->quote_identifier to quote untrusted input

The sanitization using regex and \w class of characters might be
enough but given the vast number of unicode characters in \w and
possibility of in the future the database engines interpreting some of
those characters with special meaning it is better to wrap the column
identifier to quotes using $dbh->quote_identifier so it is only
interpreted as identifier and nothing else.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

C4/Utils/DataTables.pm

@@ -85,8 +85,8 @@ sub dt_build_orderby {
     my $param = shift;
 
     my $i = 0;
-    my $orderby;
     my @orderbys;
+    my $dbh = C4::Context->dbh;
     while(exists $param->{'iSortCol_'.$i}){
         my $iSortCol = $param->{'iSortCol_'.$i};
         my $sSortDir = $param->{'sSortDir_'.$i};
@@ -105,9 +105,18 @@ sub dt_build_orderby {
     return unless @orderbys;
 
     # Must be "branches.branchname asc", "borrowers.firstname desc", etc.
-    @orderbys = grep { /^\w+\.\w+\s(asc|desc)$/ } @orderbys;
+    @orderbys = grep { /^\w+\.\w+ (asc|desc)$/ } @orderbys;
+
+    my @sanitized_orderbys;
+    for my $orderby (@orderbys) {
+      my ($identifier, $direction) = split / /, $orderby, 2;
+      my ($table, $column) = split /\./, $identifier, 2;
+      my $sanitized_identifier = $dbh->quote_identifier(undef, $table, $column);
+      my $sanitized_direction = $direction eq 'asc' ? 'ASC' : 'DESC';
+      push @sanitized_orderbys, "$sanitized_identifier $sanitized_direction";
+    }
 
-    $orderby = " ORDER BY " . join(',', @orderbys) . " " if @orderbys;
+    my $orderby = " ORDER BY " . join(',', @sanitized_orderbys) . " " if @sanitized_orderbys;
     return $orderby;
 }
 

t/db_dependent/Utils/Datatables.t

@@ -47,7 +47,7 @@ subtest 'dt_build_orderby' => sub {
     };
 
     my $orderby = dt_build_orderby($dt_params);
-    is( $orderby, " ORDER BY branches.branchname asc,borrowers.surname desc,borrowers.firstname desc ", 'ORDER BY has been correctly built' );
+    is( $orderby, " ORDER BY `branches`.`branchname` ASC,`borrowers`.`surname` DESC,`borrowers`.`firstname` DESC ", 'ORDER BY has been correctly built' );
 
     $dt_params = {
         %$dt_params,
@@ -57,5 +57,5 @@ subtest 'dt_build_orderby' => sub {
     };
 
     $orderby = dt_build_orderby($dt_params);
-    is( $orderby, " ORDER BY branches.branchname asc,borrowers.surname desc,borrowers.firstname desc ", 'ORDER BY has been correctly built, even with invalid stuff');
+    is( $orderby, " ORDER BY `branches`.`branchname` ASC,`borrowers`.`surname` DESC,`borrowers`.`firstname` DESC ", 'ORDER BY has been correctly built, even with invalid stuff');
 };