Is this Injection Proof?

Lately, I have read so much about SQL Injections and have tried to defend against them. Just wondering if the code below is enough or am I missing something? Should I use mysql_real_escape_string(stripslashes()) sooner or is it okay?


    // Sanitize Password
    $pword = trim($_POST['pword']);
    If (empty($pword) || (!(ctype_alnum($pword))))
    {
      $mistakes[] = 'Your password is empty or enter only NUMBERS and LETTERS.';
    } else {
    $pword = mysql_real_escape_string(stripslashes($pword));
    }
    $Query = "SELECT * FROM administration WHERE admin=\\"" . $admin . "\\" AND password=\\"" . md5($pword) . "\\";";
    if (!$Result = mysql_query($Query)) exit('Error Validating User');
    if (mysql_num_rows($Result) == 1 && !(sizeof($mistakes) > 0)) {
      $Row = mysql_fetch_array($Result);
      $_SESSION['id'] = $Row['a_id'];

All you need is mysql_real_escape_string by itself as it strips slashes automatically and i believe it also strips the white space from the start and end of the string as well.

Move the mistakes check outside of the database call.


[color="green"]if (empty($mistakes)) {[/color]
    $Query = "SELECT * FROM administration WHERE admin=\\"" . $admin . "\\" AND password=\\"" . md5($pword) . "\\";";
    if (!$Result = mysql_query($Query)) exit('Error Validating User');
    if (mysql_num_rows($Result) == 1[s][color="red"] && !(sizeof($mistakes) > 0)[/color][/s]) {
        $Row = mysql_fetch_array($Result);
        $_SESSION['id'] = $Row['a_id']; 
[color="green"]}[/color]

You have forgotten to escape the $admin variable.
Move the mysql_real_escape_string down to the query assignment itself, as is demonstrated in the example on the mysql_query documentation page


} else {
    $pword = [s][color="red"]mysql_real_escape_string([/color][/s]stripslashes($pword)[s][color="red"])[/color][/s];
}
if (empty($mistakes)) {
    $Query = [color="green"]sprintf([/color]"SELECT * FROM administration WHERE admin=\\"[color="green"]%s[/color]\\" AND password=\\"[color="green"]%s[/color]\\";"[color="green"],
        mysql_real_escape_string($admin),
        mysql_real_escape_string(md5($pword))
    );[/color]

You can use either single or double quotes to delimit the MySQL string, so don’t feel forced to always use double quotes. Use single quotes for the SQL string, and double quotes inside it. Or the other way around if you like. Just get rid of the need to escape quotes when there’s no need to do so.

You can also apply some formatting, to make the SQL more readable.


$Query = sprintf(
    'SELECT * ' .
    'FROM administration ' .
    'WHERE admin="%s" AND password="%s"',
    ...
);

Finally, using an initial upper case on variable names is normally restricted to the names of classes. Remain consistent by using lower case and camelCase where need be.

The end result is an improvement over what there was before:


// Sanitize Password
$pword = trim($_POST['pword']);
If (empty($pword) || (!(ctype_alnum($pword)))) {
    $mistakes[] = 'Your password is empty or enter only NUMBERS and LETTERS.';
} else {
    $pword = stripslashes($pword);
}
if (empty($mistakes)) {
    $query = sprintf(
        'SELECT * ' .
        'FROM administration ' .
        'WHERE admin="%s" AND password="%s"',
        mysql_real_escape_string($admin),
        mysql_real_escape_string(md5($pword))
    );
    $result = mysql_query($query);
    if ($result === false) {
        exit('Error Validating User');
    } else if (mysql_num_rows($result) == 1) {
        $row = mysql_fetch_array($result);
        $_SESSION['id'] = $row['a_id']; 
    }
    ...
}

Thanks, Paul.

Your programming style is so much better than mine. I now feel like I am just the ‘Joke of the Day’ after people view my personal code.

I knew mine worked, but never knew it could be cleaned up like you did.

The [coding tips article has good information relating to that, specifically, the post regarding url="http://www.sitepoint.com/forums/showpost.php?p=4686096&postcount=16"Input and Output

There’s more that can be done too, for example using filter_input if you have an appropriate version of PHP.

Many of these things are small techniques, but when put together well they can help to simplify the code, so that you don’t have to spend as long working out what it does when you (or someone else) comes back to the code 6 months later.

Here is the entire page. After adding your new snippets, it just allows entry in without checking the database to see if the admin/password is within it. If admin and password fields have legal content (any letter or number), it bypasses filling the sessions information and allows entry into the site with a Welcome, Notice: Undefined index: admin (<– the session name ).

This is a modified version of Kevin Hanks admincontrol.php that I call from my .php pages to check if the user is authorized.

Any help would be appreciated. I just don’t see the problem.


<?php
session_start();
if (!isset($_SESSION['admin'])) { // First IF
  if (!isset($_POST['admin']) || !isset($_POST['password'])) { // Second IF
?>
 
  <!-- HTML and FORM to get username and password -->
 
  <!doctype html public "-//w3c//dtd html 4.01 transitional//en"
                        "http://www.w3.org/tr/html4/loose.dtd">
 
  <html xmlns="http://www.w3.org/1999/xhtml" xml: lang="en">
    <head>
      <title><?php echo $webpageTitle; ?></title>
        <link rel="stylesheet" type="text/css" href="../styles/admin.css">
    </head>
 
    <body class="text">
 
    <center>
      <font size="3">Login</font>
        <table>
          <form method="POST" action="admin.php">
            <tr><td colspan="2"><hr width="200"></td></tr>
            <tr><td><font color="#ff0000">Username</font></td><td><input size="15" maxlength="10" type="text" name="admin"></td></tr>
            <tr><td><font color="#ff0000">Password</font></td><td><input size="15" maxlength="10" type="password" name="password"></td></tr>
            <tr><td colspan="2"><hr width="200"></td></tr>
            <tr><td colspan="2" align="center"><input type="submit" value="Log in"></td></tr>
          </form>
        </table>
      <br />
      <br />
      <a href="mailto:webmaster@awebsite.com"><font size="2">Webmaster@Awebsite.com</font></a>
    </center>
 
    </body>
 
  </html>
 
  <?php
    exit();
  } else {
    require 'connect.php';
    $mistakes = array();
 
    // Sanitize Admin
    $admin = trim($_POST['admin']);
    If (empty($admin) || (!(ctype_alpha($admin)))) {
      $mistakes[] = 'Your username is empty or enter only ALPHABET characters.';
    } else {
      $admin = stripslashes($admin);
    }
 
    // Sanitize Password
    $password = trim($_POST['password']);
    If (empty($password) || (!(ctype_alnum($password)))) {
      $mistakes[] = 'Your password is empty or enter only ALPHABET and NUMBER characters.';
    } else {
      $password = stripslashes($password);
    }
 
    if (empty($mistakes)) { // Third IF
      $query = sprintf(
        'SELECT * ' .
        'FROM administration ' .
        'WHERE aname="%s" AND apass="%s"',
        mysql_real_escape_string($admin),
        mysql_real_escape_string(md5($password))
      );
      $result = mysql_query($query);
      if ($result === false) {
        exit('Error Validating User');
      } else if (mysql_num_rows($result) == 1) {
        $row = mysql_fetch_assoc($result);
        $_SESSION['id'] = $row['aid'];
        $_SESSION['admin'] = $row['aname'];
        $_SESSION['password'] = $row['apass'];
      }
    } else {
    ?>
 
      <!-- Error HTML Page -->
 
      <!doctype html public "-//w3c//dtd html 4.01 transitional//en"
                            "http://www.w3.org/tr/html4/loose.dtd">
 
      <html xmlns="http://www.w3.org/1999/xhtml" xml: lang="en">
 
        <head>
          <title><?php echo $webpageTitle; ?></title>
            <link rel="stylesheet" type="text/css" href="../styles/admin.css">
        </head>
 
        <body class="text">
 
          <center>
            <font size="3">Access Denied</font><br />
              <ul>
                <?php
                  if (sizeof($mistakes) > 0){
                    foreach ($mistakes as $errors) {
                      echo "<li><font color=\\"#ff0000\\">", $errors, "</font></li>\
";
                    }
                  }
                ?>
              </ul>
              <br />
              To try logging in again, press back button or <a href="admin.php">Click Here</a>.<br /><br />
              To go back to Awebsite.com, <a href="../index.php">Click Here</a>.
          </center>
        </body>
      </html>
    <?php
    exit();
 
    } // Third IF closing tag
 
  } // Second IF closing tag
 
} // First IF closing tag
?>
 

Please remove my snippets and revert your code back to how you originally had it.
Then add in each of the changes, one by one, to narrow down where the issue comes in.

I have been testing it.

These work fine and any error in inputing the information is caught by the if (empty($mistakes)) {


    // Sanitize Admin Name
    $admin = trim($_POST['admin']);
  If (empty($admin) || (!(ctype_alpha($admin)))) {
      $mistakes[] = 'Your username is empty or enter only ALPHABET characters.';
    } else {
      $admin = stripslashes($admin);
    }
 
    // Sanitize Password
    $password = trim($_POST['password']);
  If (empty($password) || (!(ctype_alnum($password)))) {
      $mistakes[] = 'Your password is empty or enter only ALPHABET and NUMBER characters.';
    } else {
      $password = stripslashes($password);
    }
 
    if (empty($mistakes)) { // Third IF

I can enter in any letter in $admin and $password and it exits this page ( admincontrol.php ) back to the page that called it, but it shows misinformation: Welcome, Notice: Undefined index: admin.

If I actually enter in a CORRECT admin and password from the database, it works perfectly.

It is just when I enter for example:
Admin: Z
Password: TT
It will take those two, it will bypass $mistake and will not check it against the database to see if the information you entered is correct or not.

It is either how I check the variables at the top of the page with !isset or in this:


    if (empty($mistakes)) { // Third IF
      $query = sprintf(
        'SELECT * ' .
        'FROM administration ' .
        'WHERE aname="%s" AND apass="%s"',
        mysql_real_escape_string($admin),
        mysql_real_escape_string(md5($password))
      );
      $result = mysql_query($query);
      if ($result === false) {
        exit('Error Validating User');
      } else if (mysql_num_rows($result) == 1) {
        $row = mysql_fetch_assoc($result);
        $_SESSION['id'] = $row['aid'];
        $_SESSION['admin'] = $row['aname'];
        $_SESSION['password'] = $row['apass'];
      }
    } else {

So, on an unsuccessful login, the session doesn’t contain an entry for the admin name. What should happen differently? Should the page with the welcome message not show, or show something different?

Crusade : On

If you want to limit injection possibilities as much as possible, you should use prepared queries. Either with mysqli or [URL=“http://php.net/manual/en/book.pdo.php”]PDO.

I see you are using md5 to hash your passwords. It’s better than plaintext but it’s not very good. Check this link about storing passwords.
And DON’T LIMIT WHAT KIND OF CHARACTERS SOMEONE CAN USE FOR HIS PASSWORD. A kitten dies everytime someone can’t use the password he want.

Thanks again for the help, Paul. I finally found the error and it was all my fault.

It was only checking one and not both of these:


if (empty($mistakes) && mysql_num_rows($result) == 1) {

If there is an error with either one, they both have to go to the same place. I couldn’t figure out any other way, so I put them both on the same line again.

Below is the area affected.


    $query = sprintf(
      'SELECT * ' .
      'FROM administration ' .
      'WHERE aname="%s" AND apass="%s"',
      mysql_real_escape_string($admin),
      mysql_real_escape_string(md5($password))
    );
 
    $result = mysql_query($query);
 
    if ($result === false) exit('Error Validating User');
>>
    if (empty($mistakes) && mysql_num_rows($result) == 1) {
        $row = mysql_fetch_array($result);
        $_SESSION['id'] = $row['aid'];
        $_SESSION['admin'] = $row['aname'];
        $_SESSION['password'] = $row['apass'];
    } else {

You do not need escape_string on the password value…
MD5 contains no harmful characters to the DB.