3
\$\begingroup\$

I have a website written in PHP but the whole script is old so today I started upgrading from Mysql to Mysqli.

So here's my register page. I still have to add account confirmation and send mail part but that part can wait for now.

/* PUBLIC */
$IP = $_SERVER['REMOTE_ADDR'];


/* PRIVATE */
if(isset($_POST['submit']))
{
    $email= $_POST['email'];
    $email_safe = $mysqli->real_escape_string($email);
    $password = $_POST['password'];
    $password_safe = $mysqli->real_escape_string($password);
    $password1 = $_POST['password1'];
    $password1_safe = $mysqli->real_escape_string($password1);
    @$referrer = $_POST['referrer'];
    $referrer_safe = $mysqli->real_escape_string($referrer);

    if ($password_safe  == $password1_safe){
    if(strlen($password) < 8){
    die('Password must be at least 8 characters');
    } else {
    $result = $mysqli->query("SELECT email FROM user WHERE email='$email_safe'");
    if ($result->num_rows > 0) {
    die ("Duplicated email. Please use other email.");
    } else {
    $result = $mysqli->query("SELECT reg_ip FROM user WHERE reg_ip='$IP'");
    if ($result->num_rows > 0) {
    die ("Duplicated IP.");
    } else {
    $sql = "INSERT INTO user (email,passwd,ref) VALUES ('$email_safe','$password_safe','$referrer_safe')";
    if(!$result = $mysqli->query($sql)){
    die('There was an error running the query [' . $mysqli->error . ']');
    } else {
    echo"Registering....";
    }
    }
    }
    }   
    } else {
    die ("Password didn't match. Please go back and fix this error.");
    }
} else {
    HTML FORM
    }

Well ? How it's looking ? It is secure ? ( I mean any open gate :) )

Thanks :) P.S : The code works 100%.

HTML form

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

While it's great to see someone actually take the trouble to refactor code, to move away from the depreacted mysql_* extension, it's not so great to see that, like most, you're actually trying to use the replacement (mysqli_*) as a 1-on-1 copy of the old extension.
On a personal note, though I've restricted myself to only dealing with the mysqli_* extension, I couldn't help noticing you're using the OO API it offers. Personally, I find the mysqli_* API, both procedural and OO a tad, erm, ... messy. If you're collaborating with others, there's nothing to stop them from switching/inserting procedural-style code, and mixing it in with your OO code. That's just nasty. PDO is another, valid alternative. It's API is OO-only, a lot cleaner and waaaay more easy to use. It's said to be slower, but in reality, the differences are so minute, they don't outweight the upsides in terms of code maintainability. Look into PDO, too, is what I'm trying to say... :)

Some history:
The old extension was created back in the days of MySQL version 3. Today, MySQL is already at version 5.6. Most hosts, if not all, offer mysql 5.x as standard DB. The list of features that weren't around back in the MySQL 3 era, but that are in common use now is rather impressive, and long:

Of course, there were other reasons why the mysql_* extension was deprecated. Its source wasn't very maintainable, for example. It's been around longer than the ZendEngine2, too. In short: it'd become sort of historic ballast, that wouldn't have worked, considering the direction PHP is going in (compiled language, OOP supprt, namespaces...)

Applied to your code:
All in all, there's a lot of changes, and some of them are quite useful when it comes to security. Take prepared statements for example (Note: here's some info on what they are, what they do). Rather than calling $mysqli->escape_string(), which as it happens requires you to set the charset!, you can easily use a prepared statement, much to the same effect:

$emailStmt = $mysqli->prepare('SELECT email FROM user WHERE email= ?');
$emailStmt->bindParam('s', $_POST['email');
$emailStmt->bind_result($dbEmail);//fetches result into this variable
if (!$emailStmt->execute() || $emailStmt->num_rows === 0)
{
    header('Location: '.basename($_SERVER['PHP_SELF']));//302 redirect is fine here
    $emailStmt->close();
    $mysqli->close();//be tidy!
    exit();// I hate die statements
}
$emailStmt->close();

So, no tedious escaping of all params all the time anylonger, you don't even have to worry about the types anymore. Mysql is, after all, fairly picky when it comes to data-types. Sure it can handle an_int_field >= '123', but really, it's best to avoid forcing the DB server having to cast the data to the right type. That's not what it's for.
As I said in the comments, mysqli->escape_string is considered as safe as prepared statements, But:

  • When working on your code, it's easy to forget that call to the escape_string method. Once you've forgotten that, your code is vulnerable to attacks.
  • If you forget to set the correct char-set, your queries are no longer as safe as prepared statements would be
  • getting into the habbit of using prepared statements is a good thing. You can prepare a single statement once, then use it to query your DB as much as you want. I've explained this here, with an example of how to do this.
  • Preparing a statement for a simple query might seem like a bit much, and in some ways it is. That's why the PDO extension emulates the prepare calls. That's one of the benefits of PDO: the statements are prepared, without any intervention of the DB server, so there's very little overhead when using prepared stmts. Honestly, there's no reason not to use them.

Anyway, I'm drifting off, slightly. What I'm actually trying to say is this:

Since you're refactoring anyway, why not look into what sort of new goodies mysqli_* brings to the party? the i is for improved after all. Don't use it as a 1-on-1 copy of the old extension. Time has, as it tends to do, moved on. There are new features. Learn about them, and use them. You're code will be more reliable, more maintainable, more future proof and 9/10 more safe, too, because of it.
Begin with using prepared statements, then move along to transactions. That's just a tip.

Some other thoughts
I see (or rather don't see) a worrying lack of data validation in your code. You just assume $_POST['email'] is going to be set, and that its value is going to be an email address. At no point in time, do I see you use something like this:

if (isset($_POST['email'])) && filter_var($_POST['email'], FILTER_VALIDATE_EMAIL))
{
    //query DB
}
else
{
    exit('no valid email address provided');
}

The only time your code reflects any form of awareness that, perhaps, some POST data might not be set is here:

 @$referrer = $_POST['referrer'];

But the way it's dealt with just gets to me. As I've said before, and as I will keep on saying until either the supressor of death (@, that is) or I am no more:

Argh... the @ suppressor of death! I hate that, don't use it... save for those few rare use-cases where you can't help a notice from being issued (seen this only 2 or 3 times in 10 years). If there's a notice, there's an issue, if there's an issue: don't ignore it fix it!

In this case, there isn't even an issue that needs fixing, except for the @ itself. Why not simply write:

$referrer = null;//declare var and assign null value
if (isset($_POST['referrer']))
{
    $referrer = $_POST['referrer'];
}

isset is a language construct, and therefore it's fast. supressing a notice isn't fast, it's slow, and completely pointless in most (if not all) cases. Don't.
If you don't want your code to be too long, because of all the if(isset branches, you can easily replace them with something like this:

if (isset($_POST))
{
    $email = $password= $password1 = $referrer = false;//all default value
    foreach($_POST as $key => $value)
    {
        if (isset($$key) && $$key === false)
        {//variable variable, check if it exists, and if its value is false
            $$key = trim($value);//if it is, assign POST val... perhaps trim while you're at it?
        }
    }
}

With this code, though not very maintainable, you save a lot of space, and you know that if any of the variable's used holds false as a value, it wasn't posted. All other variables (the ones that aren't false) have been reassigned the posted data.

\$\endgroup\$
  • \$\begingroup\$ Huh, well, thanks for your reply. - Still trying to understand your reply fully, but thanks for referrer fix. About email verification, is being done by HTML code imageshack.com/scaled/800x600/15/4jk6.jpg \$\endgroup\$ – rgerculy Oct 6 '13 at 15:56
  • \$\begingroup\$ About prepared statement, isn't that to much for a simple query ? In case I have an error , It's hard to find a fix for mysqli, therefore I try to keep it simple. \$\endgroup\$ – rgerculy Oct 6 '13 at 16:03
  • \$\begingroup\$ @rgerculy: HTML (client side) validation of the input is all well and good, but what if I were to disable JS? I would be able to post anything as email input. Never trust client input, and always validate/sanitize the input server-side, too. Escaping input is safer than blindly concatenating input into queries, true, it's even considered to be as safe as prepared statements, but your code remains more error prone, I'll add some more details to my answer on that subject \$\endgroup\$ – Elias Van Ootegem Oct 6 '13 at 18:07
  • \$\begingroup\$ True, without JS everything is down. Thanks for help and update. \$\endgroup\$ – rgerculy Oct 6 '13 at 18:27

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.