1
\$\begingroup\$

Hello I am a beginner in PHP and I need some opinions. How secure is my registration code ? If I launched this on the web would you feel safe signing up with personal info ?

  <?php

    // shows all errors
    ini_set('display_errors', 1);
    error_reporting(E_ALL);

    //check if form is submitted
    if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) 
    {
        // looks like a hack, send to index.php
        header('Location: index.php');
        die();
    }

    include("connect.php");

        $errors = [];
    if (empty($_POST["username"])) {
        $errors[] = "Fill in username to sign up";
    }

    if (empty($_POST["pw"])) {
        $errors[] = "Fill in password to sign up";
    }

    if (empty($_POST["pw2"])) {
        $errors[] = "Confirm password to sign up";
    }
    // and so on...
    if ($_POST['pw'] !== $_POST['pw2']) {
        $errors[] = "The passwords do not match.";
    }

    if (!$errors) {
        $stmt = $conn->prepare("SELECT * FROM users WHERE username=?");
        $stmt->bind_param("s", $_POST['username']);
        $stmt->execute();
        $row = $stmt->get_result()->fetch_assoc();

        if ($row && $row['username'] == $_POST['username']) {
            $errors[] = "Username exists";
        }
    }
    if (!$errors) {
        $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, array('cost' => 14));

        $stmt = $conn->prepare("INSERT INTO users (username, pw) VALUES(?, ?)");
        $stmt->bind_param("ss", $_POST['username'], $pw );
        $stmt->execute();

        echo "Registration successful <a href= index.php>Login here</a><br />";
    } else {
        foreach ($errors as $error) {
            echo "$error <br /> \n";
        }
        echo '<a href="index.php">Try again</a><br />';
    }
    ?>
\$\endgroup\$
1
  • 2
    \$\begingroup\$ There is nothing much to review. The code is up to modern security standards. You should just set 'display_errors' to 0 on a live server. \$\endgroup\$ – Your Common Sense May 22 '20 at 13:11
1
\$\begingroup\$

Some remarks:

You have repetitive code eg:

if (empty($_POST["username"])) {

You might as well define an array of fields to check and run validation in a loop. Although you have only 3 items here...

There is a major flaw: you are not sanitizing the input fields:

$stmt->bind_param("s", $_POST['username']);

At a minimum you should trim the text because it may contains whitespace (before and after). So if user 'admin' is already taken, I can register as 'admin ' for example (one space). That is not necessarily a direct vulnerability, it depends on the rest of the code, but you have inconsistent data and this will likely cause problems later. Usernames should not contain whitespace leading or trailing whitespace. Because you can have two different accounts the all look the same in print and on screen. That makes impersonation easy.

What you should do is assign the POST fields to variables, and then use variables in the rest of the code, after checking them and sanitizing them. Do not reuse $_POST['whatever'] all across your code.

You should also test what happens if the HTTP request is tainted, for example if $_POST['username'] is included twice in the POST request. Or if the field contains multiline input (or null characters). How will your code react ? Do you have error handling and logging ?

I note that you are not checking the length of input fields, including the password. What happens if the text is very large, larger than the corresponding table field ?

You should have a sensible password policy and not accept any password like 1234 or same as the username...

What you are showing is the registration form, but what would be interesting to see is the login page. That's the thing that should not be easy to trick or bypass.

If you use a modern development framework you can simplify your life, and your code will very probably be more secure.

In my opinion a page that does not have error handling is not secure, because you are not seeing what's going on and the code could also behave unpredictably.

Conclusion:

If I launched this on the web would you feel safe signing up with personal info ?

No. There are simply not enough checks. The code is not robust. You should test it in adverse conditions. Try automated tools like SQLmap, Nikto and simulate an attack against yourself.

It is good that you are using parameterized queries, that's the least you could do in 2020. But that does not mean user input does not have to be checked and sanitized. You may be opening your site to other vulnerabilities. What is pretty much guaranteed is that users will use very weak passwords since you are accepting anything. Therefore many accounts will be susceptible to brute force attacks. And you will be blamed for facilitating a breach of personal data.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ There are good points and bad points. I'll highlight the latter. trim() is anything but a sanitization. "Usernames should not contain whitespace" is a questionable statement. You probably meant "leading or trailing whitespace". By the way, I can register as "аdmin" without any whitespace. You said trim is "minimum" but didn't venture any further. Make your own statements consistent first. \$\endgroup\$ – Your Common Sense May 22 '20 at 15:34
  • 1
    \$\begingroup\$ "a page that does not have error handling is not secure" is outright wrong. Having each separate page fumbling with its homebewed error handler IS insecure. An error handler should be centralized and never have any business with other scripts. PHP's default error handler is all right. You cannot tell there is no error handler. It is always there. Check your error log. \$\endgroup\$ – Your Common Sense May 22 '20 at 15:41
  • 2
    \$\begingroup\$ The overall tone is rather alarming ("there is no sanitization!", "the code is not safe!") but without any particular attack vector suggested. It leaves the OP rather confused than enlightened. \$\endgroup\$ – Your Common Sense May 22 '20 at 15:46
  • \$\begingroup\$ Some fair points, edit made. To rephrase my statement, judging by the presence of stuff like ini_set('display_errors', 1); I think it is safe to assume that the OP is not handling any errors (otherwise logged by PHP) and there is no centralized handling of errors either. For that reason I do not consider the code as 'safe'. Ignoring errors is never good. \$\endgroup\$ – Anonymous May 22 '20 at 16:09
  • 1
    \$\begingroup\$ To make it clear: Yes. Ignoring errors is never good. It doesn't mean, however, that there should be a single line of (system) error handling code in this script. So we should just tell the op to keep an eye on the error log and may be to think of a custom error handler. But in general error handling is irrelevant to this code. \$\endgroup\$ – Your Common Sense May 22 '20 at 16:35

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.