PHP and MySQLi login script - is it secure / am I doing something wrong?

Below is the code for my login page. I haven't really used MySQL (especially OOP) before, so I'd like to know if I'm doing something inherently wrong. The code is working as expected, but I don't trust my knowledge of SQL so much that I'd use this code publicly before asking someone to review it.

So please, tell me any and all problems you find within.

$link = new mysqli('host', 'user', 'pass', 'database');

if($link->error)
    die('Couldn\'t connect to the database. Error: '.$link->error);

if(isset($_POST['username']))
{
    if(!$stmt = $link->prepare('SELECT password FROM accs WHERE username = ?'))
    {
        echo 'Couldn\'t prepare the database query.';
        if($link->error)
            die(' Error: '.$link->error);
    }

    if(!$stmt->bind_param('s', $_POST['username']))
    {
        echo 'Couldn\'t bind parameters to the query.';
        if($stmt->error)
            die(' Error: '.$stmt->error);
    }

    if(!$stmt->execute())
    {
        echo 'Couldn\'t execute the query.';
        if($stmt->error)
            die(' Error: '.$stmt->error);
    }

    if(!$stmt->bind_result($result))
    {
        echo 'Couldn\'t bind the query results.';
        if($stmt->error)
            die(' Error: '.$stmt->error);
    }

    switch($stmt->fetch())
    {
        case NULL:
            die('Username not found.');
            break;

        case FALSE:
            echo 'Couldn\'t fetch the results.';
            if($stmt->error)
                die(' Error: '.$stmt->error);

            break;

        case TRUE:
            // I don't actually store the password in plaintext.
            if($_POST['password'] === $result)
                echo 'Logged in';
            else
                echo 'Wrong password';

            break;
    }
    $stmt->close();
}
else
    die('Error, no username specified.');

$link->close();

The only thing I could think of myself is that I should make a function for the $link->prepare(), $stmt->bind_param(), `$stmt

So after reading through your question, I noticed that you seem very concerned about the "security" of your web application. I'll do my best to share what knowledge I can muster up!

However, there are a couple key things I think need to be cleared up before we touch on the safety of your data.


I haven't really used MySQL (especially OOP)

Cool! Good to hear that you're expanding your horizons :)

But... Don't be mistaken and call your code object oriented. There's a lot more to the pattern than simple calling an object's methods.

Don't think that we can write up an entire website procedural style, and then make one call to $object->method() and have it all of a sudden be object oriented! Object orientation means a separation of concerns in different class/interfaces.

It's too hard to give you one or two links that will magically "show you the way". I'm afraid you'll just have to read and practice until you can compare your code to other OO code, and say, "Wow, my code really looks object oriented!"


I don't trust my knowledge of SQL

That's good! Don't ever publish something without having a second pair of eyes check your work.

When was the last time you wrote an essay without spell check turned on?

Okay, so, how secure is your code?

Well, from the comments, I'm sure you already realize there's something wrong with giving the user $stmt->error.

But, why is this bad?

Well, there are two main issues with it:

-
It's not user friendly. Kind of cheesy, but the guys on UX.StackExchange wouldn't be disagreeing with me. When you visit any major website, have you ever seen an error complaining the database connection failed on host 8080 on line 482? Of course not! The user doesn't need to know that information.

All the user needs to know is: how can I fix the problem. Most of the time this will just be "Try again later", sometimes it can be your browser telling you that you don't have an internet connection! Does the user need to know that the application couldn't bind the parameters to the query? What the hell does that even mean to the average user!

So now that we know all this, what should you do? Throw out all those errors that say you can bind parameters or prepare the query. All you need to tell the user is something such as: "Sorry, we couldn't execute your command" or whatever. The exact message will depend on the users, and you may need to do some A/B testing.

-
It in fact does assist any attack. Oh, so when I inject this SQL into the POST, it breaks on that step, that must mean the code is vulnerable there!

Here's a good read on StackExchange's information security site. There are tons of questions relating to your issue already. I'm not joking, there's a lot of relevant information for you! Did you ask for another sweet Q&A to read, because this one might help.

A Google can tell you a lot more than I can summarize here. A synopsis though: how much error message you show does depend on your site, but in general, it's best not to show the attacker a better way to attack!

Congratulations on discovering the security / user experience trade off!

OK, is there anything else that's insecure about this code? Well, unfortunately, nothing else pops out at me. Don't think that that means your code is safe though!

Any other comments from me? Yep!

• Avoid die(). It's really not user friendly to kill the page when things go wrong.

• Use brackets {, }! The language has them for a reason, and it was ridiculous to implement the shorthand.

• It's always weird to read lot's of logic inside a switch's case. I'd see about refactoring that so you're not doing such large operations inside the statement.

• When you secure passwords, which you will... I recommend using PHP's password_hash() utilities. It's probably the safest option for password storage at this point in time.

• Over all, I'd look at trying to implement a database access layer into your application. I'd say the most well respected is Doctrine, but of course, do what the project calls for.

Hopefully that helps :)

StackExchange Code Review Q#58735, answer score: 6