PHP script security for new user registration

Question

I would like to ask the experts here if this is secure. If it is not can I get an explanation as to why it would not be secure? I have been reading off and on on sites for this and I have trouble understanding a couple of things.

Here is the main registration script:

<?php
$name = $_POST['username'];
$password = $_POST['password'];
$email = $_POST['email'];
$hash = md5 (rand(0,1000));
if(isset($name))
{
    if (trim($name) == '' || trim($name) == ' ')
    {
        print_r('Username cannot be empty');
    }
    else
    {
        if(preg_match('/^[a-zA-Z0-9]{5,}$/', $name))
        { // for english chars + numbers only
            if( strlen($password) < 8 ) { print_r("Password too short!"); }
            elseif( !preg_match("#[0-9]+#", $password) ) { print_r("Password must include at least one number!"); }
            elseif( !preg_match("#[a-z]+#", $password) ) { print_r("Password must include at least one letter!"); }
            elseif( !preg_match("#[A-Z]+#", $password) ) { print_r("Password must include at least one CAPS!"); }
            elseif( !preg_match("#\W+#", $password) ) { print_r("Password must include at least one symbol!"); }
            else
            {
                if(isset($_POST['submit']))
                {
                    if(filter_var($email, FILTER_VALIDATE_EMAIL))
                    {
                        $subject = 'Signup | Verification'; // Give the email a subject 
                        $message = '

Thanks for signing up!
Your account has been created, you can login with the following credentials after you have activated your account by pressing the url below.

------------------------
Username: '.$name.'
Password: '.$password.'
------------------------

Please click this link to activate your account:
http://www.mywebsite.com/verify.php?email='.$email.'&hash='.$hash.'

'; // Our message above including the link

                        $headers = 'From:[email protected]' . "\r\n"; // Set from headers

                        if(!isset($con))
                        {
                            $config = parse_ini_file('config2.ini');
                            $con = mysqli_connect(`localhost`,$config["username"],$config["password"],$config["dbname"])or die ("MySQL Error: " . mysqli_connect_error());
                        }
                        $stmt = $con->prepare("SELECT login FROM Accounts WHERE login=?");
                        $stmt->bind_param("s", $GLOBALS['name']);
                        $stmt->execute();
                        $stmt->bind_result($un);
                        $usernamefound = 'false';
                        while ($stmt->fetch())
                        {
                            if(isset($un))
                            {
                                $usernamefound = 'true';
                                print_r ('Username already exists.');
                            }
                        }
                        $stmt->close();

                        if ($usernamefound == 'false')
                        {
                            if (filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP) || filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV6))
                            {
                                $ip = filter_var($_SERVER['REMOTE_ADDR']);
                                mail($email, $subject, $message, $headers); // Send our email
                                $hashed_password = password_hash($GLOBALS['password'], PASSWORD_DEFAULT);
                                $result = $con->prepare('INSERT INTO Accounts(login,password,email,lastip,hash) VALUES (?, ?, ?, ?, ?)');
                                $result->bind_param("sssss", $GLOBALS['name'], $hashed_password, $GLOBALS['email'], $ip, $GLOBALS['hash']);
                                $result->execute();
                                $result->close();
                                $con->close();
                                echo "Registered: " . $ip;
                            }
                            else
                            {
                                header('Location: noIPError.html');
                                die();
                            }
                            echo "<br/>";
                            // Return Success - Valid Email
                            print_r('Your account has been made, <br /> please verify it by clicking the activation link that has been send to your email.');
                        }
                    }
                    else
                    {
                        // Return Error - Invalid Email
                        print_r('The email you have entered is invalid, please try again.');
                    }
                }
            }
        }
        else
        {
            print_r('Invalid Username: Must be alphanumeric and longer than or equal to 5 chars');
            echo "<br/>";
            print_r('No special characters or spaces allowed');
        }
    }
}
?>

I would really like to know what i am doing wrong when it comes to mysqli_real_escape_string as well and from what I've been reading it's not even really that secure anyway. What if I do the variables directly and simply just get rid of $name, $password and so on all together. Would that be the most secure way to do it?

I'm assuming it's ok for those globals to exist due to the password hash?

If there is any major exploit I'd like to learn about it I guess is what this question is mainly about.

Answer 1

Coding well is hard, but reading code should be easy

After reading through your code I think it is reasonable secure. You protect yourself against SQL-injection and you use password_hash(). All good things.

However this code only provides the most basic security, and that not very well. Sorry, I don't mean to be rude.

There is one thing, I worry about, above everything else: This code is a bit of a mess. And messy code is a security risk, even if it all works correctly now. You might want to change something, years from now, and make a mistake because you misunderstand your own code. Or even worse: Someone else has to work on your code.

Well written code should be almost self-evident in what it does and how it does it. How to do this? Well there are many resources, for instance:

https://www.macronimous.com/resources/articles/writing-clean-secure-easy-maintain-php-code

https://deliciousbrains.com/refactoring-php-code-better-readability

https://code.tutsplus.com/tutorials/top-15-best-practices-for-writing-super-readable-code--net-8118

And many, many more. Visiting, and reading, StackExchange, is also a good idea.

Here are some of the comments I wrote down regarding your code (in random order):

• Unused hash variable: You have a global variable called $hash, you even store it in the database, but it is not used in any meaningful way. You should get rid of it, especially because a reader might think its used to improve the security in some way. It is not.
• Output to user: The function print_r() is normally used to output human-readable information about a variable, and shouldn't be used to output plain strings. It works, but better use echo() or just print().
• Encapsulate code: It's better to put your code in a function or method, that way it is better isolated from other code you might write. You could, for instance, be using variables like $name and $email in other code as well.
• Choosing a password: You have many rules for the password. Users find that very annoying. Since you email the password to the users anyway, why not choose a password for them? Save the users the hassle of choosing one. Mind you: Some might regard the emailing of a password as a security risk., but it all depends on how secure you want to be. Security can be overdone at the expense of usability.
• escaping strings: I don't see escaping of strings in your code. You are using bind_param() which is what you should be using.
• Choice of variable names: Apart from having every variable at the global scope (see point 3), the names of the variables don't seem to be chosen with care. After careful reading, I was able to find out that $un probably means 'username', but why the obfuscation? Also, the 'username', is sometimes called $name, then 'login' in your database, and finally $un when you read the database. These should preferably be the same.
• Use of globals: Don't use $GLOBALS['name'] when you can just write $name. If you ever need to use a global variable, you have created yourself, in a function or method, use global $name; to indicate you will be referring to the global version of that variable.
• Create variables only when they are needed: An example is the $message variable. You create it when the email address is validated, but only use it after the database connection was made, the username was not found, and you have a valid IP address. It might not be used at all, so you've wasted resources.
• Database connections can fail: You check to see if you have a database connection, and you make one when there isn't. That is weird in itself, but more trouble is it that you don't check whether you actually got a good connection. A connection will, one day, fail, and you need to handle that gracefully.
• Many nested if ... elseif ....: It is easy to loose track of what's going on in your code because of all the nested code fragments. The code doesn't have much structure. Using functions and classes can solve this problem.
• Comments should be useful and accurate: There's no point in reading a comment like "Return Success - Valid Email" when nothing is returned and the email was validated 60 lines before that comment. There are no comments above the queries, where they could have been useful. Also be consistent about either placing comments before or after whatever they refer too, otherwise it is very confusing.

This list seems long, but it is by no means exhaustive. I do realize that learning to code is difficult, hence the title, and I can see you're on the right track. With a bit more practice, and taking some of the suggestions on board, you coding style could improve greatly.

Answer 2

Aside from the excellent review from KIKO Software, I think it would be beneficial for you to understand some basic concepts, so here you are:

1. Your sql queries are 100% safe because you are using prepared statements for the data literals.
2. I don't really get what is your concern about mysqli_real_escape_string but be advised that this function has nothing to do with security at all.
3. there is a big security hole in your code nevertheless. $config = parse_ini_file('config2.ini'); means that your database credentials are open to anyone. At least put them in a PHP file, so even called directly it won't reveal any information.
4. Your code should be always readable. And your current code is almost unreadable due to insane nesting. There are a lot of useless conditions such as if(isset($name)), and there are a lot of unnecessary nesting. There is no point in checking the email only if the password is correct. Why not to check both and tell a user at once? So, do not nest one into another, make them consequent, not dependent. Introduce an $error variable to collect all validation errors and then have just a single condition to tell whether the data is OK or not.

And also there are some minor issues as well:

• $name = $_POST['username']; if(isset($name)) makes absolutely no sense due to the nature of isset() function. You just defined this variable yourself, why you're checking its existence?
• trim($name) == ' ' condition makes no sense as well, due to the nature of trim() function. It just cannot return a lone space character, it will be stripped.
• if(isset($_POST['submit'])) is severely misplaced. You are already working with half of a dozen POST variables and then all of sudden decide to test whether it was request was the POST request. It should be the first condition in the code.
• if(!isset($con)) is also look strange. Why do you think there could be no connection? There should be no such uncertainty.
• the connection code is insufficient which will cause many problems. Please refer to my article on How to properly connect to Mysql database using mysqli

• the fetch code is superfluous as well. The whole code block

                  $stmt->bind_result($un);
                  $usernamefound = 'false';
                  while ($stmt->fetch())
                  {
                      if(isset($un))
                      {
                          $usernamefound = 'true';
                          print_r ('Username already exists.');
                      }
                  }
  // should be changed to just TWO lines:
  
                  $stmt->bind_result($usernamefound);
                  $stmt->fetch();
  

• the if ($usernamefound == 'false') is just wrong. false is a constant and should be written without quotes.
• the whole condition if (filter_var($_SERVER['REMOTE_ADDR']... is superfluous as well. $_SERVER['REMOTE_ADDR'] is always correct, otherwise the request would not reach your server at all. There is no point in testing $_SERVER['REMOTE_ADDR'] (as opposed to HTTP headers that may contain various IPs)

After fixing these issues (and - especially - the nesting) you'll get a pretty decent code.

Answer 3

Two big concerns on the actual password security side:

You're enforcing hard-to-remember-but-easy-to-crack passwords. (Obligatory XKCD: Correct Horse Battery Staple)

Rather, the current guidance from the National Institute of Science and Technology, the standards body responsible for people writing commercial software in the United States is:

5.1.1.1 Memorized Secret Authenticators

Memorized secrets SHALL be at least 8 characters in length if chosen by the subscriber. Memorized secrets chosen randomly by the CSP or verifier SHALL be at least 6 characters in length and MAY be entirely numeric. If the CSP or verifier disallows a chosen memorized secret based on its appearance on a blacklist of compromised values, the subscriber SHALL be required to choose a different memorized secret. No other complexity requirements for memorized secrets SHOULD be imposed. A rationale for this is presented in Appendix A Strength of Memorized Secrets.

You have the 8 character minimum limit right, but rather than making sure users supply lower case, capitals, numbers, and non-alphanumeric symbols, you should be making a call to a service that has a list of known insecure passwords. I use the Pwnd Passwords API call to the database made available by Troy Hunt. If you want to be truly paranoid, download the hash files and import them into a database yourself so that you can check them while knowing that no information about their password is leaving your server.

Which brings me to the second big concern:

Never show the user their password (unless they explicitly ask for it while they're typing/pasting it in).

Even on your own test system that you're setting up in a server at your own home that will never serve traffic to real customers.

After all, if you're the only one who will touch it, you can always reset the password. And if there are customers on the server, you don't know if they're in a coffee shop and there's a malicious person standing over their shoulder. There's a reason why every password bar only shows dots instead of the plaintext password. Respect that and respect your users' privacy.

And besides, if the user ever does forget their password, you should be using a password reset system, not implying that they should have recorded it somewhere. (And when building the password reset system, follow the guidelines laid out by OWASP.

Remember, it's not your password, it's theirs... and they're likely reusing that password for their banking site and the rest of their online presence. You don't want to be responsible for that information, so don't ever let anyone, including them, be able to access it in plaintext after they have sent it to your server.