0

Good afternoon.

I'm a beginner in PHP programming, followed some courses and have a theoretical knowledge about it. I've been hired now and been given some 'basic' tasks. Coworkers here tell me that 'real world' code differs a bit from what it is taught at University, in books and the like. I've been reading about security and found out about SQL-injection. I also learned that the best way to avoid them is using prepared statements and parameters bounding.

So, I'd be very thankful if you could give me your opinions or suggestions about the code below. Please, anything you have to say will be very useful. Opinions about the logic of the code, about the structure, about performance, about security... anything.

if (isset($_POST['username']) && isset($_POST['password']))
{
    $dbaddress = 'myhost';
    $dbuname = "database_user";
    $dbpass = "database_password";
    $dbname = 'customers_db';

    $r = new mysqli($dbaddress, $dbuname, $dbpass, $dbname);

    $q = $r->prepare("SELECT * FROM users WHERE uAccessName = ? AND uAccessPass = ?");
    $q->bind_param("ss", $user, $pass);

    $user = $_POST['username'];
    $pass = $_POST['password'];
    $q->execute();
    $q->store_result();

    if ($q->num_rows === 1)
        // Do many many other things here
        echo "<h1>Access granted</h1>";
    else
        echo "<h1>Access denied</h1>";

    $q->close();
    $r->close();
} else {
    // Handle the case where the form sent no data
}

Thanks a lot.

Tom
  • 9
  • 1
  • 4
    Probably better if you posted this on code review: http://codereview.stackexchange.com/ – Kisaragi Jan 18 '17 at 18:31
  • @Tom I hope you understood and I given my best answer – msk Jan 18 '17 at 18:50
  • If we're talking about "efficient" then putting `$_POST` data into `bind_param` without intermediate variables is more efficient. The good news is you're using prepared statements, that's a big step towards writing secure code. If you want to learn more, worth reading up on what [OWASP has to offer](https://www.owasp.org/index.php/Main_Page). – tadman Jan 18 '17 at 19:04
  • **WARNING**: Writing your own access control layer is not easy and there are many opportunities to get it severely wrong. Please, do not write your own authentication system when any modern [development framework](http://codegeekz.com/best-php-frameworks-for-developers/) like [Laravel](http://laravel.com/) comes with a robust [authentication system](https://laravel.com/docs/5.3/authentication) built-in. At the absolute least follow [recommended security best practices](http://www.phptherightway.com/#security) and **never store passwords as plain-text**. – tadman Jan 18 '17 at 19:04

1 Answers1

-1

Do not store the database passwords in plain text. Use a two way encryption algorithm so you can store the database password in encrypted form, but you can still use it in database queries

Also sanitize the POST variables using filter_var or similar Php function.

Nadir Latif
  • 3,690
  • 1
  • 15
  • 24