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.
