3
\$\begingroup\$

Does my code look secure right now? I know that I still have the password hashing and so forth, but what about SQL injections? Do I have any? Are there any other security issues?

<?php
$con=mysqli_connect("localhost","root","xxx","s");
// Check connection
if (mysqli_connect_errno())
  {
  echo "Failed to connect to MySQL: " . mysqli_connect_error();
  }


$name = $con->real_escape_string($_POST['name']);
$username = $con->real_escape_string($_POST['username']);
$email = $con->real_escape_string($_POST['email']);
$password1 = $con->real_escape_string($_POST['pass1']);
$password2 = $con->real_escape_string($_POST['pass2']);

if (empty($name) || empty($username) || empty($email) || empty($password1) || empty($password2))
{
    echo "Complete all fields";
    // you can stop it here instead of putting the curly brace ALL the way at the bottom :)
    return;
    }   



if (!filter_var($email, FILTER_VALIDATE_EMAIL))
{
    echo $emailvalid = "Enter a  valid email";
    }

if (strlen($password1) <= 6)
{
echo $passlength = "Password must be at least 6 characters long";
}

// Password numbers
if (!preg_match("#[0-9]+#", $password1))
{
echo $passnum = "Password must include at least one number!";
}   
if (!preg_match("#[a-zA-Z]+#", $password1))
{
echo $passletter = "Password must include at least one letter!";
}
if ($password1 <> $password2)
{
echo $passmatch = "Passwords don't match";
}   

if(!(isset($emailvalid) || isset($passlength) || isset($passnum) || isset($passletter) || isset($passmatch))) {
mysqli_query($con,"INSERT INTO pass (Name,Username,Email,Password) VALUES     ('$name','$username','$email','$password1')");
 }


mysqli_close($con);
?>
\$\endgroup\$
0

3 Answers 3

3
\$\begingroup\$

Not only is there code that does this type of stuff already out there, freely available. This question has been asked and answered a couple of times before on this site already. This is one such example.

However, given that the answers currently posted here are either incomplete or, IMHO, wrong (suggesting bad practices, or providing incomplete info), I'll post a short review of mine here, too.

You have a simple, procedural script. There's nothing wrong with that, per se. Although, nowadays, people will tell you to pour your code into objects and let each class do its own, specific task.
Why should you use objects here? Well, that's really down to what kind of project you're putting together: is the site going to be a fully dynamic LAMP driven site? Is the code likely to grow, or is it probable that, as you go along, new features will be added to your site? Then yes, you should look into writing your own classes, or adopt a framework (ZendFW, Symfony2,... there are tons to choose from).

If the amount of PHP you'll be using is relatively small, and its role won't exceed simple user input checks and some DB connectivity, then OO code is just going to add clutter, and bloat.

On the code
Connecting to the DB, and executing queries alongside code that actually generates output for the user/client is one of the main reasons why PHP is said to be a badly designed language. Yes, PHP allows you to write that kind of code, but you should know that it's actually seen as bad practice. A saw doesn't prevent you from sawing off the branch you're sitting on, but that doesn't mean it's a good idea to do so... Same thing applies to programming.

If you don't require object-oriented code (yet), at least create functions to handle the DB stuff, put them in a separate file, and call the functions when you need them:

require 'db_functions.php';//or require_once
$db = getDbConnection(
    array(
        'user'  => 'username',
        'pass'  => 'yourpass'
    )
);

Perhaps look into ways to keep the actual login credentials oustide of your code.

Escaping and assigning
I cannot, for the life of me understand why you would do anything like:

$name = $con->real_escape_string($_POST['name']);

with all of your post values. All those values belong together: they represent the data the user posted to your script, to validate and process. Why would you assign them to individual variables? keep that data as a whole.
If you want to check if an array has all values/keys set, then please use isset and then compare the values to an empty string. empty has a couple of oddities you should be aware of.

In this case, validating the post data can be easily done in a single loop:

$keys = array('name', 'username', 'email', 'pass1', 'pass2');
foreach ($keys as $k)
{
    if (!isset($_POST[$k]) || $_POST[$k] == '')
        exit();//<-- more on this later
}

Code like this can be easily poured into a function, to which you can pass both the array to check ($_POST) and the keys to check ($keys). Just 2 simple arguments, and you never need to write code assigning and checking variables again, just a single function call will do.

Now, the real_escape_string calls really are a bad idea. Quote chars, slashes... they're all allowed in email addresses. calling real_escape_string means that there is a chance that you're screwing up perfectly valid user input, and rendering it invalid. don't.

Using prepared statements here would be the only proper way to go: don't process the input a user gave you. If the input is anything other than perfect, assume the input is invalid or possibly malicious.

Avoid too many regex's
Checking for a password that contains both digits and characters can be done through regex, but that might not be the best tool for the job. The actual benchmarking is your job, but a regex often is slower than a simple loop, checking if a char is numeric or not.

Either way: you are using 2 regular expressions for something that can be easily done with just one expression. I've posted an answer here which goes into more details on the matter.

re-assigning again?
After validating the input (that, as I explained above, you may have rendered invalid yourself!), you then check variables like $emailValid again. That really ought to set off alarm bells.

Just think about it: you check some things. If the very first thing you checked showed you the user sent invalid input, you still procede to check, validate, reassign all other values, only to then again check if all input is valid.
That's doing the same thing twice, isn't it? DRY (Do not Repeat Yourself) is a common expression/piece of advice in programming. Your code basically repeats the most basic input validation task. That can't be good, now can it?

That's why throwing an exception is preferable, and that is also why separating output code, and logic/db related code into (at least) functions. These functions (functional units, as their name suggests) could be written to throw exceptions, which will then determine the output message the client will get to see.
Here's a basic example to clear this up. Remeber how I mentioned a function to check if all POST params are set? Here's how that would work in your case:

function checkKeys(array $data, array $keys)
{//note the type-hints!
    foreach ($keys as $k)
    {
        if (!isset($data[$k]) || $data[$k] == '')
            throw new InvalidArgumentException($k.' parameter not set');
    }
    return true;
}
//your code:
try
{
    checkKeys($_POST, array('name', 'username', 'email', 'pass1', 'pass2'));
}
catch( InvalidArgumentException $e)
{//catch Exception works, too
    echo 'Error: ', $e->getMessage();
}

That should be enough to get you started for now ;-P

\$\endgroup\$
1
\$\begingroup\$

Unfortunately, this snippet could be written a lot better, and it's already been written a lot better. However, I'll assume this isn't anything close to production and you're doing it for the learning process!

Consider delving into the PDO world before getting to familiar with the MySQLi functions. Doing so will keep you on a good track to learning correct object oriented programming too.

Which bring us to the next point: your code is highly dependent and incredibly coupled and tight. I suggest you read up on some object oriented matters, like this, this, and this.

However, since your code isn't lookin' like that, I'll just jump to reviewing the code available.

Firstly, in your check to see if the connection is successful, your fail block only echos the error. That allows a broken connection to continue in the script; that's bad. A solution would be to throw an exception from a database connection class which would in turn allow the view to present a user friendly error.

Up next, you're using a real escape function. Prepare your statements instead.

Your empty string checking is a good example of guard clauses, good job.

Good for using the built-in email validation function!

When comparing password equality, it is acceptable to use <> however it's easier to understand and read !==. I'd suggest switching those.

And lastly, adding parameters to your queries would eliminate some of the SQL injection possibility currently open in your code. You could also place back ticks (`) around your column and table names, but if your databases are made properly this shouldn't be necessary.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ The article comparing PDO to mysqli you linked to is over 2 years old, and is (perhaps conveniently) leaving out those features that mysqli supports, that are not supported by PDO (yet). Yes, I too prefer PDO's API, but that does not deter from the fact that, to this day, mysqli still is the more powerful extension. I therefore disagree with your saying mysqli is "acceptible", and PDO is preferable. Both are equally fine. Sometimes PDO is preferable, in other cases, mysqli is better. In the end, and in this case, it's all down to personal preference. oh, and die is bad practice. always. \$\endgroup\$ Apr 21, 2014 at 16:19
  • 1
    \$\begingroup\$ Injection of the second order is still possible, even when using prepared statements. Please, don't spread information hailing prepared statements as the ultimate answer to all vulnerabilities, because they're not. Placing backticks around the tables will work fine on MySQL (and related) DB's, but the use of backticks is non-standard, leaving them out really is the better. \$\endgroup\$ Apr 21, 2014 at 16:24
  • \$\begingroup\$ I'll have to say, linking to that tuts + page was an ify decision for me, I felt hesistant and I should have left it out! I'll edit my answer to be less biased towards PDO and I'll make an effort to read a bit more about the differences. As for the back ticks, I agree that if your table is made properly in the first place they shouldn't be needed. \$\endgroup\$
    – Alex L
    Apr 22, 2014 at 0:29
  • 1
    \$\begingroup\$ It's not a matter of the backticks being needed or not, it's about them being a MySQL oddity... not a standard SQL thing. One of the plusses of PDO mentioned in the linked page is supporting multiple adapters, if you then write queries that specifically target MySQL, then you loose that "edge" \$\endgroup\$ Apr 22, 2014 at 5:46
-1
\$\begingroup\$
  1. Consider parameterized queries over building a query string.
  2. If you really want to use encoding, do the encoding directly before passing it to mysql
  3. Factor out the password strength rules into a separate function. This function could output an array of violations.
  4. Don't mix logic and IO.
\$\endgroup\$

Your Answer

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

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