2

I am very new to mysqli earlier i am writing queries in mysql but mysqli is more advanced so, i am first time using it. Below is my php code.

    function clean($str) {
        $str = @trim($str);
        if(get_magic_quotes_gpc()) {
            $str = stripslashes($str);
        }
        return mysql_real_escape_string($str);
    }

        $email = clean($_POST['email']);
        $password = clean($_POST['password']);
        //$password =md5($password);


    if(empty($res['errors'])) {
        $result = $mysqli->query("SELECT uid FROM users where email='$email' and password = '$password'");
        if($result->num_rows == 1){
            $res['success'] = true;
        }
        else{
            array_push($res['errors'], 'Invalid login details');
            $res['success'] = false;

        }
    }else{
        $res['success'] = false;        
    }
    echo json_encode($res);
}

clean function is not working as expected because sql queries return false if i enter username and password correct. So, it seems like this is not valid in mysqli case.

I checked this link PHP MySQLI Prevent SQL Injection and got to know that we have to prepare query.

I can see there is an example but i am not able to understand how to prepare/bind if i have to use two or more form data.

Thanks for your time.

Updated code

$result = $mysqli->prepare("SELECT uid FROM users where email=:email and password = :password");
        $result->execute([
':email' => $email,
         ':password' => $password]);
        //$result->execute();
        if($result->num_rows == 1){
        //if(mysqli_num_rows($result) === 1) {
            $res['success'] = true;

        }
        else{
            array_push($res['errors'], 'Invalid login details');
            $res['success'] = false;

        }
Community
  • 1
  • 1
Roxx
  • 3,738
  • 20
  • 92
  • 155
  • use redbeanphp which is built on pdo. – jewelhuq Jan 02 '16 at 14:16
  • reason is, you're mixing MySQL APIs, you can't do that if you're still using `return mysql_real_escape_string($str);` with your present/new code. Just get rid of all that and use a prepared statement. Plus, we don't know if there are any values in your POST arrays since you didn't post your HTML form. – Funk Forty Niner Jan 02 '16 at 14:24
  • we also don't know which API you're (now) using to connect with. `mysql_`? `mysqli_`? PDO? Other? Then you have `if($result->num_rows == 1){` that is `mysqli_` syntax and your `where email=:email and password = :password` is PDO. Again, they do NOT intermix. – Funk Forty Niner Jan 02 '16 at 14:28

4 Answers4

5

As already stated in comments, you need to be consistent with your API choice. You can't mix APIs in PHP.

You started out with mysqli_*, so I'll continue with that. You had some mysql_* and PDO in there, and it might not be a bad idea to use PDO over mysqli_* - but if your server supports mysqli_*, there is nothing wrong with using that. See Choosing an API and decide for yourself (just stay away from mysql_*, it's outdated).

Using mysqli_*, you connect to the database like this (you didn't show your connection).

$mysqli = new mysqli("host", "username", "password", "database");
if ($mysqli->connect_errno) {
    echo "Failed to connect to MySQL: (".$mysqli->connect_errno.") ".$mysqli->connect_error;
}
$mysqli->set_charset("utf8");

As for preventing SQL injection in it self, all you need is to use prepared statements. You can still clean or sanitize your data if there are some kind of values you don't want sitting in your tables - but that's kind of another discussion.

You also need to know if your passwords are hashed in the database. They really should be, and you should be using password_hash($password, $algorithm) and password_verify($password, $hash) if you're on PHP5.5 and above (if not, look into something like password_compat).

You need to be consistent with your hashes too, you can't insert it with md5 and selecting it with no hash. It all needs to be the same. Because if you are selecting an md5 hash, and comparing it to an unhashed string, they will be different, and the query fails.

I'm showing you an example of using password_verify(), so that means that the password stored in the database will also need to be stored with password_hash() (or your query fails).

if ($stmt = $mysqli->prepare("SELECT uid, password FROM users where email=?")) {
    $stmt->bind_param("s", $_POST['email']);           // Bind variable to the placeholder
    $stmt->execute();                                 // Execute query
    $stmt->bind_result($userID, $password);         // Set the selected columns into the variables
    $stmt->fetch();                                   // ...and fetch it
    if ($stmt->num_rows) {
        if (password_verify($_POST['password'], $password)) {
            // Password was correct and matched the email!
        } else {
            // Password was incorrect...
        }
    } else {
        // Accountname not found
    } 
}

This is just a basic example, but it will get you started. Never trust user input, use prepared statements.

Qirel
  • 25,449
  • 7
  • 45
  • 62
3

You can bind more variables like so:

$stmt = $mysqli->prepare("SELECT uid FROM users where email= ? and password = ?");
$stmt->bind_param('ss', $email, $password);

/* execute prepared statement */
$stmt->execute();

As you can see, you can expand on the bind_param() function. You can also add different type of variables:

i   corresponding variable has type integer
d   corresponding variable has type double
s   corresponding variable has type string
b   corresponding variable is a blob and will be sent in packets

From: http://php.net/manual/en/mysqli-stmt.bind-param.php

Dacaspex
  • 669
  • 7
  • 15
2

First of all, I suggest you learn PDO instead of MySQLi, just because it supports more drivers.

Second, you use mysql_real_escape_string, as you might see, that is a MySQL function, not a MySQLi function.

So where you have:

$result = $mysqli->query("SELECT uid FROM users where email='$email' and password = '$password'");

You should do something like:

<?php
$stmt = $dbConnection->prepare("SELECT uid FROM users where email = :email AND password = :password");
try{
$stmt->execute([
    ':email' => $email,
    ':password' => $password
]);
}
catch(Exception $e){
    echo $e->getMessage(); //Remove when putting online
}

if($stmt->num_rows){
    $res['success'] = true;
}
?>
Tom
  • 606
  • 7
  • 28
  • Thanks for your answer. First i will learn mysqli then i will move to PDO. Voted up. – Roxx Jan 02 '16 at 13:54
  • @404 If this helped you, please click the button under the V button to mark it as an answer. It will help other people in the future as well. I also want to note that you don't have to use the clean() function with this anymore as far as I know. – Tom Jan 02 '16 at 13:58
  • Can you please advise me how to use email and password both in query. you gave me example of email only. – Roxx Jan 02 '16 at 14:03
  • Just add the same as for email, only change the words to password, in the query and in the execution. – Tom Jan 02 '16 at 14:08
  • I tried but not working. i am updating the code in question. – Roxx Jan 02 '16 at 14:22
  • Try my updated code and tell me if you get an error. – Tom Jan 02 '16 at 14:29
2

You're presently mixing MySQL APIs/functions with mysql_real_escape_string(), then num_rows and then a PDO binding method where email=:email and password = :password which seems to have been taken from another answer given for your question.

  • Those different functions do NOT intermix.

You must use the same one from connection to querying.

It looks like you're wanting to setup a login script. I suggest you use the following and pulled from one of ircmaxell's answers:

Pulled from https://stackoverflow.com/a/29778421/

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
}

It's safer and uses a safe password hashing method, rather than what you seem to want to use is MD5 $password =md5($password); and is no longer considered safe to use now.

References:

Sidenote: If you do go that route, remember to read the manuals and that your password column is long enough to hold the hash. Minimum length is 60, but they recommend 255.

It is also unclear if your HTML form does have name attributes for the POST arrays, so make sure the form is using a POST method.

I believe I have given you enough information to get started.


What you must NOT do, is to use the above with your present code and simply patching it. You need to start over.


Add error reporting to the top of your file(s) which will help find errors.

<?php 
error_reporting(E_ALL);
ini_set('display_errors', 1);

// rest of your code

Sidenote: Displaying errors should only be done in staging, and never production.

Community
  • 1
  • 1
Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141
  • what is the draw back if we create own hasing function? – Roxx Jan 03 '16 at 16:50
  • @404 it's just more work really. Why not just use the functions that are already written, tried-tested-and-true? ;-) and you may end up creating code that could be broken by a hacker. – Funk Forty Niner Jan 03 '16 at 17:05