From 87933c74fe5428bc5f2fa52bec899a30cf38ac7f Mon Sep 17 00:00:00 2001 From: =?utf-8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Fri, 19 Feb 2021 14:18:15 +0200 Subject: [PATCH] Bug 27715: Use $dbh->quote_identifier to quote untrusted input Content-Type: text/plain; charset=utf-8 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 Signed-off-by: Marcel de Rooy --- C4/Utils/DataTables.pm | 15 ++++++++++++--- t/db_dependent/Utils/Datatables.t | 4 ++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/C4/Utils/DataTables.pm b/C4/Utils/DataTables.pm index b43849c008..23c7ebd405 100644 --- a/C4/Utils/DataTables.pm +++ b/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; } diff --git a/t/db_dependent/Utils/Datatables.t b/t/db_dependent/Utils/Datatables.t index 9f52ce3508..ae0c61bc4e 100755 --- a/t/db_dependent/Utils/Datatables.t +++ b/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'); }; -- 2.11.0