2
\$\begingroup\$

I have been reading about SQL injection and I want to secure my code.

I am not asking anyone to write me a code, but I just want to learn it in simple terms. The best way for me to learn is to edit my code so I can compare them.

<?php
if (isset ($_POST['email']))  {
    //Connect to the database through our include 
    include_once "config/connect.php";
    $email = stripslashes($_POST['email']);
    $email = strip_tags($email);
    $email = mysqli_real_escape_string($db_conx, $email);
    $password = preg_replace("[^A-Za-z0-9]", "", $_POST['password']); // filter everything but numbers and letters
    $password = md5($password);
    // Make query and then register all database data that -
    // cannot be changed by member into SESSION variables.
    // Data that you want member to be able to change -
    // should never be set into a SESSION variable.
    $sql = "SELECT * FROM members WHERE email='$email' AND password='$password'"; 
    $query = mysqli_query($db_conx, $sql);
    $login_check = mysqli_num_rows($query);
    if($login_check > 0){ 
        while($row = mysqli_fetch_array($query, MYSQLI_ASSOC)){ 
            // Get member ID into a session variable
            $id = $row["id"];   
            session_register('id'); 
            $_SESSION['id'] = $id;
            // Get member username into a session variable
            $username = $row["username"];
            $email = $row["email"];
            $password = $row["password"];
            $firstname = $row["firstname"];
            $lastname = $row["lastname"];
                    session_register('username'); 
            session_register('firstname');
            session_register('lastname');
            // Update last_log_date field for this member now
            $sql = "UPDATE members SET lastlogin=now() WHERE id='$id'";
            $query = mysqli_query($db_conx, $sql); 
            // Print success message here if all went well then exit the script
            header("location: members/index.php?id=$id"); 
            exit();
        } // close while
    } else {
    // Print login failure message to the user and link them back to your login page
    header("location: login.php");
        exit();
    }
}
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ The best way for me to learn is to edit my code so I can compare them. I'm not so sure about that. We review code here, so no one will just be editing your code to make it better. Any answers you receive will offer suggestions based on your provided code. \$\endgroup\$ – Jamal Apr 7 '14 at 0:53
  • \$\begingroup\$ @Jamal, and I am not asking anyone to edit my code for me. I need suggestions and I will do the editing myself! We review code here, so no one will just be editing your code to make it better. who is "we" by the way? so far not a single suggestion apart from your unhelpful comment! \$\endgroup\$ – rooz far Apr 7 '14 at 8:10
  • \$\begingroup\$ I apologize for misunderstanding you, then. And by "we," I mean reviewers on this site. \$\endgroup\$ – Jamal Apr 7 '14 at 8:19
  • \$\begingroup\$ funny, 22 people viewed this and not a single person contributed! \$\endgroup\$ – rooz far Apr 7 '14 at 10:48
  • 1
    \$\begingroup\$ @roozfar: Right, I've gone ahead and posted some critiques. If you perceive them as harsh, blunt or find some of it offensive, I understand. But know that I'm only trying to help. In order to do so, as I've explained here in great detail, I must be severe :) \$\endgroup\$ – Elias Van Ootegem Apr 7 '14 at 12:36
6
\$\begingroup\$

So, you want to defend against possible injection attacks. That's great. But I struggle to understand why you're bothering to escape all parameters in various ways, but then let the whole thing down by not using the easiest, fastest and rather safe tool you have: prepared statements.

First off, are prepared statements 100% safe, all of the time? No, I'm not going to postulate that. Simply because I don't have a solid basis (as in proof) that it is.
Have I seen injection attacks of the first order that managed to circumvent the security gotten from prepared statements? Nope. Never. So, as long as you're not using mysqli_multi_query, don't use user-data in tandem with data gotten from another query and you don't allow yourself to get bogged down in endless sub-queries or JOIN queries without aliases, you should be pretty safe.

So, first step in defending against inject: use prepared statements.

Now, look at what you're doing with your data:

$email = stripslashes($_POST['email']);
$email = strip_tags($email);
$email = mysqli_real_escape_string($db_conx, $email);
$password = preg_replace("[^A-Za-z0-9]", "", $_POST['password']);
$password = md5($password);

Email:
What's wrong with how you process the email? A couple of things.

  • stripslashes: can potentially change email addresses that are perfectly valid! However unusual an email address is, you have the responsibility to write code that can handle valid input.
    This is a valid email address: "very.(),:;<>[]\".VERY.\"very@\\ \"very\".unusual"@strange.example.com, believe it or not. stripslashes, then, is NOT an option. See other wacky email addresses here.
  • striptags: I take it that, from the example here, and on the wiki I linked to, it's self-evident that this function call is right out, for the same reasons.

What to do, then? Simple. PHP comes with an email validator function out of the box, simply use that:

if (filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
    $email = $_POST['email'];
else
    //error.

If you want to be lenient towards your users, which I'd encourage, you can add a sanitation call to this code. If the email address posted contains invalid chars, then notify the user and ask him/her if he meant to write a valid version of the address:

if (!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{//not the !<-- not valid
    echo $_POST['email'], ' is not a valid email address. Did you mean: ',
        filter_var($_POST['email'], FILTER_SANITIZE_EMAIL);//<-- sanitize
}

Password:
What's wrong with the password? A few things.

  • first off: you're going to hash the password. An md5 hash is guaranteed to contain only hex chars (0-9a-f). These chars pose no risk at all in terms of injection.
    If you want to inform the user about him/her using an "invalid" password, however, a regex can come in handy. However, the one you're using now is not going to work!

  • preg_replace: You are not replacing what you thing you are. In PHP, a regex has to be delimited by special chars (for example: /a/ matches a). Of course, delimiters like [ and ] are allowed, too. That means that PHP doesn't interpret your pattern as a character class, but rather.

It is translated to this:

preg_replace('/^A-Za-z0-9/', '', $string);

You're matching the beginning of the string, followed by the literal string: A-Za-z0-9. This will be replaced, the rest is left as is.
Just test:

echo preg_replace("[^A-Za-z0-9]", '', 'Hello World!');

Which should replace the space and exclamation mark. The actual output, however, is: Hello World!.
I take it you were actually going for something along the lines of:

echo preg_replace('/[^A-Za-Z0-9]/', '', 'Hello World!');

Which echoes HelloWorld. But actually, it's a good thing this code doesn't work. You're actually prohibiting users from entering a safer password. Which pass, according to you (and according to common sense) would be most likely to be cracked using a simple dictionary attack:

mySimplePass
//or
|\/|y=5!mpL3_P@55

Allow complex passes, you have no reason not to.

  • md5: Ok, it's good to see you're hashing passwords. It really is. But md5 is deemed unsafe. Collisions have been spotted in the wild, and thus it's likely that there are several passwords that will be accepted as valid for a single hash.
    sha1 is a safer option, but best switch to blowfish.

Another niggle when using hash functions is: never simply hash the pass. use salt. If I were to create a dummy account, and at some point saw the hash you were using, I could figure out what type of hashing algorithm you are using, and I would know you're not using salt.
In that case, all I need is to see someone elses salt and set my machine to work, generating random strings and computing the hash. For every match I find, I have access to another users' account on your site.

Couple this to your policy of forcing people to use weak passes and you must admit, you have a huge security problem on your hands.
In case you don't know what I mean by salt:

$hash = sha1($pass, 'S0me->53cr3t<>5tr!~');

That way, I'd first have to generate random strings, in the off chance I find the salt, which is of unknown length, can contain any char, and even when I get a matching hash (in case of md5 especially), I could be dealing with a false positive (collision...).

Moving on: Like I said: use prepared statements, instead of just stringing arguments into a query string, for starters. Then, do away with the mysqli_num_rows call. All your code is contained within a while loop. If there are no results to be found, the loop body won't be executed. Instead, I'd simply write a single query:

$stmt = mysqli_prepare(
    $db_conx,
    'UPDATE members
        SET lastlogin=NOW()
    WHERE email = ?
      AND password = ?'
);
//after validation, of course
mysqli_stmt_bind_param($stmt, 'ss', $email, $pass);
mysqli_stmt_execute($stmt);
if (mysqli_affected_rows($db_conx))
{
    mysqli_stmt_close($stmt);//<-- CLEAN UP AFTER YOURSELF!
    //update was successful
}

Of course, this code doesn't select username, first and last name from the table. If you want to do so, then you'll have to keep the second query, still.

There, too, your code can be improved on. SELECT * is something that can, and should be avoided as much as possible: only SELECT what you need!
If the supplied email and pass are valid, you already have that data, so there's no reason to query for it again. You only need 4 fields: the id, user-, first- and last name.
Not only is selecting what you need easier on your server, it makes life of the people who end up maintaining your code (and that includes you, yourself) a lot easier if ever something needs to be changed/added a couple of weeks from now:

$stmt = mysqli_prepare(
    $db_conx,
    'SELECT id, username, firstname, lastname
     FROM members
     WHERE email = ?
       AND password = ?'
);
mysqli_bind_param($stmt, 'ss', $email, $pass);
if (!mysqli_stmt_execute($stmt))//returns false on success
    throw new RuntimeException('Failed to execute statement');
//get the result first
$result = mysqli_stmt_get_result($stmt);
//or $row = mysqli_fetch_assoc($result)
if ($row = mysqli_fetch_array($result, MYSQLI_ASSOC))
{
    $_SESSION['id'] = $row['id'];//and so on
    //AND CLEAN UP!!
    mysqli_free_result($result);
    mysqli_stmt_close($stmt);
}

As you can see, the mysqli_* extension offers a lot of powerful features, and tools. In return, it does require you to take care of a couple of things: freeing the results, closing the statements when you're done playing with them etc...
It takes some getting used to at first, but it's not that hard, really.

If you find this a bit too wearisome (can't blame you if you do), then take a look at the alternative extension PDO.
It offers a cleaner, less verbose API. The trade-off: it's OO-only (IMO, a good thing), slightly less powerful and marginally slower, but not as if you'd notice. Other things are far more likely to be the bottleneck.

\$\endgroup\$
  • 1
    \$\begingroup\$ Question for you - do you think it was necessary to preg_match the password if you are going to hash it? Unless you are specifically telling the user for particular sequence required (ie ur password must contain 1 cap, 1 special char...etc), why not just use the hash algorithm straight \$\endgroup\$ – azngunit81 Apr 7 '14 at 14:05
  • \$\begingroup\$ @azngunit81: preg_match as a validation tool in this case is, indeed, completely pointless and silly. A hash like md5 is guaranteed to yield a string of 0-91-f chars only. I thought about adding that, but I got carried away with explaining why the regex the OP used was wrong, and he'd probably never know about it. will edit, though \$\endgroup\$ – Elias Van Ootegem Apr 7 '14 at 14:14
  • \$\begingroup\$ @EliasVanOotegem, Thanks for the explanation. I just wish you did explain it the other way round (easier way "PDO" first and a link to learn the hard way) lol... non the less, appreciate it mate.... \$\endgroup\$ – rooz far Apr 7 '14 at 16:20
  • \$\begingroup\$ @roozfar: Well, this is code-review after all... I have to start from what you give me: your code. Anyway, glad I could help. Just a side-note: On stack-exchange sites, the help section asks not to post thank you comments, but to mark an answer as accepted (by clicking the big check-mark next to the answer in question), and/or upvoting the answer. If you accept an answer, you get an extra +2 rep reward to encourage accepting answers \$\endgroup\$ – Elias Van Ootegem Apr 7 '14 at 17:46
  • \$\begingroup\$ @EliasVanOotegem, i am still trying to learn what you said in your answer so just to be honest to myself, I won't mark this as the answer as it really doesn't makes sense to me as of yet! however, +1 for your explanation and effort to write that up. \$\endgroup\$ – rooz far Apr 7 '14 at 17:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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