From b0f60221f41041665c4fecacce35654fc8d45a01 Mon Sep 17 00:00:00 2001
From: Chris Nighswonger <cnighswonger@foundations.edu>
Date: Thu, 24 Feb 2011 09:57:11 -0500
Subject: [PATCH] Security Bugfix: Bug 1953 Adding Placeholders to SQL To Avoid
 Potential Injection Attacks
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

This patch addresses both security issues mentioned in the summary of the report
submitted by Frère Sébastien Marie included below.

---------------------------
The problem is here: 'C4/AuthoritiesMarc.pm' in the function 'DelAuthority':
The argument $authid is included directly (not via statement) in the SQL.

For the exploit of this problem, you can use 'authorities/authorities-home.pl'
with authid on the URL and op=delete (something like
"authorities/authorities-home.pl?op=delete&authid=xxx").

This should successfully call DelAuthority, without authentification...
(DelAuthority is call BEFORE get_template_and_user, so before authentification
[This should be an issue also...]).

Please note that the problem isn't only that anyone can delete an authority of
this choose, it is more general: with "authid=1%20or%1=1" (after inclusion sql
will be like: "delete from auth_header where authid=1 or 1=1") you delete all
authorities ; with "authid=1;delete%20from%xxx" it is "delete from auth_header
where authid=1;delete from xxx" and so delete what you want...

SQL-INJECTION is very permissive: you can redirect the output in a file (with
some MySQL function), so write thea file of you choose in the server, in order
to create a backdoor, and compromise the server.

Signed-off-by: Frère Sébastien Marie <semarie-koha@latrappe.fr>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
---
 C4/AuthoritiesMarc.pm           | 4 ++--
 authorities/authorities-home.pl | 5 +----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
index e5808b016b..9315e55110 100644
--- a/C4/AuthoritiesMarc.pm
+++ b/C4/AuthoritiesMarc.pm
@@ -719,8 +719,8 @@ sub DelAuthority {
     my $dbh=C4::Context->dbh;
 
     ModZebra($authid,"recordDelete","authorityserver",GetAuthority($authid),undef);
-    $dbh->do("delete from auth_header where authid=$authid") ;
-
+    my $sth = prepare("DELETE FROM auth_header WHERE authid=?");
+    $sth->execute($authid);
 }
 
 sub ModAuthority {
diff --git a/authorities/authorities-home.pl b/authorities/authorities-home.pl
index edf02a5989..e4eae84749 100755
--- a/authorities/authorities-home.pl
+++ b/authorities/authorities-home.pl
@@ -147,9 +147,6 @@ if ( $op eq "do_search" ) {
 
 }
 elsif ( $op eq "delete" ) {
-
-    &DelAuthority( $authid, 1 );
-
     ( $template, $loggedinuser, $cookie ) = get_template_and_user(
         {
             template_name   => "authorities/authorities-home.tmpl",
@@ -160,7 +157,7 @@ elsif ( $op eq "delete" ) {
             debug           => 1,
         }
     );
-
+    &DelAuthority( $authid, 1 );
 }
 else {
     ( $template, $loggedinuser, $cookie ) = get_template_and_user(
-- 
2.39.2