problem understanding $mysqli->real escape string

[Orangutang]

Hi,

1.

I'm trying to sanitize my data before inserting it into mysql db but it doesn't seem to work?

I'm picking up my posted variable like this:

@$first_name = strip_tags($_POST['firstname']);

Then I'm running it throungh real escape string with this:

@$first_name = $mysqli->real_escape_string($first_name);

Then echoing it out with:

echo $first_name . "<br />";

All I'm getting is a blank screen when I'm trying to prove the method by adding in special char's to the form and echoing it out to prove they have been removed.

If I remove the following it works but when I add it not only does it not echo out the first name let alone be able to check it's removing any special char's.

@$first_name = $mysqli->real_escape_string($first_name);

2.

After this I visited the php manual and ran the following script which I copied and pasted direct from the site and it doesn't error out but the temporary table is not created and nothing is echoed out, I just get a blank screen when it's meant to echo

Error: 42000
1 Row inserted.

The code from the php manual.

<?php
$mysqli = new mysqli("localhost", "xxx", "xxx", "xxx");

/* check connection */
if (mysqli_connect_errno()) {
printf("Connect failed: %s\n", mysqli_connect_error());
exit();
}
$mysqli->query("CREATE TEMPORARY TABLE myCity LIKE City");

$city = "'s Hertogenbosch";

$city = $mysqli->real_escape_string($city);

/* this query with escaped $city will work */
if ($mysqli->query("INSERT into myCity (Name) VALUES ('$city')")) {
printf("%d Row inserted.\n", $mysqli->affected_rows);
}

$mysqli->close();
?>

Thanks in advance for any help.

[swa66]

Since you're using mysqli, why not use prepared statements and be done with this escaping ?

[Orangutang]

swa66,

Thanks for the advice, eliminating sql injection is exactly what I was trying to do.

Due to this I thought I may as well start at the beginning and redo my login script which I've posted below.

It works but I'm not sure if there are any glaring issues? Interestingly I had to do 2 select statements unlike mysql, one to use num_rows and another to get the userid. I don't know if it can be done with one but I couldn't get it to work when I tried.

$mysqli = new mysqli("localhost", "xxx", "xxx", "xxx");

if (mysqli_connect_error()) {
 die('Connect Error (' . mysqli_connect_errno() . ') '
 . mysqli_connect_error());
}

// Encrypt
if ($stmt = $mysqli->prepare("SELECT * FROM `clients` WHERE `username` = ? AND `password` = ? LIMIT 1")) {

  $stmt->bind_param("ss", $username, $password);
  $stmt->execute();
  $stmt->store_result();

  if ($stmt->num_rows == 1) {

    if ($stmt = $mysqli->prepare("SELECT `userid` FROM `clients` WHERE `username` = '$username'")) {

       $stmt->execute();
       $stmt->bind_result($user_id);

       while ($stmt->fetch()) {
          echo $user_id;
       }

       $_SESSION['userid'] = $user_id;
       $_SESSION['username'] = $username;
       $sess_id = session_id();
       $_SESSION['sessid'] = $sess_id;

       header("Location: nextpage.php");
       $stmt->close();
       $mysqli->close();

[swa66]

The most glaring issue that your second query still sends the username coming from user generated input to the database.

The second glaring is that you store plain passwords. You really should not do that.

What should you store:
- a salt
- a hash of the salt concatenated with the password

Salt: just a plain random value
hash: a one way hash such as SHA-1 or one of the SHA-2 family.

when setting the password:
- generate a random string (the salt)
- calculate sha256(salt.password)
- store the salt and the hash

to verify a password:
- retrieve salt and hash from the database
- calculate sha256(salt.password)
- validate the calculate hash is the same as the stred hash, if they are the same the right password was provided

Next I'd close the first result before issuing the second one. Storing them is sensible but you're not going to use them anymore anyway, so it's less resource intensive to release them.

But you really do not need two selects to get just one and the same row.

I'd avoid a select *, always list the columns you need.

You can bind both parameters and results on the same prepared statement.

order is:
$mysqli = new mysqli(...)
$stmt = $mysqli->prepare("SELECT col1, col2 FROM table WHERE col3 = ?");
$stmt->bind_param('s', $inputvar);
$stmt->execute();
$stmt->bind_result($outputvar1, $outputvar2);
$stmt->fetch();
$stmt->close();

Just test if the fetch succeeds to know if the login exists.

[Orangutang]

Hi swa66,

I think I'm getting somewhere, I've posted my registration and login script and I'm hoping you can confirm the issues you've mentioned have been addressed.

Registration:

// Connect to db
// Other checks like is form filled out, valid email etc

$sess_id = session_id();
$_SESSION['sessid'] = $sess_id;
$salt = uniqid(mt_rand(), true);
$hashed_pw = sha1($salt . $password);

if ($stmt = $mysqli->prepare("INSERT INTO clients 
    (status, username, clientusername, salt, password, type, sessid, firstname, lastname, companyname) 
    values ('A', ?, ?, ?, ?, 'superuser', ?, ?, ?, ?)")) {

    $stmt->bind_param('ssssssss', $email, $email, $salt, $hashed_pw, $sess_id, $first_name, $last_name, $company_name );
    $stmt->execute();
    header("Location: next_page.php");
    $stmt->close();
    $mysqli->close();

Login:

// Connect to db
// Other checks like is form filled out, not empty etc 

if ($stmt = $mysqli->prepare("SELECT userid, salt, password FROM `clients` WHERE `username` = ? ")) {
   $stmt->bind_param("s", $username);
   $stmt->execute();
   $stmt->bind_result($user_id, $salt, $hashed_pw);

   if(($stmt->fetch()) == 1) {
       $entered_hashed_pw = sha1($salt . $password);

       if ($entered_hashed_pw === $hashed_pw) {

          $_SESSION['userid'] = $user_id;
          $_SESSION['username'] = $username;
          $sess_id = session_id();
          $_SESSION['sessid'] = $sess_id;
          header("Location: next_page.php");
          $stmt->close();
          $mysqli->close();

[swa66]

Looks much better.

One slight point of worry lies in the use of uniqid(mt_rand(), true) ...
It's not really a problem for a salt - but uniqid is not generating random data in terms of cryptography. Cryptographic random has terribly high requirements.
Again: not a big deal for a salt - its only purpose is to combat rainbow tables, just be careful to use a real random generator when needed. E.g. [php.net...] is as cryptographically sound as it gets (being part of a cryptographic library helps a bit).

A tip: when a user changes their password: also generate a new salt. (again minor compared to it all, but for the best results...).

I'm not sure why you store the session_id in the user table ...

[Orangutang]

Looks much better == Relief :-)

I'd like to try to implement openssl_random_pseudo_bytes() but the version of php (5.3.8) I'm running on my local host doesn't seem to have the openssl extension enabled. Thanks for the pointer though and when I upload to host I will try to utlise this function.

I'm not sure why you store the session_id in the user table ...

To be honest, I'm not sure either, the reason I started to do that was because I read it on the internet! Anyway to cut a long story short I've cancelled doing that from now on.

I'm not even sure if I need the session id at all, I create it at the moment so after login or registration I can do the following at the top on every page and nest the complete script within these if's:

if (isset($sess_id) and ! empty ($sess_id)){
   if (isset ($username) and ! empty ($username)){
   // Code here...
   }
   else ...
}
else ...

Then on sign out I run the following:

<?php
session_start();
error_reporting(E_ALL);

//Destroy all sessions
$user_id = ($_SESSION['userid']);
$username = ($_SESSION['username']);
$sess_id = ($_SESSION['sessid']);

session_destroy();
header("Location: signin.php");
?>

Would you say I'm doing to much checking? If I completely removed the session id if i'd still be using the username if.