4
\$\begingroup\$

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->execute() and $stmt->bind_result() conditionals, but that's not too big of a problem, just a design-quirk.

Oh, and if in the future I want to get more than one result from a query (i.e. a whole table) how would I know how many arguments to put into $stmt->bind_result()?

Also, is the way I handled errordetection and -notification optimal from a security and user-friendliness standpoint?

\$\endgroup\$
9
  • \$\begingroup\$ In production, dont print $stmt->error if something fails.. \$\endgroup\$
    – netigger
    Commented Aug 1, 2014 at 11:29
  • \$\begingroup\$ What would be a better alternative? \$\endgroup\$
    – nvbmnbvm
    Commented Aug 1, 2014 at 11:31
  • \$\begingroup\$ @Lubrikitten "Couldn't connect to the database. If this problem persists, please contact us at [...]" is fine. At a maximum, you can also output the error code. \$\endgroup\$
    – tim
    Commented Aug 1, 2014 at 11:39
  • 2
    \$\begingroup\$ You can also log error details and display a generic message to users (like @tim suggests). That way you don't lose information which might help you troubleshoot issues. \$\endgroup\$
    – Kolargol00
    Commented Aug 1, 2014 at 12:38
  • 2
    \$\begingroup\$ // I don't actually store the password in plaintext. In what form do you store passwords then? \$\endgroup\$
    – Kolargol00
    Commented Aug 1, 2014 at 12:40

2 Answers 2

6
\$\begingroup\$

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:

  1. 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.

  2. 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 :)

\$\endgroup\$
0
2
\$\begingroup\$

Some recommendations:

  • In production, don't print $stmt->error if something fails.

  • Don't print different output depending on if it's wrong password, or username doesnt exists.

  • If you don't use salts, then start using it.

  • You might want to close the connection before you die(), though this might not directly relate to the security.

If you want to print anything, just print "Wrong username or password", in all cases.

You can print this instead of "Error, no username specified" as well. And then use javascript to indicate to the user that this field needs to be filled, but if it's submitted, it just says "Wrong username or password" anyway, since the empty-string isnt a correct username.

\$\endgroup\$
2
  • \$\begingroup\$ Could you give a short explanation on why not to give a different output depending on if username exists / password is incorrect? I remember reading about it, but I'd like to hear your reasoning. Also, doesn't MySQLi automatically close the connection when the script ends? Or does that not work with die()? \$\endgroup\$
    – nvbmnbvm
    Commented Aug 1, 2014 at 17:18
  • \$\begingroup\$ Sorry for the late answer, but just read this answer and saw your comment. You can read here: owasp.org/index.php/Improper_Error_Handling, specifically "Even when error messages don’t provide a lot of detail....". You reveal how your login mechanism works internally if you provide different output for the different scenarios. \$\endgroup\$
    – netigger
    Commented Jun 29, 2016 at 6:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.