CODEWITHSUNDEEP
phpmysqlsecurity
Dominus
Dominus

Reputation: 998

Is this good practice regarding security and MySQL

I would just like to have some feedback on some php code for a website, because it's the first time I'm writing code that will properly be used on public websites and I'm a little paranoid.

This code is at the top of a signup page with a post form that sends back to itself, so I check if the form has been completed by checking if the POST "isset".

It's for a signup page : 

<?php
if (isset($_POST['email'])) {

    $thanks = '';

    $name = $_POST['name'];
    $email = $_POST['email'];
    $postcode = $_POST['postcode']; //not used
    $password = $_POST['password'];


    //connects to database, DB variable is dbConnection
    require_once("include/database_connect_test.php");

    //First check email isn't already on DB
    $sql = 'SELECT * FROM `user_profile` WHERE email=?';

    $stmt = $dbConnection->prepare($sql);
    $stmt->bind_param('s', $email);
    $stmt->execute();
    $stmt->store_result();


    $num_of_rows = $stmt->num_rows;
    $stmt->close();

    if($num_of_rows>0)
        $thanks = 'This email has already been registered';

    //If not, create profile
    else{

        require_once('generate_uuid.php');

        $hash = password_hash($password, PASSWORD_BCRYPT); //password_verify(pass, hash) to verify



        $sql = 'INSERT INTO `user_profile`(`name`, `email`, `hash`, `uuid`) VALUES (?,?,?,?)';

        $stmt = $dbConnection->prepare($sql);
        $stmt->bind_param('ssss', $name, $email, $hash, $uuid);

        $stmt->execute();

        $stmt->close();


        $thanks = 'Thank you for signing up!'; //used in the html
    }
}?>

Thanks in advance for any input :)

Upvotes: 2

Views: 78

Answers (4)

Neil McGuigan
Neil McGuigan

Reputation: 48267

  1. User enumeration. Use a 2-step registration (w email link)
  2. You are not preventing high-speed robotic registrations
  3. You are not confirming the user's email address or password (they might have typed it wrong. Most sites use a "confirm password" form field)
  4. Not validating data (is email really an email, is password ten spaces?)

Unrelated to security, you should use the Post-Redirect-Get pattern. For now, when user sees "Thanks" message, if they hit "refresh" on their browser, it will try to re-register them.

Upvotes: 1

Martin
Martin

Reputation: 22760

Here is a small collection of PHP features and functions that can be useful to your code block:

  • filter_var, check that the string email address $_POST['email'] is a valid email shape: 
 if(filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)){...}
  • You really should set the $options['cost'] value for your Password_hash. THe default standard is 10 which is a bit weak, depending on your server specs I would say you want at least 12 cost value. 
$options['cost'] = 12;
$hash = password_hash($password, PASSWORD_BCRYPT,$options);

  • I would recommend running your $_POST variables through a custom stripper function such as a preg_replace Regex. I would personally always use a Regex routine to strip out unwanted characters from a submitted string.

  • Your uuid value with the name of id I would expect it to be a counter integer but it's cast to the binding as a string(S), so not sure about what type of variable that is?

  • Also you can improve your security by researching and looking into Cross Site Scripting and how to avoid this, basically set a unique value in your submitting form, and save this value to a $_SESSION and then compare, this will much more ensure that things sent to your PHP / MySQL will come and be accepted only from your sending form.

  • On a related security note, you can also setup a Content Security Policy header for your website.

Upvotes: 1

&#193;lvaro Gonz&#225;lez
&#193;lvaro Gonz&#225;lez

Reputation: 146490

I see nothing particularly wrong from the security standpoint. You're doing the most important things right:

  • Use a MySQL extension that's currently supported
  • Use prepared statements
  • Hash passwords with password_hash()

Being picky:

  • The fact that $_POST['email'] is set doesn't imply other post variables will also exist. You should really verify all input because filling your form is not the only way to submit data to your script. But rather than security is a way to ensure you can take benefit of PHP notices by avoiding false positives.

    Also, you may like the filter extension, a feature that's been available for a while but is not particularly widespread and allows code like this:

    $name = filter_input(INPUT_POST, 'name');
$email = filter_input(INPUT_POST, 'email');
$postcode = filter_input(INPUT_POST, 'postcode'); //not used
$password = filter_input(INPUT_POST, 'password');

  • I've never liked the technique of counting rows in a result set to validate if a user exists because it's too dependent on external factors (database driver, query function used...). I simply fetch the row, which also allows to retrieve additional user data for session variables.

  • I suppose that $uuid is defined inside generate_uuid.php but when you look at code it seems to pop up from nowhere. A better structure is that your included files load functions and then you call those functions:

    $uuid = generate_uuid();
$stmt->bind_param('ssss', $name, $email, $hash, $uuid);

... but, as I said, none of these factors affect security.

Upvotes: 3

Jojo01
Jojo01

Reputation: 1289

Right now someone is able to for example put javascript as his username, and if you added a comments section that javascript would run. Your script is vulnerable to xss injection. Use mysqli_real_escape_string and htmlpurifier.

Upvotes: 0

Related Questions