0

I am trying to set up a simple php login system with a mysql database and a simple php script. it seems to be working with one minor error; You can also login with the wrong credentials.

You can see it in action at rietool.roxtest.nl.

My code:

<?php
  // get values passed from form in login.php file
  $username = $_POST['username'];
  $password = $_POST['password'];

  // to prevent mysql injection
  $username = stripcslashes($username);
  $password = stripcslashes($password);
  $username = mysql_real_escape_string($username);
  $password = mysql_real_escape_string($password);

  // connect to the server and select database
  mysql_connect("my database info");
  mysql_select_db("roxtest_nl_RIEtool");

  // query the database for username
  $result = mysql_query("Select * from users where username = '$username' and password = '$password'")
            or die ("Failed to query database" .mysql_error());
  $row = mysql_fetch_array($result);
  if ($row['username'] == $username && $row['password'] == $password) {
      echo "login geslaagd, welkom";
    } else {
      echo "login mislukt, probeer opnieuw";
    }

 ?>

If anyone could help me ou, that would be greatly appreciated!

FYI I followed this tutorial: https://www.youtube.com/watch?v=arqv2YVp_3E

  • 1
    [Little Bobby](http://bobby-tables.com/) says ***[your script is at risk for SQL Injection Attacks.](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php)***. Even [escaping the string](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) is not safe! – Jay Blanchard Oct 13 '16 at 12:49
  • 1
    ***Please [stop using `mysql_*` functions](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php).*** [These extensions](http://php.net/manual/en/migration70.removed-exts-sapis.php) have been removed in PHP 7. Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [PDO](http://php.net/manual/en/pdo.prepared-statements.php) and [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and consider using PDO, [it's really pretty easy](http://jayblanchard.net/demystifying_php_pdo.html). – Jay Blanchard Oct 13 '16 at 12:49
  • 1
    **Never store plain text passwords!** Please use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). Make sure you ***[don't escape passwords](http://stackoverflow.com/q/36628418/1011527)*** or use any other cleansing mechanism on them before hashing. Doing so *changes* the password and causes unnecessary additional coding. – Jay Blanchard Oct 13 '16 at 12:49
  • 2
    Sorry, but question simply asking: "why isn't that code working" are clearly considered off-topic here. You are expected to debug yourself. Only if you have _specific_ questions, _then_ is the time to ask about those here. – arkascha Oct 13 '16 at 12:50
  • What is the "minor error"? – Jay Blanchard Oct 13 '16 at 12:50
  • 2
    logging in with wrong credentials is not a minor error, but basically a non-functional login-system. basically, you did almost everything wrong that's possible. a) don't use mysql_, it's deprecated, use mysqli_ or PDO. b) don't put data into your queries directly - learn about SQL injection. c) don't ever store passwords as plain text. use password_hash and password_verify – Franz Gleichmann Oct 13 '16 at 12:50
  • And a general hint: one _never_ stores a password in a database. Never. Period. What is stored is a _hash of a password_. Then, at authentication time, you again hash the provided password and compare both hashes. That way you do not loose your users passwords even if your system gets compromised. Indeed, some huge companies had to learn that the hard way lately. But that does not mean we should do the same stupid mistake ;-) – arkascha Oct 13 '16 at 12:52
  • 1
    Try with another lessons. This have a lot of issues. – Kristiyan Oct 13 '16 at 12:57

2 Answers2

0

First, as others already said, ur code is insecure ! Never use MYSQL_ even with _escape ! Learn how to use PDO which is more easy to use and can be more secure if u use it correctly !

In ur code, u already get :

where username = username and password = password in ur query

So it will return true or false, and u don't need to use if to compare username and password !

U just need the row count, if it's 1, the username and password is correct and if it's 0, they are incorrect !

Sinf
  • 123
  • 1
  • 4
  • 11
0

It's not entirely clear what is happening here. But my guess is that:

$username = mysql_real_escape_string('foo');

Evaluates to false. Because it relies on a database connection that you haven't yet started. Same for the password.

Then later you get an empty result set:

$row = mysql_fetch_array($result);

$row is false. ("SELECT * FROM USERS WHERE username='' AND password=''";)

Then you have:

if ($row['username'] == $username && $row['password'] == $password) {

Which equates to:

if(null == false && null == false) {

Becomes:

if(true && true) {

So we have:

if(true) {
    echo "login geslaagd, welkom";
}

So any credential will satisfy this!

Be very careful!

So to fix, make the connection before your calls to mysql_real_escape_string, or better, pass mysql_real_escape_string your database connection. You may want to check that the function doesn't return false either.

Then later make sure you have a row.

if(
      $row = mysql_fetch_array($result)
      && $row['username'] == $username
      && $row['password'] == $password
) {
    echo 'Success';
}

As mysql functions are deprecated you may wish to rewrite your example using a different database interface, and take on board some of the other feedback.

And look into error reporting and logging in Php. It would have helped you here.

Progrock
  • 7,373
  • 1
  • 19
  • 25
  • This worked, thank you so much! I will defenitely take the other feedback to heart and learn more about PDO. I'm a very junior developer and eager to learn, your feedback means a lot! – Roxanne Allard Oct 13 '16 at 13:50