password reset being bypassed, why?

[moose-en-a-gant]

I've used this update statement before, even with parameter binding, something easy is off...

 

I'm trying to update a hash knowing a person's user name and email combination, this is not ideal I realize or safe. I actually generate a unique random hash per person who registers, I tell them to remember this eg. keep the email.

 

I don't know why the update statement is being bypassed.

 

So they enter username, email associated with username, new password. New password is hashed, replaces old one, redirect.

 

I've been echoing stuff just to see the flow of the code, what is being executed and what isn't.

if(empty($errors)){

$userrname = test_input($_POST['userrname']);
$email = test_input($_POST['email']);
$newpassword = test_input($_POST['newpassword']);

$hash = password_hash($newpassword, PASSWORD_BCRYPT, array("cost" => 9));

$stmt = $link->prepare("SELECT username,hash FROM User where username=? And email=?");
$stmt->bind_param("ss",$userrname,$email);

if($stmt->execute())
{
$stmt->bind_result($username_from_db,$hash_from_db);
    if($stmt->fetch()){
        $_SESSION['user']=$username_from_db;
    $query = "UPDATE User SET hash=$hash WHERE email=$email And username=$username_from_db";
    if($result=$link->query($query)){
    $_SESSION['status_message'] = "Password has been reset";
    }
    
    }else {
    echo "no good";
    }
    $host  = $_SERVER['HTTP_HOST'];
        $uri   = $_SERVER['REQUEST_URI']; // the path/file?query string of the page
        
        header("Location: newlocation.com");
        exit;
        $link->close();
}

Edited February 2, 2015 by moose-en-a-gant

[Ch0cu3r]

You need to be wrapping the values in quotes within your query

$query = "UPDATE User SET hash='$hash' WHERE email='$email' And username='$username_from_db'";

But still you should still be using a prepared query.

[moose-en-a-gant]

hello Ch0cu3r

 

Thanks for your response.

 

Does the same problem of quotes apply for prepared?

 

I mean is it

$stmt = $link->prepare("UPDATE User SET hash=? WHERE email=? AND username=?");
$stmt->bind_param("sss",$hash,$email,$username);

if($stmt->execute()) ... 

Or do the question marks also get single quotes around them ?

 

This is an update with prepared query I currently have on one website that functions properly

$stmt=$link->prepare("UPDATE Repairs SET expedited=expedited+1 where repco=?");
$stmt->bind_param('i',$_SESSION["value"]);

I have problems knowing the difference between single quotes and double quotes RTFM haha someone said

 

I'm also concerned about the variable $stmt being used more than once as well as the $link, I was helped to set up a dbcon.php file which has the login information for a sql database and this is "required once" when you use required once, can it be called more than once? I mean once you declare a $link = new mysqli... can that be used over and over again?

 

See I didn't think so, so I did it twice, declare two different $stmts eg. $stmt and $stmt2 and $link, $link2... not sure if that is unnecessary

 

Thanks for the help

[maxxd]

Your prepared statement is correct - you don't need to put the quotes around the question marks.

 

Single vs. double quotes is a deceptively large conversation... When are where to use either or both is largely a question of taste, though there are some hard and fast rules to remember. For the base of it, remember that you can't mix and match the two, but you can nest them. So, if your string assignment starts with a single quote, it has to end with a single quote. That string can include double quotes without problem, but any single quotes in that string will need to be escaped. The same goes with nesting double quotes inside double quotes. Also, php will parse most variables contained within a string using double quotes, but will not when using single quotes. Hopefully this will make some sense...

$string = 'This starts with a "single quote" and ends with it, too...'; //this is fine
$string = 'This starts with a 'single quote' and will cause your script to barf'; //nope
$string = "This starts with a 'double quote' and ends with it, too..."; //also fine
$string = "Look! more "barfing scripts"!"; //still nope
$string = 'Starting with a single quote means I\'ll have to escape any other single quotes inside this string.'; //note the backslash before the single quote - that's necessary
$string = "Same thing with \"double quotes\""; //again, escaping...

$variable = "Hidee ho, matey!";
$string = '$variable will not parse...'; //contains the string $variable will not parse...
$string = "$variable will parse..."; //contains the string Hidee ho, matey! will parse...

As for the question about $link, yes, you can use the same connection multiple times. If you need to run several different queries, you should use the same connection object and pass the different query strings to that connection object. If you're concerned about multiple instances of your database connection object being created and used, you can look at using a Singleton pattern for the the connection object, which theoretically will enforce there being at most one instantiated object of the class at a time. Now, you could have an issue if $stmt gets overwritten before you're done using the original value. For that, you just need to format your code and approach logic trips carefully. Make sure you've gotten all the use you need out of the original $stmt query before you write a different value to that variable. Some folks will argue that using the same variable names leads to confusion within a script, and I agree with that, most of the time. However, other times it makes perfect sense to do so, so just think about what you're doing, and document your code like it was your second job.

[moose-en-a-gant]

Hey maxxd,

 

Thank you for that thorough explanation.

I don't really know how to express my gratitude or sincere appreciation but most if not all of my code has pieces of everyone's contribution.

 

So whatever I amount to with the code I "write", it would be all thanks to your guys' help, so thanks.

[moose-en-a-gant]

Oh man this is still not working... I'll have to work on this, I'm like retardedly side tracked... working on so many things.

 

This seems really simple I don't know what I'm overlooking besides not actually "learning" anything, it's a good thing I'm going to build the rapid website prototyper that has all of these things pre-written hehe, once I figure it out haha.

if(empty($errors)){

$userrname = test_input($_POST['userrname']);
$email = test_input($_POST['email']);
$newpassword = test_input($_POST['newpassword']);

$hash = password_hash($newpassword, PASSWORD_BCRYPT, array("cost" => 9));

$stmt = $link->prepare("SELECT username,hash FROM User where username=? And email=?");
$stmt->bind_param("ss",$userrname,$email);

if($stmt->execute())
{
$stmt->bind_result($username_from_db,$hash_from_db);
    if($stmt->fetch()){
        $_SESSION['user']=$username_from_db;
    $query = "UPDATE User SET hash='$hash' WHERE email='$email' And username='$username_from_db'";
    if($result=$link->query($query)){
    $_SESSION['status_message'] = "Password has been reset";
    }
    
    }else {
    echo "no good";
    }
    $host  = $_SERVER['HTTP_HOST'];
        $uri   = $_SERVER['REQUEST_URI']; // the path/file?query string of the page
        
        header("Location: http://www.parsemebro.com/userpanel.php");
        exit;
        $link->close();
}
 
 }

This current setup, the hash update is bypassed, redirected to userpanel

[scootstah]

One of the best pieces of advice that I can give to beginners, is to make a habit of formatting your code properly. Your random/inconsistent spacing and indentation makes it hard to follow your code. You should always use consistent styling everywhere in your code, because it makes maintaining that code much easier later on. Plus, when you need other people to read it (like right now), it makes it that much easier for those people. Here is your code cleaned up:

if (empty($errors)) {
    $userrname = test_input($_POST['userrname']);
    $email = test_input($_POST['email']);
    $newpassword = test_input($_POST['newpassword']);

    $hash = password_hash($newpassword, PASSWORD_BCRYPT, array("cost" => 9));

    $stmt = $link->prepare("SELECT username,hash FROM User where username=? And email=?");
    $stmt->bind_param("ss",$userrname,$email);

    if ($stmt->execute()) {
        $stmt->bind_result($username_from_db,$hash_from_db);

        if ($stmt->fetch()) {
            $_SESSION['user']=$username_from_db;
            $query = "UPDATE User SET hash='$hash' WHERE email='$email' And username='$username_from_db'";

            if ($result=$link->query($query)) {
                $_SESSION['status_message'] = "Password has been reset";
            }
        } else {
            echo "no good";
        }

        $host  = $_SERVER['HTTP_HOST'];
        $uri   = $_SERVER['REQUEST_URI']; // the path/file?query string of the page
            
        header("Location: http://www.parsemebro.com/userpanel.php");
        exit;
        $link->close();
    }
}

Now that that's out of the way... Your problem should be pretty obvious. There are two most likely culprits here: either the query is failing, or the query does not contain the correct data (matching the wrong rows/no rows).

 

So, step one is to make sure that the query actually executed without errors. If we look at the manual for mysqli::query, we can see that it returns FALSE when the query fails. So, you can check if the query failed and then show an error.

if (($result=$link->query($query)) !== false) {
    $_SESSION['status_message'] = "Password has been reset";
} else {
    echo 'Query failed!<br />';
    echo $link->error;
}

If the query is successfully executing, then the next most likely problem is that the query is not matching any rows, or not matching the rows that you expect it to. In that case, you should print out the $query variable and see exactly what the query is that is executing.

 

One thing to note: you are using proper prepared statements in one spot, but are open to SQL injection in another spot. Bad!

Edited February 3, 2015 by scootstah

[moose-en-a-gant]

Thanks, the random indenting is because I have been using random editors, Kate, Bluefish, Cpanel... so yeah... sort of a "fix, look, works?, no, fix, look, works? ..." thing

 

I did do a var_dump in the past and it did return false.

 

I'm going to look at this more.

 

Thanks for your help and the great advice.

[scootstah]

If it returned false then you should be able to get an error message with $link->error right after the query call.

[moose-en-a-gant]

Yeah I deleted that one, I made two different versions the lazy one and the correct one eg. using the prc <- password reset code that I randomly generated and stored per user which I email to them upon the account creation. My friend asked me if he could reset his password, obviously a feature I intended to implement but have not gotten to.

 

So, not doing the email version anymore, do something like reset my password, they enter their user name and prc code, if this matches any rows in the database, ask them for new password.

Edited February 3, 2015 by moose-en-a-gant

[moose-en-a-gant]

Well the problem is the link

 

I know repetition is bad, I am not happy being half ass, this works though but I don't intend to keep this habit of half ass, I guess it's one of those things that follows you from when you're younger... don't have to try hard, put in less effort, then things start to get more challenging, more important, and you still half ass... god how many drone platflorms have I destroyed because of my half ass soldering, or finding random points on the circuit board to attach an antenna too only to laugh at my own stupidity as the aircraft loses control and crashes... a simple $20.00 fix becomes $500.00

 

anyway this is the code as is, just the top php it is a mess

<?php
session_start();

mysqli_report(MYSQLI_REPORT_OFF);
error_reporting(E_ALL);
ini_set('display_errors', TRUE);
error_reporting(-1);

require_once(dirname(__FILE__) . DIRECTORY_SEPARATOR.'password_compat-master/lib/password.php');

function test_input($data) {
  $data = trim($data);
  $data = stripslashes($data);
  $data = htmlspecialchars($data);
  return $data;
 
}

$servername = " ";
$username = " ";
$password = " ";
$dbname = " ";

$link = new mysqli("$servername", "$username", "$password", "$dbname");

if($_SERVER['REQUEST_METHOD']=='POST'){

 $errors = array();
 
  if (empty($_POST['resetcode'])) {
     $errors['resetcode'] = "error";
   } else {
     $resetcode = test_input($_POST['resetcode']);
   }

if (empty($_POST['newpassword'])){
$errors['newpassword']="error";
}else {
$newpassword = test_input($_POST['newpassword']);
}

if(empty($errors)){

$resetcode = test_input($_POST['resetcode']);
$newpassword = test_input($_POST['newpassword']);

$hash = password_hash($newpassword, PASSWORD_BCRYPT, array("cost" => 9));

$stmt = $link->prepare('SELECT username FROM User Where prc=?');
$stmt->bind_param('s',$resetcode);

if($stmt->execute())
{
$stmt->bind_result($username_from_db);
    if($stmt->fetch()){
    $_SESSION['user']=$username_from_db;

    $servername = " ";
    $username = " ";
    $password = " ";
    $dbname = " ";

    $link2 = new mysqli("$servername", "$username", "$password", "$dbname");
    
    $stmt2 = $link2->prepare('UPDATE User SET hash=? Where prc=?');
    $stmt2->bind_param('ss',$hash,$resetcode);
    if($stmt2->execute()){
      $host  = $_SERVER['HTTP_HOST'];
        $uri   = $_SERVER['REQUEST_URI']; // the path/file?query string of the page
        
        header("Location: http://parsemebro.com/userpanel.php");
        exit;
        $link->close();
    }
    
    }
}
}
 
 }
?>

[scootstah]

You're opening up two connections to the same database in the same script. There's absolutely no reason to be doing that. What led you to that conclusion? What is the problem with $link?

[moose-en-a-gant]

If I only use one, it doesn't work, the $stmt2 is false

 

I'm not saying this is correct, I'm using it because it works.

 

This is not one of my "top projects" and I realize that great things are built from the basic building blocks so... if the building blocks are shit then the bigger project is shit.

 

The answer to your question, I am not doing it the best way, I did that because it worked.

 

Quick helped me with setting up dbcon.php which has the server name, username, etc... and that worked but I think I only used it once...

[scootstah]

What does "doesn't work" mean? Are you getting an error? Having to open a new database connection every time you execute a query is just a ludicrous waste of server resources.

 

Before you said that $link->query was returning false. Did you do what I suggested and look at $link->error? Was there a query error?

[moose-en-a-gant]

I don't know what this means : $link->error

 

Is that literal, like type that in or var_dump($stmt); or what?

 

By "doesn't work" I mean it does not connect to the database so the query fails, hence I open a new one and then it works, the query is fine and updates the hash by the password reset code

[JackTheRipper]

Holy crap. That was hard to read.

<?php
session_start();

mysqli_report(MYSQLI_REPORT_OFF);
error_reporting(E_ALL);
ini_set('display_errors', TRUE);
error_reporting(-1);

require_once(dirname(__FILE__) . DIRECTORY_SEPARATOR . 'password_compat-master/lib/password.php');

function test_input($data) {
	$data = trim($data);
	$data = stripslashes($data);
	$data = htmlspecialchars($data);
	return $data;
}

$servername = " ";
$username = " ";
$password = " ";
$dbname = " ";

$link = new mysqli($servername, $username, $password, $dbname);

if($_SERVER['REQUEST_METHOD'] == 'POST') {

	$errors = array();
 
	if(empty($_POST['resetcode'])) {
		$errors['resetcode'] = "error";
	} else {
		$resetcode = test_input($_POST['resetcode']);
	}

	if(empty($_POST['newpassword'])) {
		$errors['newpassword'] = "error";
	} else {
		$newpassword = $_POST['newpassword'];
	}

	if(empty($errors)) {

		$resetcode = test_input($_POST['resetcode']);
		$newpassword = $_POST['newpassword'];

		$hash = password_hash($newpassword, PASSWORD_BCRYPT, array("cost" => 9));

		$stmt = $link->prepare('SELECT username FROM User WHERE prc = ?');
		$stmt->bind_param('s', $resetcode);
		$stmt->execute();
		$stmt->store_result(); // You need store result or nothing will be called and displayed

		if($stmt->num_rows) {

			$stmt->bind_result($username_from_db);
			if($stmt->fetch()) {

				$_SESSION['user'] = $username_from_db;

				$stmt2 = $link->prepare('UPDATE User SET hash = ? WHERE prc = ?');
				$stmt2->bind_param('ss', $hash, $resetcode);

                                // Why do you need these lines if you aren't going to use it??
				$host  = $_SERVER['HTTP_HOST'];
				$uri   = $_SERVER['REQUEST_URI']; // the path/file?query string of the page


				$link->close(); // Close before you exit. If you exit, PHP will ignore everything after the exit which means you aren't manually closing your connection anymore

				header("Location: http://parsemebro.com/userpanel.php");
				exit;

			}
		}
	}
}
?>

The inconsistency made it super hard to read. I highly highly highly highly suggest you start formating your code a little better. You're going to have a hard time reading your own code. Why make it harder for yourself if you're suppose to know what to do? I fixed a little bit for you. I haven't tested it so you'll have to do it yourself.

 

Here's a huge question. Why are you modifying a user's password? You should NEVER ever modify a user's input password. What if they use special characters to make their password stronger? You're making a secure future like bcrypt less secure by not allowing users to use special characters.

 

SQL Injection comes from code, not from user input. If your code is written wrong, SQL injection will most likely happen. If you don't escape or use prepared statments, you're likely to be a victim of SQL Injection. However, you should NEVER modify a user's input password.

 

Are you saying that

Rat123

is more secure than

R@t123!#%^&)*

I would assume not. The first password can easily be brute forced while the second one has multiple special characters including first cap a letter and 3 numbers.

 

By "doesn't work" I mean it does not connect to the database so the query fails

 

If that's the case then I would suggest re-writing your code. When you re-write your code, you should always and I can never stress this far enough. Always debug as you go a long. If you debug at the way end. You won't know what is really wrong with your code. If you debug as you go a long, you will know exactly where the script is not running. Also, I would suggest using 1 connection because if you keep making new connections, it's a little redundant.

Edited February 3, 2015 by JackTheRipper

[moose-en-a-gant]

Here's a huge question. Why are you modifying a user's password? You should NEVER ever modify a user's input password. What if they use special characters to make their password stronger? You're making a secure future like bcrypt less secure by not allowing users to use special characters.

 

Where do you see that?

 

The test_input function?

 

I see your point, but I have used a password... ahhh so when I use one with an exclamation point, it doesn't store the exclamation point and removes it... duly noted so don't check the password part at all then? Just take it as is?

 

What about a username or anything else in general should I still apply stuff like

 

strtolower and the rest of the test_input function :

 

$data = trim($data);
  $data = stripslashes($data);
  $data = htmlspecialchars($data);

 

?

Edited February 3, 2015 by moose-en-a-gant

[moose-en-a-gant]

Here's a huge question. Why are you modifying a user's password? You should NEVER ever modify a user's input password. What if they use special characters to make their password stronger? You're making a secure future like bcrypt less secure by not allowing users to use special characters.

 

Man that was huge, thanks for that whew... haha (dumb look) hahahaha

 

Jesus, this could have perpetuated through all of my future websites, thank you very much.

Edited February 3, 2015 by moose-en-a-gant

[moose-en-a-gant]

I wonder if that explains why my hashed values are so short... well I didn't modify those but... hmm I don't get it

 

here's an example password reset code I generated using these lines of code

 

$reset = rand().rand().rand(5, 15);

$prc = password_hash($reset, PASSWORD_BCRYPT, array("cost"=>9));

 

which generates something like this

 

$2y$09$D.47EPRqwcUEgKJnAWkl6.o76kDqdFtotog649T6II3I73MDVRUQy

 

even a password isn't that long is that normal? or is it the array("cost"=>9)) ?

 

Man thank you, haha jesus what a major save you have just given me wow "Do you understand the gravity of your situation?" says John Goodman

 

[JackTheRipper]

I wonder if that explains why my hashed values are so short... well I didn't modify those but... hmm I don't get it

 

here's an example password reset code I generated using these lines of code

 

$reset = rand().rand().rand(5, 15);

$prc = password_hash($reset, PASSWORD_BCRYPT, array("cost"=>9));

 

which generates something like this

 

$2y$09$D.47EPRqwcUEgKJnAWkl6.o76kDqdFtotog649T6II3I73MDVRUQy

 

even a password isn't that long is that normal? or is it the array("cost"=>9)) ?

 

Man thank you, haha jesus what a major save you have just given me wow "Do you understand the gravity of your situation?" says John Goodman

 

Let me explain to you what I know.

 

$2y = The password algorithm

$09 = Is your cost. I would suggest sticking to the default which is 10. You're using 1 less than the default. Unless if you think it would save you resource and money then go for it.

$D.47EPRqwcUEgKJnAWkl6 = Is the salt

.o76kDqdFtotog649T6II3I73MDVRUQy = Is the hashed password.

 

When using modern password hashes, it doesn't store passwords as plain text and it makes the password more secure. Using bcrypt already escapes all of the bad stuff.

 

Here's an example of why you shouldn't be modifying user password inputs.

 

One day, person A signs up on your website. We'll call person A (Fred). So when Fred signs up with the password Y2!@%&$;s325, your old version would do something like

Y2s325

You see where that might be a problem? The user automatically thinks that their password was typed in wrong, they attempt many many hours and still no luck. They find out that their password was modified and all of the special characters that they had used was removed. Not only is this less secure, but that wasn't even the original user's password.

 

That's why you shouldn't modify user passwords because they might actually think that their password was right, but in reality. It was actually wrong since you stripped out all of their special characters out of their password.

Edited February 3, 2015 by JackTheRipper

[moose-en-a-gant]

 

$09 = Is your cost. I would suggest sticking to the default which is 10. You're using 1 less than the default. Unless if you think it would save you resource and money then go for it.

 

I did an array cost test on my old server and this was the result, I upped cores and ram so maybe I have a higher cost, will need to do the test again.

 

This is the script, I picked it up from somewhere, this also says 9 for my current server which has 4 cores, 8 GB of RAM, it's a VPS

<?php
/**
 * This code will benchmark your server to determine how high of a cost you can
 * afford. You want to set the highest cost that you can without slowing down
 * you server too much. 8-10 is a good baseline, and more is good if your servers
 * are fast enough. The code below aims for ≤ 50 milliseconds stretching time,
 * which is a good baseline for systems handling interactive logins.
 */
require_once('password_compat-master/lib/password.php');

$timeTarget = 0.05; // 50 milliseconds

$cost = 8;
do {
    $cost++;
    $start = microtime(true);
    password_hash("test", PASSWORD_BCRYPT, ["cost" => $cost]);
    $end = microtime(true);
} while (($end - $start) < $timeTarget);

echo "Appropriate Cost Found: " . $cost . "\n";
?>

I didn't intend to modify their password, it was a mistake I carried over, something I overlooked as I just assumed <- first mistake, that every input should be passed through the function test_input which is something I picked up from W3Schools.

 

Quick talked to me about this, too about the test_input function.

 

Thanks for your response and your time.

 

I added the required_once because password_hash seems to have been deprecated last time I worked with it and again I carried this over once I got it to work.

Edited February 3, 2015 by moose-en-a-gant

[scootstah]

I wonder if that explains why my hashed values are so short... well I didn't modify those but... hmm I don't get it

A hash always produces the same length value regardless of the size of the input.

 

which is something I picked up from W3Schools.

W3Schools is garbage. You should be using more up-to-date resources.

 

Also, it's good practice to not store modified data. Your test_input() function is modifying user input. You should be performing those tasks on output instead. Otherwise, you're limiting how your data could be used in the future. For example, what if, later on down the road, you decided that you wanted to allow (filtered) HTML? Well, now your database is full of broken tags and you've created a mess. Another reason is that if you escape on output instead of input, then you know that the data was escaped. The database isn't the only place to get input from.

[moose-en-a-gant]

 

W3Schools is garbage. You should be using more up-to-date resources.

 

I've heard this a lot. (Then why haven't you listened?) because I don't know... it's my Go To place to learn I guess.

 

The test_input was primarily for "injection" which I realize parameter binding, but I thought it was a standard to "modify" input for potential harmful code

 

I will be looking into the "...allow(filtered)HTML..." in the future

 

Thanks for your responses and your time

[scootstah]

I've heard this a lot. (Then why haven't you listened?) because I don't know... it's my Go To place to learn I guess.

Probably because it comes up first in Google results. Try to stick more to the PHP manual and sites like Stackoverflow (and this one!).

 

W3Schools is full of outdated and harmful practices. Avoid it like the plague.

 

The test_input was primarily for "injection" which I realize parameter binding, but I thought it was a standard to "modify" input for potential harmful code

That function does nothing to prevent SQL injection. To do that you either need to use mysqli::real_escape_string() or prepared statements.

 

Also, except for SQL injection, there is nothing else that will harm your database that you need to filter first. You do need to modify output before displaying it though. It's just usually preferred to do that as close to output as possible, rather than on input.

[moose-en-a-gant]

I don't get this

 

 

You do need to modify output before displaying it though. It's just usually preferred to do that as close to output as possible, rather than on input.

 

Can you state a circumstance that would require this?

 

I think I have one in mind from another thread of mine, or post somewhere, displaying photos only showing photos from server even if bad code got through