2

I am attempting to make a more secure log in system. My registration is working fine so its not a connection issue. Just needing a fresh pair of eyes to see if there are any errors that I may be missing, can anyone help please? Thanks!

login.php

session_start();

if (isset($_POST['submit'])) 
    {

        $user = $_POST['username'];    
        $pass = $_POST['password'];


        if(!($stmt = $mysqli->prepare("SELECT username, password FROM users WHERE username = ?"))){
            echo "Prepare failed: (" . $mysqli->errno . ")" . $mysqli->error;
        }
        if(!$stmt->bind_param('s', $user)){
            echo "Bind failed: (" . $stmt->errno . ")" . $stmt->error;
        }
        if(!$stmt->execute()){
            echo "Execute failed: (" . $stmt->errno .")" . $stmt->error;
        }
        $userdata = $stmt->get_result();
        $row = $userdata->fetch_array(MYSQLI_ASSOC);

        $stmt->bind_result($user, $pass);
        $stmt->store_result();

        if(password_verify($pass, $row['password'])){

            $_SESSION['login_user'] = $_POST['username'];
            header('Location: profile.php');
            exit();
        }

    }
else{
    echo "Login Failed: (" . $stmt->errno .")" . $stmt->error;
}
$stmt->close();

$mysqli->close();

index.php(The login form)

<div id="loginform">

    Log in details<br /><br />

    <form method="post" action="login.php">

        Username:
        <input type="text" name="username" />
        <br /><br>
        Password:
        <input type="password" name="password" />
        <br /><br>
        <input type="submit" name="submit" value="Submit" />
    </form>

 </div>
Barmar
  • 741,623
  • 53
  • 500
  • 612
Zach Hall
  • 27
  • 1
  • 1
  • 8
  • 1
    so is `Errno(0)` meant to mean that you get the error message from `echo "Bind failed: (" . $stmt->errno . ")" . $stmt->error;`? and if so, what is the actual error message? – Sean Dec 19 '15 at 16:19
  • What is your question? My fresh pair of eyes doesn't see anything obviously wrong. Are you getting an error? From which call? – Barmar Dec 19 '15 at 16:22
  • 'Login Failed: ()' Sorry Not 0! Doesnt give me any more information on the error – Zach Hall Dec 19 '15 at 16:22
  • The script runs and loads. Just when submit is pressed the page refreshes, and posts the message 'Login Failed: ()' – Zach Hall Dec 19 '15 at 16:24
  • 2
    Why are you using both `bind_result` and `fetch_array` to get the results? – Barmar Dec 19 '15 at 16:24
  • `'Login Failed: ()'` is in the `else {...}` of `if (isset($_POST['submit'])) {...}`. So you need to look at why your `$_POST['sumbit']` is not set. Try adding a `print_r($_POST);` or `var_dump($_POST);`. Is your form submitting the standard/traditional way, and not through ajax? – Sean Dec 19 '15 at 16:28
  • I know, I deleted my comment. – Barmar Dec 19 '15 at 16:31
  • I am submitting it the traditional way! – Zach Hall Dec 19 '15 at 16:37
  • I have have used the print_r($_POST); and it is showing the $_POST when I add it before the mysqli->prepare. Does this mean my error is within the statements?? – Zach Hall Dec 19 '15 at 16:39
  • 1
    Are you sure you copied the code accurately? Maybe that line is in the `else` for the `password_verify` check? – Barmar Dec 19 '15 at 16:39

1 Answers1

3

You shouldn't use $stmt->errno and $stmt->error in the Login Failed error message. That line is in the else clause for if (isset($_POST['submit'])), so it has nothing to do with a MySQL error. I'm not sure why you're ever getting that message, since it should only happen if you go to login.php without submitting the login form on index.php.

Since you're using $userdata->fetch_array() to fetch the row of results from the database, you shouldn't also use $stmt->bind_result() -- do one or the other. I don't think bind_result() is doing anything, since you never call $stmt->fetch() to get a row into those variables.

If you decide to go with bind_result(), you need to use a different variable for the password, because $pass contains the password submitted from the form.

You need to check whether a row was returned by the query -- if the user enters an invalid username, it will not. So the password check should be:

if ($row && password_verify($pass, $row['password']) {
    ...
}

You should then have an else clause for this that reports that the username or password was invalid. You should not distinguish whether it was the username or password that was wrong, this would be bad for security (it helps a brute-forcer to know that he guessed the username correctly, and just needs to try different passwords).

$stmt->close() should be inside the if block.

Barmar
  • 741,623
  • 53
  • 500
  • 612