Security concern for SQL Injections/XSS

Question

This question is everywhere, and I looked at many answers. A lot suggest using mysql_real_escape_string but that isn't an option, since it's depreciated now.

Being a rookie at PHP/SQL and especially when it comes to security, I can only hope some of you may be able to help out here. I know for a fact that some of these procedures I already use contribute to security.

As an example: I didn't use :pname before when defining query_params, which caused SQL to interpret submissions like 'values'; drop table dbname; as actual commands. One thing that did save me from the command not being executed is the conversion of spaces to underscores.

search_submit.php

<?php
$pname=str_replace(' ','_',strtolower($_GET['pname']));
require 'connection.php';
  if(isset($_GET['pname'])){

  if(preg_match("/\w/i", $_GET['pname'])){
   $query="SELECT SCHEMA_NAME FROM INFORMATION_SCHEMA.SCHEMATA WHERE   SCHEMA_NAME = :pname;";
           $query_params = array( 
             ':pname' => str_replace(' ','_',strtolower($_GET['pname']))
        );
   $prep=$conn->prepare($query);
   $result=$prep->execute($query_params) or die("failed!");
   $row = $prep->fetch(); 
        if($row) 
        { 
            die("Exists"); 
        }
        else{
            echo("doesn't");
        }
  }
  }
  else{
  echo  "<p>Error</p>";
  }
?>

index.html

<form  method="get" action="search_submit.php"  id="searchform">
  <input  type="text" name="pname">
  <input  type="submit" name="input" value="Search">

As of right now everything appears to be working perfectly, and any sneaky submissions are simply interpreted as text. But I am a very paranoid person.

Answer 1

Preventing SQL Injection

A lot suggest using mysql_real_escape_string

Some people do suggest this, but it's a bad suggestion. The only proper defense against SQL injection are prepared statements.

Escaping is at best a second-best solution in case you cannot use prepared statements for some reason.

But that isn't an option, since it's depreciated now.

mysql_ is deprecated, and has been for quite some time. mysqli_ however is not, and it does offer mysqli_real_escape_string. Still, prepared statements are the way to go.

The good news is that you use prepared statements, and use them correctly, so you are defending against SQL injection exactly as you should be.

Your additional Filters

That being said, your additional filters are not helping, and are in fact adding a false sense of additional security:

• Replacing with _ doesn't really prevent anything. Pretty much all attacks I can imagine would likely still be possible, depending on the concrete situation. For SQL injection, you could eg use comments (/**/) instead of spaces.
• your regex checks if there is any word character given. So ' and 1=1-- would pass through it, as a is a word character.

But as I said, your code is secure against SQL Injection, so you don't need additional protection. Still, input filtering as defense in depth is a good idea (just not the way you were filtering), although I would do it in a centralized place and a bit more structured. See eg here.

As you mention XSS: As you are not echoing anything variable, you do not need to worry about XSS at the moment.

Misc

• your indentation is off, making your code hard to read (it's eg hard to see what the last else closes).
• your spacing is also off, as is your positioning of brackets.
• you never use $pname.
• it's not clear to me why you sometimes die and sometimes echo.
• you can make your queries more readable by separating keywords and variables by case. So for example, keywords could be all uppercase, variables all lowercase.

Given all of this, your code could look something like this:

require 'connection.php';
if(!isset($_GET['pname'])) {
    return "<p>Error</p>";
}

$query="SELECT schema_name FROM information_schema.schemata WHERE schema_name = :pname;";
$query_params = array( 
    ':pname' => $_GET['pname']
);

$prep = $conn->prepare($query);
$result = $prep->execute($query_params) or die("failed!");
$row = $prep->fetch(); 

if($row) { 
    echo "Exists" ; 
} else {
    echo "doesn't" ;
}

Answer 2

I would like to add to @tim's answer with:

1) make a validation function that check your $_GET['pname'] parameter, this way its isolated and you can return the error early if the validation fails without hitting the DB

2) as tim mentioned escaping string is useless because you are using PDO, prepare statements already quotes the parameter for you. As an alternative if you need to quote again (but your input might be wonky in storage would be to use PDO::quote() as found in the solution: https://stackoverflow.com/questions/3716373/real-escape-string-and-pdo