1
\$\begingroup\$

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:asdf@mywebsite.com' . "\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.

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview! Please edit the title of your post to explain what the code does, not a specific question about it. Apart from that it looks good and I hope you get some good reviews, enjoy your stay! \$\endgroup\$ – ferada Sep 30 '18 at 14:32
  • 1
    \$\begingroup\$ I should point out this is a fully working script i just don't know if it's fully secure. \$\endgroup\$ – user2455808 Sep 30 '18 at 14:49
  • \$\begingroup\$ I have rolled back your edits. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Oct 1 '18 at 6:53
  • \$\begingroup\$ Could you please elaborate on the I'm assuming it's ok for those globals to exist due to the password hash? question? I don't really get what is asked here \$\endgroup\$ – Your Common Sense Oct 1 '18 at 8:52
  • \$\begingroup\$ what i was saying is it's ok if the globals are insecure due to the verification email. anyway, if i update the code a lot should i post the new code here to see if it improved by people or start a new question? i tried to change it once already and for some reason the old code was put back on? I've changed this tremendously and would like feedback on the new way. \$\endgroup\$ – user2455808 Oct 2 '18 at 0:23
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Actually, funny thing. I understand this more then functions. I can't seem to get function to work properly that's why i have removed all of them. To many times things become null and don't work for no apparent reason. How is the hash not improving security? It's directly the main security needed to verify the email? \$\endgroup\$ – user2455808 Sep 30 '18 at 23:10
  • \$\begingroup\$ you would also need to still use if/elseif even with functions. How else would you define conditions? \$\endgroup\$ – user2455808 Sep 30 '18 at 23:20
  • \$\begingroup\$ the point of $un is just a temporary variable to see if it found the username in the database. So that way if it is i can say it's already in use. \$\endgroup\$ – user2455808 Sep 30 '18 at 23:27
  • \$\begingroup\$ give users a temporary password they have to then change anyway? sounds counter productive to me. \$\endgroup\$ – user2455808 Sep 30 '18 at 23:55
  • \$\begingroup\$ i only use the $global when i need to. For some reason sometimes i have to use it and other times i don't. Probably due to the nesting. \$\endgroup\$ – user2455808 Sep 30 '18 at 23:56
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Oh dear, I completely missed that 'config2.ini' thing, that is serious, and the rest is also spot on. Luckily, and with some foresight, I claimed that my list was far from exhaustive. You get an upvote from me. \$\endgroup\$ – KIKO Software Oct 1 '18 at 18:34
  • \$\begingroup\$ The config ini is secure because of .htaccess. I have it set to DENY ALL for that config file. My host won't let me put it one folder up. I would much rather prefer that... \$\endgroup\$ – user2455808 Oct 1 '18 at 19:05
  • \$\begingroup\$ i check names existence because the next page (the php) will not have a form and it will have a resend verification for people who didn't verify correctly. \$\endgroup\$ – user2455808 Oct 1 '18 at 23:57
  • \$\begingroup\$ trim is needed so spaces are not in usernames or just having the username " " is possible without this \$\endgroup\$ – user2455808 Oct 1 '18 at 23:57
  • \$\begingroup\$ the while fetch is so the bind_result information is not used anywhere else in code and can not be altered by any means. The only period it's used is DURING the fetch command. \$\endgroup\$ – user2455808 Oct 1 '18 at 23:59
1
\$\begingroup\$

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.

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.

\$\endgroup\$

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.