1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ mysql_real_escape should not be used indeed. The code looks fine. It's not vulnerable to sql injection using parameterized queries. You should also keep in mind that the user you're connecting to the database with doesnt have permissions to alter other databases or drop tables if you want to be extra careful. \$\endgroup\$ – JazzCat Mar 19 '16 at 19:42
1
\$\begingroup\$

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" ;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for these inputs. The regex checking I want because I want to make sure only letters (upper and lower) digits (0-9) spaces and underscores get through. Would using '/\d[a-zA-Z]/' be a better option in that case and would it allow underscores? I want this only because it'll be impossible for those results to contain any characters besides those anyway. What I am making here is in very early development, so I was only preparing to use that variable. Thanks for the rest of the recommendations too. \$\endgroup\$ – xmaxert Mar 19 '16 at 20:05
  • \$\begingroup\$ @xmaxert no, that regex doesn't look better. preg_match searches until it finds the pattern, and then returns true. That means when your regex allows say a-Z, if your input is a!$#%@^#, that would match. You need negation, see eg here. And as you will likely have more than one input variable in the future, I would definitely extract the filter to somewhere else instead of repeating it (imagine you find another bug in the future in your regex, now you would need to change it in multiple places). \$\endgroup\$ – tim Mar 19 '16 at 20:14
0
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.