2

Considering a project where will work more than one developer and that will receive constant update and maintenance, what of the two codes below can be considered the best practice in PHP, considering Readability and Security? If we talk in performace, the second option will probably be a little better, but there are ways to solve this point.

Option 1

$user = $_POST['user'];
$pass = $_POST['pass'];

// Prevent SQL Injection and XSS
$user = anti_injection($user);
$pass = anti_injection($pass);

if(strlen($user) <= 8 && strlen($pass) <= 12)
{
    // SQL query
    $sql = "SELECT id 
            FROM users 
            WHERE username = '$user' AND password = '$pass';";
}

Option 2

// Retrieve POST variables and prevent SQL Injection and XSS
$_POST['user'] = anti_injection($_POST['user']);
$_POST['pass'] = anti_injection($_POST['pass']);

if(strlen($_POST['user']) <= 8 && strlen($_POST['pass']) <= 12)
{
    // SQL query
    $sql = "SELECT id 
            FROM users 
            WHERE username = '" . $_POST['user']. "' AND password = '" . $_POST['pass'] . "';";
}

EDIT 1

I am not using MySQL, my database is PostgreSQL

Marcio Mazzucato
  • 7,849
  • 6
  • 60
  • 75
  • 4
    If you're interested in security, why aren't you using bound parameters? – andrewsi Jul 13 '12 at 14:46
  • The two are practically the same but look ugly to me but i'm a crazy coder, i abstract and do stuff a little too over the edge some time... – Mathieu Dumoulin Jul 13 '12 at 14:49
  • 1
    I would always use the first option if only to ensure that I never forget to call the `anti_injection` thing on the values. The second option somehow magically infers that it is happening, but you can never be sure. Also, I would have been sure that the first is faster as array access is probably slower than accessing a variable – are you sure about the performance? – poke Jul 13 '12 at 14:50
  • @poke when compiled to opcode, array access and variable access shouldnt be significantly different, unless you do a 1 billion loop to access 1 billion elements. – Mathieu Dumoulin Jul 13 '12 at 14:53
  • It may be difficult to comment thoroughly on this for a couple of reasons. For one, you have a UDF that you don't show the definition of. One thing you might want to look into is [http://us2.php.net/manual/en/book.pdo.php](PDO), which is a "data-access abstraction layer" that can be used with multiple RDBMS's, and allows things like parametrized queries, transactions, and many many other useful things. –  Jul 13 '12 at 14:54
  • 1
    Upvoted because you are taking the time to explore security for your code. (But that being said don't use either) – Mahn Jul 13 '12 at 14:58
  • Also, the article you linked from Google has several things wrong. Memory consumption 1) doesn't make your script slower and 2) doesn't automatically double when you assign the contents of a variable to another variable (because of the copy-on-write mechanism) – Mahn Jul 13 '12 at 15:04
  • You should really check out [StackExchange Security](http://security.stackexchange.com/) - it's an awesome resource for learning how to properly manage security in your code. – Polynomial Jul 13 '12 at 15:05
  • @andrewsi, It is part of an old code, the in the new one we will use PDO. – Marcio Mazzucato Jul 13 '12 at 15:07
  • @poke, I think like you about use new vars to ensure that they have been scanned. I've putted a link to Google where i found the information about performance. – Marcio Mazzucato Jul 13 '12 at 15:09
  • @palintropos, Thanks for your suggestion, in the new version we will use PDO. – Marcio Mazzucato Jul 13 '12 at 15:10
  • I provided an example of a clean, secure, and easy way to implement your sql. Please scroll down! – Developer Jul 13 '12 at 15:11
  • @Mahn, I have another question where i discuss about performance, [you can see it here](http://stackoverflow.com/questions/11417010/). – Marcio Mazzucato Jul 13 '12 at 15:11
  • @Polynomial, Thank you very much, it's really a very useful content! – Marcio Mazzucato Jul 13 '12 at 15:12
  • @MarcioSimao I've updated my answer to cover your postgres requirements. – Polynomial Jul 13 '12 at 15:14

5 Answers5

7

Don't do either.

I can only assume anti_injection is some sort of custom filtering function, which is a Bad Idea™. If you really want to adopt either of these idea, you should be using mysql_real_escape_string.

The only way to remain secure when writing SQL queries is to use parameters, e.g. through MySQLi. The mysql_* functions are becoming deprecated anyway, so you're best to move across as soon as possible.

In fact mysql_real_escape_string is not a foolproof defense against injection attacks. Consider an integer comparison in a query, such as WHERE $var > 30. I could inject 1=1 or 100 into $var successfully, and completely break the logic.

Parameters completely separate data from query language, completely mitigating the injection risk. The server receives a query containing parameter notation, and a set of values to insert, so it can handle the query language and data completely differently.

Furthermore, you seem to be storing passwords in plaintext. This is a bad idea. You should look into a strong password storage hash algorithm such as bcrypt, which makes it very difficult to obtain plaintext passwords from the hashes.

MD5 and SHA1 are not ideal for password storage, because they are designed to be fast, meaning an attacker can quickly crack even strong salted passwords. Modern GPUs can achieve 5 billion MD5 hashes per second. In fact, there are people with dedicated hash cracking rigs, some of which can crack MD5 at 45 billion hashes per second.

You should also take a look at these awesome questions, which completely cover SQL injection attacks, password storage, and a multitude of other security issues:

Update: You mentioned you're using postgres. You can use PDO to run parameterised queries from PHP, as described briefly here.

Community
  • 1
  • 1
Polynomial
  • 25,567
  • 8
  • 75
  • 106
3

Neither of these options is a best practice, if only for the dubious anti_injection which is almost certainly not working as advertised. There's also the matters of:

  • munging your input data before validating them
  • storing plaintext passwords
  • constructing SQL queries manually instead of using bound parameters

Depending on the scope of the project the last one might be acceptable.

Regarding performance, the first option is going to be theoretically faster because it does less array indexing. But the difference would definitely be so small as to not be observable at all. The first option is also more readable and provides better abstraction, all for just a negligible amount of extra memory.

Jon
  • 396,160
  • 71
  • 697
  • 768
2

Both are wrong. You appear to be storing the password as plain text, which is just asking for trouble.

Also, if my username is "123456", I would be unable to log in because you would escape it to \"123456\" and that would fail the "length <= 8" check.

Niet the Dark Absol
  • 301,028
  • 70
  • 427
  • 540
1

I would suggest using PDO if you want security and performance. You cannot sql inject when using parameters. Below is an example, you need to "define" the mysql params for the below to work.

This also allows for the database to cache your query, since it won't change every time you execute with a different parameter which will increase performance as well.

:p_user and :p_pass

is used to denote the parameters and 

array ( ':p_user' => $user, ':p_pass' => $pass ' )

sets the parameters to the values you need to pass in.

You should also consider adding a password salt, and storing the sha1 of that in the databased, so that if your database is compromised, your passwords are not clearly revealed to the hacker.

class users{ 
  function __construct() {
    define('MySQLHost', 'localhost');
    define('MySQLName', 'databasename');
    define('MySQLUser', 'username');
    define('MySQLPass', 'password');
    define('pwsalt', 'tScgpBOoRL7A48TzpBGUgpKINc69B4Ylpvc5Xc6k'); //random characters
  }

  static function GetQuery($query, $params)
  {
    $db = new PDO('pgsql:host='.MySQLHost.';dbname='.MySQLName.';', MySQLUser, MySQLPass);
    $cmd = $db->prepare($query);
    $cmd->execute($params);

    if($cmd->rowCount()==0)
      return null;
    return $cmd->fetchAll();
  }

  static function GetUser($user, $pass)
  {
    $query = "select id
      from `users`
      where username = :p_user and password = :p_pass";

    $rows = users::GetQuery($query, array(
      ':p_user' => $user,
      ':p_pass' => sha1($pass.pwsalt) //Append the salt to the password, hash it
    ));

    return $rows;
  }
}

$user = $_POST['user'];
$pass = $_POST['pass'];
if( strlen($user) <= 8 && strlen($pass) <= 12 )
{
  $result = users::GetUser($user, $pass);
  if($result != null)
    print 'Login Found';
}
Developer
  • 1,963
  • 11
  • 11
0

While I agree with the other posters, don't try to "reinvent the wheel" regarding SQL security, the question seems to be about performance and how to use the superglobals.

As best practice, do NOT mutate the superglobals ($_GET, $_POST, $_REQUEST, $_SYSTEM, etc.) I can't think of a single example where violating this rule would improve your performance, and any modification will cause uncertainty and confusion down the road.

So in this case, neither option is correct. Option1 copies a variable needlessly (a no-no according to the Google performance docs). Option 2 mutates the superglobals, which violates the maxim above. Instead do something like:

$user = anti_injection( $_POST['user'] );
$pass = anti_injection( $_POST['pass'] );

if( strlen($user) <= 8 && strlen($pass) <= 12 ) ...

However I should reiterate that "homemade sanitation" is a frightening prospect, the other commentors have elaborated quite thoroughly on this point.

Mr Griever
  • 3,924
  • 2
  • 18
  • 39