i have made a change password script in PHP and it isnt working

Question

I have the following code, and it will not work. I am currently working on a simple change password feature for a system and cant get it to function correctly. i was wondering if i was overlooking a really simple solution?

<?php    
    $con = mysql_connect("localhost","root");
    if (!$con) {
        die('Could not connect: ' . mysql_error());
    }

    $username = $_POST['userid'];  
    $password = $_POST['cpword'];
    $newpassword = $_POST['pword'];
    $confirmnewpassword = $_POST['pword2'];

    $result = mysql_query("SELECT username, pword FROM login WHERE username='$username'");

    if(!$result) {
        echo "The username entered does not exist!";
    } else
        if($password != mysql_result($result, 0)) {
            echo "Entered an incorrect password";
        }

    if($newpassword == $confirmnewpassword) {
        $sql = mysql_query("UPDATE login SET pword = '$newpassword' WHERE username = '$username'");     
    }

    if(!$sql) {
        echo "Congratulations, password successfully changed!";
    } else {
        echo "New password and confirm password must be the same!";
    }       
?>

`it will not work` is not an error message. Unless you work for Microsoft. — DaveRandom, Sep 11 '12 at 14:28
Don't use the ancient `mysql_*` functions. Hash the password with a proper algorithm. — PeeHaa, Sep 11 '12 at 14:29
I hope this script isn't available EVER! Read something about hash first after that about brute forcing. — Ron van der Heijden, Sep 11 '12 at 14:29
Please, don't use `mysql_*` functions for new code. They are no longer maintained and the community has begun the [deprecation process](http://goo.gl/KJveJ). See the [**red box**](http://goo.gl/GPmFd)? Instead you should learn about [prepared statements](http://goo.gl/vn8zQ) and use either [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli). If you can't decide, [this article](http://goo.gl/3gqF9) will help to choose. If you care to learn, [here is a good PDO tutorial](http://goo.gl/vFWnC). — PeeHaa, Sep 11 '12 at 14:29
http://stackoverflow.com/questions/4795385/how-do-you-use-bcrypt-for-hashing-passwords-in-php — PeeHaa, Sep 11 '12 at 14:30
so many problems.... use `mysqli` or `pdo`. read this and learn about sanitising your inputs: http://bobby-tables.com/ . `if ($newpassword==$confirmnewpassword)` needs an else, don't test the returned `$sql` variable, it's hard to understand. — totallyNotLizards, Sep 11 '12 at 14:30
@jammypeach `don't test the returned $sql variable, it's hard to understand.` - what's hard to understand about a boolean? Couldn't be more clear cut, I would say. — DaveRandom, Sep 11 '12 at 14:32
Dont use the users inputs directly! At least do something like $username = mysql_real_escape_string(trim($_POST['userid'])); — Stefan Gi, Sep 11 '12 at 14:32
@PeeHaa: Lol, they're not that ancient. It's only recently that PHP is using PDO, instead. — user1477388, Sep 11 '12 at 14:32
To expand on what @Bondye said, look into hashing and salting - two processes to make passwords harder to extract if the database is stolen. — halfer, Sep 11 '12 at 14:32
@DaveRandom sorry i wasn't too clear - I mean it's hard to understand the purpose of that code. an else statement on the `if ($newpassword==$confirmnewpassword)` test would be much more readable. it's a bit more subjective than my other points but it took me 30 seconds longer to comprehend than if it was done this way. — totallyNotLizards, Sep 11 '12 at 14:33
@user1477388 PDO was released with PHP5.1. That's 24 Nov 2005. 7 years now. That isn't recently in my book. And `mysqli` was released with PHP5: 13 July 2004. — PeeHaa, Sep 11 '12 at 14:33
I'm joining whatever site you have, my username will be _Bobby'; DROP TABLE login; --_ [bobby](http://bobby-tables.com/) — Elias Van Ootegem, Sep 11 '12 at 14:35
@PeeHaa: It wasn't until 2011 when PHP announced they would not be using mysql anymore and suggested using mysqli or PDO Ref. http://news.php.net/php.internals/53799 That was only just last year :) — user1477388, Sep 11 '12 at 14:37
@jammypeach Oh I agree that the code is incomprehensible and that the way in which it is checked makes no sense (I'm pretty sure the messages in the `if`/`else` are the wrong way round? It's hard to tell) but my point is that *not* checking the statement execution is successful would be a bad move. — DaveRandom, Sep 11 '12 at 14:37
You know what @user1477388 do whatever you think is best, just let us know what sites you have built so we can prevent ever going to those security holes. Good bye. — PeeHaa, Sep 11 '12 at 14:38
@user1477388 It wasn't until 2011 that it was officially recommended. It has been unofficially recommended (by the people who actually *use* PHP) for a lot longer than that. — DaveRandom, Sep 11 '12 at 14:38
@DaveRandom: I used PHP everyday in my work before I recently switched to asp.net mvc. I never heard anything until they made the announcement and added it to the man page (and the forums started buzzing). — user1477388, Sep 11 '12 at 14:40
@user1477388 You weren't moving in the right circles then ;-) — DaveRandom, Sep 11 '12 at 14:42
@DaveRandom what I really meant was *rather than* but worded it poorly. $sql should be tested, but not to determine if $newpassword==$confirmpassword — totallyNotLizards, Sep 11 '12 at 14:58

score 5 · Answer 1 · answered Sep 11 '12 at 14:34

5

Ok because of this dangerous script that is going to infect the internet I advice you some.

Use hash to save passwords, Nobody want a visible password.
Never tell a user what you have in your database like the Entered an incorrect password notice tells me that I found a username so my bruteforce is 100x easier.
And what people shout all over the net is: STOP USING MYSQL Step to PDO or mysqli
Last but not least: Ever heard about mysql injections?

answered Sep 11 '12 at 14:34

Ron van der Heijden

14,803
7
58
82

Great answer. All of these tips should be used! – endyourif Sep 11 '12 at 14:38
Really a great answer :) – baig772 Sep 11 '12 at 14:49

score 0 · Accepted Answer · edited May 23 '17 at 10:33

OK there are alot of things wrong with this code so I've rewritten it to be up to date and not at all dangerous (mostly).

I'm not hashing the passwords here like you really should but you can do it quite simply with a bit of reading ( try this: Secure hash and salt for PHP passwords )

For a comprehensive list of the exact problems and solutions to those problems, take a look at @Bondye's post or the comments on the OP's question.

Disclaimer: not tested so might have a few syntax errors. this is still not fantastic but it's a much better starting point than the original code. See below for a list of what I've changed and why.

Here goes...

<?php
    $host = "localhost";
    $database = "yourdatabase";
    $username_db = "root";
    $password_db = "databasepassword";
    $con = mysqli_connect($hostname, $username_db, $password_db, $database) or die(mysqli_error($con));

    $username = $_POST['userid'];  
    $newpassword = $_POST['pword'];
    $confirmnewpassword = $_POST['pword2'];

    if($newpassword == $confirmnewpassword)
    {
        //password & password confirm match, do the update
        $query = sprintf("UPDATE login SET pword=%s WHERE username=%s",
                          mysql_real_escape_string($newpassword),
                          mysql_real_escape_string($username));                    
        $sql = mysqli_query($query, $con) or die(mysqli_error($con);     
        if($sql)
        {
            echo "Congratulations, password successfully changed!";
        } 
        else
        {
            //sql error or update didn't work?
            echo 'generic failure message';
        }        
    }
    else
    {
        //new password and confirm password weren't the same.
        echo "New password and confirm password must be the same!";
    }    

?>

Changes: Removed the query to look for a username - personal choice really but I don't see the point in querying the DB to see if the user exists when you are going to be doing an implicit search for that user in the update query. Secondly, you should have logged them in before they can access this script, so there should be no question about whether the user exists or not.

Changed mysql functions to their mysqli equivelants.

Simplified and cleaned up the mess of if tests and put validation before the query itself. This is better as you shouldn't tell your users what you have in your database as this is useful information for attackers and not at all useful for users who should already be logged in by this point.

Hope this helps and open to corrections.

i have made a change password script in PHP and it isnt working

2 Answers2