Broken PHP Login-Script

Question

I need help with my login-script - it seems to be broken. If I enter no password I still get logged in correctly. Also if I don't enter anything. But if I enter the wrong username AND password it says my login credentials were wrong.

<?php   
    $verbindung = mysql_connect("localhost", "root" , "")
    or die("Verbindung zur Datenbank konnte nicht hergestellt werden"); 
    mysql_select_db("v1nce_website") or die ("Datenbank konnte nicht ausgewählt werden"); 

    $username = $_POST["username"];
    $password = $_POST["password"];

    $abfrage = "SELECT username, password FROM logins WHERE username='$username' LIMIT 1"; 
    $ergebnis = mysql_query($abfrage);
    $row = mysql_fetch_object($ergebnis);

    if($row->password == $password) 
        {
        $_SESSION["username"] = $username; 
        echo "<p>Login erfolgreich.</p>"; 
        } 
    else 
        { 
        echo "<p>Benutzername oder Passwort waren falsch. <a href=\"index.php?p=login\">Login</a></p>"; 
        } 
?>

Any help would be appreciated.

[**Please, don't use `mysql_*` functions in new code**](http://bit.ly/phpmsql). They are no longer maintained and the [deprecation process](http://j.mp/Rj2iVR) has begun on it. See the [**red box**](http://j.mp/Te9zIL)? Learn about [*prepared statements*](http://j.mp/T9hLWi) instead, and use [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli) - [this article](http://j.mp/QEx8IB) will help you decide which. If you choose PDO, [here is a good tutorial](http://j.mp/PoWehJ). — Madara's Ghost, Dec 10 '12 at 14:43
mysql_fetch_object returns FALSE if there are no rows. A blank password in $_POST would also evalutate to false... so if(false == false)... — GarethL, Dec 10 '12 at 14:52
Please, notice that your passwords are NOT SAFE!! please read about [hash](http://php.net/manual/en/faq.passwords.php) — Ron van der Heijden, Dec 10 '12 at 15:03
To start the session you need to write `session_start()` at starting of the `php` code — Pankit Kapadia, Dec 10 '12 at 15:18

score 3 · Accepted Answer · edited May 23 '17 at 12:12

There's a lot of things going on in this script that are worrisome:

Let's start with the actual problem you are here for. If your query won't find a matching row, $row will equal false. And since therefore, $row is not an object, $row->password will evaluate to NULL. And so, if $password is an empty string $row->password == $password will evaluate to true, because NULL == "" is truthy.

You would have been notified of this, had you turned on the displaying of errors, for instance with ini_set( 'display_errors', true );, in combination with a sufficient error reporting level, error_reporting( E_ALL ); for instance.

When you enter a wrong username and a wrong password, $row->password will again be NULL, but since you entered a non-empty string for a password, this time $row->password == $password will evaluate to false.

So, to mitigate this problem you need to make certain that you first check that there is actually a matching row, before you start comparing the passwords, for instance by evaluating mysql_num_rows( $ergebnis ) first.
Your script is vulnerable to SQL injection. This means that users of your script could potentially do harm when they enter SQL hacks as values for $_POST[ 'username' ]. For instance if I were to enter ' OR 1 = 1 -- your SQL query would result in the following (formatted for display purposes):
```
SELECT username, password
FROM logins
WHERE username='' OR 1 = 1 --' LIMIT 1
```
... always resulting in at least one row if the table is non-empty, because WHERE username='' OR 1 = 1 always evaluates to true (-- in SQL signifies a comment, so ' LIMIT 1 won't even be evaluated anymore).

To mitigate this problem, in your current setup, you need to sanitize your input values first with mysql_real_escape_string(), before passing them into the SQL query, like this:
```
$username = mysql_real_escape_string( $_POST["username"] );
```
But as others have advised already as well, you'd be wiser to start using a MySQL compliant library that offers prepared statements with parametrized queries, such as PDO or MySQLi, since the mysql_* library is in the process of being deprecated, because it offers poor means of defending against SQL injection.
Your passwords are stored verbatim (as plain text) in the database. This offers a variety of potential risks of accounts (and possibly user-related accounts) being compromised. Anyone who has access to the database (be it direct, authorized, access, or when the database is compromised) can view the passwords in clear text, and could therefore use these to either log in to your site, or use it as a potential login for other sites and/or services. After all, it is not uncommon for people to use the same combination of username and password for a variety of other sites and services.

To mitigate this problem you'd be wise to hash (one-way encrypt) the passwords before storing them in the database, and then, when the user wants to log in, compare the stored hashed value with the hash (using the same hashing function again) of the user entered password. Using a unique salt per user password as an extra security measure is also strongly advised, as this protects against what is known as rainbow table attacks.

For a more thorough explanation of what the preferred hashing algorithm to use is, and why, see this answer by user Andrew Moore to this question.

score 1 · Answer 2 · answered Dec 10 '12 at 14:45

1

$abfrage = "SELECT username, password FROM logins WHERE username='$username' LIMIT 1"; 
    $ergebnis = mysql_query($abfrage);
if(mysql_num_rows($ergebnis) > 0) // you can check for one
{
    $row = mysql_fetch_object($ergebnis);
//rest code goes inside here
// redirect to any page or do whatever you like
}

answered Dec 10 '12 at 14:45

Dev

186
2
12

1

and do one thing. use mysql_real_escape_string while fetching the usernames, passwords to avoid sql injection. if possible migrate to pdo or mysqli_* function as manticore mentioned – Dev Dec 10 '12 at 14:47

score 1 · Answer 3 · answered Dec 10 '12 at 14:52

I assume you're doing the login that way to protect against SQL injections, however, mysql_real_escape_string() will quickly fix that problem. I would do something like this:

 $login = mysql_query("SELECT id FROM users WHERE username='" . mysql_real_escape_string($username) . "' AND password='" . mysql_real_escape_string($password) . "' LIMIT 1");
 if ( mysql_num_rows($login) > 0 ) {
      $user = mysql_fetch_array($login);
      $_SESSION['userID'] = $user['id'];
      echo 'Login successful!';
 } else { // login failed
      echo 'Login failed.';
 }

Notice that I'm storing the user's ID in the session instead of their username. This is usually a better idea because it's much easier to manage user activity based on a unique auto_increment index value rather than a textual username. Most sites don't, but this method would allow you to let multiple users have the same username if you wished to do so. But if you do this, make sure you're logging them in based on their email address rather than their username, because you don't want someone getting logged in as someone else because they share the same username.

Also, you should be encrypting passwords. MD5 is a simple solution and can be implemented like so:

 $password = md5($_POST['password']);

If you md5 the password input, you don't need to worry about slashes, either. Hope that helps.

@MrCode Eh, Stack Overflow, and other StackExchange sites, actually allow for this, I believe. ;-) — Decent Dabbler, Dec 10 '12 at 16:36
@fireeyedboy SE differs from most in that it's treated as more of a person's name (hence allows spaces) rather than a username/nickname/handle which traditionally don't allow spaces or duplicates. — MrCode, Dec 10 '12 at 17:11

score 1 · Answer 4 · answered Dec 11 '12 at 06:06

Maybe the password is not gotten properly from the database. use print_r($row) to make sure everything is good there.

Also use sha1 encription with salt to increase security. Also do:

$username = mysql_real_escape_string($_POST["username"]);
$password = mysql_real_escape_string($_POST["password"]);

to avoid injection.

Hope this helps.

Broken PHP Login-Script

4 Answers4