0

I get an error on the last line on mysqli_escape_string($hash)); by using the following code:

$hash = md5( rand(0,1000) );
$stmt = $mysqli->prepare("INSERT INTO users (username, password, hash) VALUES (?, ?, mysqli_escape_string($hash))");
$password = md5($password);
$stmt->bind_param('ss', $username, $password, mysqli_escape_string($hash));

It says, that the mysqli_escape_string($hash)) is a non-object. But using only $hash instead doesn't help either

Can someone help?

Selfish
  • 6,023
  • 4
  • 44
  • 63
Sohail
  • 61
  • 1
  • 3
  • 1
    RTM >>> http://php.net/manual/en/mysqli.real-escape-string.php `string mysqli_real_escape_string ( mysqli $link , string $escapestr )` – Funk Forty Niner Dec 08 '15 at 14:01
  • You really shouldn't use MD5 password hashes. 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). – Jay Blanchard Dec 08 '15 at 14:01
  • 2
    Parameterize the hash. `?, mysqli_escape_string($hash)` should be `?, ?`. Also don't need to escape when parameterizing. – chris85 Dec 08 '15 at 14:02
  • It is recommended that you do your own research before posting your question on SO. See https://stackoverflow.com/tour for more information. – Audite Marlow Dec 08 '15 at 14:03
  • that too @chris85 that answer below has their work cut out for them. – Funk Forty Niner Dec 08 '15 at 14:04
  • 1
    there is far too many things wrong with this code and will be extremely difficult to provide a solution. You're best finding yourself a piece of working code, and there are many out there. – Funk Forty Niner Dec 08 '15 at 14:07
  • do yourself a favor and use this, one of ircmaxell's answers http://stackoverflow.com/a/29778421/ – Funk Forty Niner Dec 08 '15 at 14:09

2 Answers2

2

Your code should be

$hash = md5( rand(0,1000) );
$stmt = $mysqli->prepare("INSERT INTO users (username, password, hash) VALUES (?, ?, ?)");
$password = md5($password);
$stmt->bind_param('sss', $username, $password, $hash);

You don't need to escape with parameterized queries.

Issues you had, your escape function was incorrect you need the object with the function when using OO approach.

$mysqli->real_escape_string($hash);

would have been what you wanted.

You also were binding that value again though which would have thrown an error and didn't set it in the variable types being passed.

A string that contains one or more characters which specify the types for the corresponding bind variables.

So

$stmt->bind_param('ss', $username, $password, mysqli_escape_string($hash));

should have had three 's's because there are three strings, and no need for the escaping.

Also md5ing passwords isn't the best practice anymore, take a look at:

Secure hash and salt for PHP passwords

https://security.stackexchange.com/questions/19906/is-md5-considered-insecure

Community
  • 1
  • 1
chris85
  • 23,846
  • 7
  • 34
  • 51
  • 1
    You *really* don't want to encourage someone to use MD5 for password hashes, do you? http://security.stackexchange.com/questions/19906/is-md5-considered-insecure The OP should really use PHP's built-in password functions. – Jay Blanchard Dec 08 '15 at 14:09
  • @chris85 Thanks a lot, really appreciated. :) And jay, I will follow your advice also, thanks. :) – Sohail Dec 08 '15 at 14:16
  • 1
    @JayBlanchard not encouraging, added notes about it as well. I prefer md5ing than plain text though, so OP has a better starting approach than a number of questions I see. – chris85 Dec 08 '15 at 14:18
  • OP got their solution and that's what's important ;-) Btw, I didn't use the links or anything else in your answer to formulate mine Chris ;-) cheers – Funk Forty Niner Dec 08 '15 at 14:19
  • Oh... I just noticed the OP chose my answer and unaccepted yours. No idea why they did that Chris. Wasn't my goal to get one. – Funk Forty Niner Dec 08 '15 at 14:20
  • @Fred-ii- no objections from me, your answer goes more into depth and provides OP the better hashing method. – chris85 Dec 08 '15 at 14:25
  • 1
    @chris85 Yours is just as informative Chris and does provide enough for them to dig deeper into researching. *Show a person how to fish...*, you know the rest, am sure ;-) *cheers* – Funk Forty Niner Dec 08 '15 at 14:26
2

There are far too many things wrong with your code and will be extremely difficult to provide a solution by fixing what you have now.

Firstly, MD5 is no longer considered safe to use for password storage.

Consult:

Plus, you're not using prepared statements correctly.

As I stated, the mysqli_escape_string() function requires a database connection be passed as the first parameter:

Do yourself a favor and use this, one of ircmaxell's answers https://stackoverflow.com/a/29778421/

Pulled from his answer:

Just use a library. Seriously. They exist for a reason.

Don't do it yourself. If you're creating your own salt, YOU'RE DOING IT WRONG. You should be using a library that handles that for you.

$dbh = new PDO(...);

$username = $_POST["username"];
$email = $_POST["email"];
$password = $_POST["password"];
$hash = password_hash($password, PASSWORD_DEFAULT);

$stmt = $dbh->prepare("insert into users set username=?, email=?, password=?");
$stmt->execute([$username, $email, $hash]);

And on login:

$sql = "SELECT * FROM users WHERE username = ?";
$stmt = $dbh->prepare($sql);
$result = $stmt->execute([$_POST['username']]);
$users = $result->fetchAll();
if (isset($users[0]) {
    if (password_verify($_POST['password'], $users[0]->password) {
        // valid login
    } else {
        // invalid password
    }
} else {
    // invalid username
}
Community
  • 1
  • 1
Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141