2
\$\begingroup\$

So I have two PHP files that execute SQL code in them. It's a simple registration script and an account recover script.

I want to know is my code safe from SQL exploits & other exploits? Here's how my code works.

  1. how my registration system works

    a person goes to my url with specified data such as the following example.

    http://localhost/registeruser.php?identity=438746285267827419&idnumber=2201

  2. how my recovery account system works

    user goes to this url with this specified data passed through.

    http://localhost/accountrecovery.php?secretcode=GU3DZ99S4D73D9G7H

Below is the code for my following files

  • registeruser.php
  • accountrecovery.php

registeruser.php

<?php
setcookie('timerValueHolder', 0);
?>
<?php
if (!isset($_SESSION)) session_start();
$timeCurrently = round(microtime(true));
$UserRegTime = (isset($_SESSION["timeLastAccessed"])) ? $_SESSION["timeLastAccessed"] : '0';
if (($timeCurrently - $UserRegTime) > 3)
{
    $_SESSION['timeLastAccessed'] = $timeCurrently;
}
else
{
    header('refresh: 1');
    die("cannot continue because you must wait " . (3 - ($timeCurrently - $UserRegTime)) . " seconds.");
}
$title = "User registration";
require_once ("header.php");
$passedInfo = $_GET['identity'];
$passedInfoTwo = $_GET['idnumber'];

if (strlen(trim($passedInfoTwo)) < 1)
{
    echo "Invalid identification number of your account.";
    setcookie('timerValueHolder', 0);
}

if (strlen(trim($passedInfo)) < 1)
{
    echo "Invalid identification number of your account.";
    setcookie('timerValueHolder', 0);
}

if ($_COOKIE['timerValueHolder'] >= 0)
{

    if (strlen(trim($passedInfo)) > 0)
    {
        if (!isset($_COOKIE['timerValueHolder']))
        {
            setcookie('timerValueHolder', 0);
        }
        if (isset($_COOKIE['timerValueHolder']) && $_COOKIE['timerValueHolder'] < 4)
        {
            $servername = "localhost";
            $username = "admin";
            $password = "abc123";
            $dbname = "databaseholder";
            $conn = new mysqli($servername, $username, $password, $dbname);
            if ($conn->connect_error)
            {
                die("Failed to Complete Connection: " . $conn->connect_error);
            }
            $usersIPAddress = $_SERVER['REMOTE_ADDR'];
            $checkAction = mysqli_real_escape_string($conn, "select * from userconfiguration where membersID='$passedInfo' and MemberName<>''");

            if ($checkAction > 0)
            {

                $checkActionRows = mysqli_num_rows($checkAction);
                if ($checkActionRows > 0)
                {
                    echo "please wait as web page refreshes until you see a successful message.";
                }
            }
            else
            {
                $secondaryCheck = mysqli_real_escape_string($conn, "select * from userconfiguration where ipaddress='$usersIPAddress' and membersID='$passedInfo'");
                if ($secondaryCheck > 0)
                {
                    $rowCheckTwo = mysqli_num_rows($secondaryCheck);
                    if ($rowCheckTwo > 0)
                    {
                        echo "please wait as web page refreshes until you see a successful message.";
                    }
                }
                else
                {
                    $sql = "INSERT INTO userconfiguration (ipaddress, membersID, TheirName)
VALUES ('$usersIPAddress', '$passedInfo', '$passedInfoTwo')";
                    if ($conn->query($sql) === true and $secondaryCheck > 0 and $rowCheckTwo > 0)
                    {
                        echo "please wait as web page refreshes until you see a successful message.";
                    }
                    else
                    {
                        echo "please wait as web page refreshes until you see a successful message.";
                    }
                }
            }
            $conn->close();
            $current_val = $_COOKIE['timerValueHolder'];
            $current_val++;
            setcookie('timerValueHolder', $current_val);
            echo $_COOKIE['timerValueHolder'];
            header('refresh: 4');
        }
        else
        {
            echo "success. go on to our application and finalize the registry by typing #finalizeregister ";
            echo $_GET['identity'];
            setcookie('timerValueHolder', 0);
        }
    }
}
?>

accountrecovery.php

<?php
setcookie('timerValueHolder', 0);
?>
<?php
if (!isset($_SESSION)) session_start();
$timeCurrently = round(microtime(true));
$UserRegTime = (isset($_SESSION["timeLastAccessed"])) ? $_SESSION["timeLastAccessed"] : '0';
if (($timeCurrently - $UserRegTime) > 3)
{
    $_SESSION['timeLastAccessed'] = $timeCurrently;
}
else
{
    header('refresh: 1');
    die("cannot continue because you must wait " . (3 - ($timeCurrently - $UserRegTime)) . " seconds.");
}
$title = "user recovery";
require_once ('header.php');
$servername = "localhost";
$username = "admin";
$password = "abc123";
$dbname = "databaseholder";

$conn = new mysqli($servername, $username, $password, $dbname);

if ($conn->connect_error)
{
    die("Failed to Complete Connectio: " . $conn->connect_error);
}

$passedData = $_GET['secretcode'];

$dataPassedTwo = mysqli_real_escape_string($conn, $passedData);

$actionCheck = mysqli_query($conn, "select * from userconfiguration where recoveryCode='$dataPassedTwo'");

$rowCheckAction = mysqli_num_rows($actionCheck);
$rowCount = mysqli_fetch_row($actionCheck);
if ($rowCheckAction > 0 and strlen(trim($passedData)) > 0)
{
    echo "account recover details are ";
    echo " your password: ", $rowCount[4];
    echo " your security pin: ", $rowCount[5];
    echo "to recover your account in the future you must do the following task.";
    echo "in our application type #finalizeregister to obtain a new recovery code.";

    $update = mysqli_query($conn, "UPDATE userconfiguration SET recoveryCode = '' WHERE recoveryCode = '$dataPassedTwo'");

    if (!$update)
    {
        echo "An issue has occured in the update task.";
    }
}

else
{
    echo "failed to recover account. try typing #finalizeregister in our application for a new code to generate.";
}
$conn->close();
?>
\$\endgroup\$
5
  • 3
    \$\begingroup\$ I see lots of "interesting" code here. Tell me why you are doing this: session_start(); if (!isset($_SESSION)) session_start();. Have you heard of "prepared statements"? \$\endgroup\$ Nov 7 '20 at 8:37
  • 2
    \$\begingroup\$ Welcome to Code Review, please edit your question so that it only states the task accomplished by the code, anything else belongs to the body of the question \$\endgroup\$
    – user228914
    Nov 7 '20 at 9:01
  • 1
    \$\begingroup\$ @mickmackusa wow... i didn't notice the part you pointed out before expressing the usefulness of prepared statements ty brother i will fix that. but mick, can't you say if i use escape strings & if I use 'utf8mb4' format for mysql database it might as well be as safe as prepared statements? \$\endgroup\$ Nov 7 '20 at 11:49
  • 2
    \$\begingroup\$ @AryanParekh thank you for the advice i formatted my post as best as I could in regards to your response. \$\endgroup\$ Nov 7 '20 at 11:59
  • 1
    \$\begingroup\$ security.stackexchange.com/q/116670/208614 \$\endgroup\$ Nov 7 '20 at 12:36
7
\$\begingroup\$

No, your code is not safe. Far from it. It's purely broken. Let's get into details.

Registration

I will go through the full code line by line and point each problem with it.

  1. Why is your script divided into two parts? What is the purpose of
    <?php
    setcookie('timerValueHolder', 0);
    ?>
    
    If this is part of your PHP logic then why escape into HTML context and then back into PHP? You really should name your cookies better. Each name should clearly describe the purpose.
  2. Why is your session_start() conditional? This is an indicator that you are losing control of your source code and it might be a spaghetti code. A session should be started only once per each execution of the script.
  3. Why round(microtime(true))? Why not simply time()?
  4. Use null-coalesce operator instead of ternary operator.
    $UserRegTime = isset($_SESSION["timeLastAccessed"]) ? $_SESSION["timeLastAccessed"] : '0';
    // change to
    $UserRegTime = $_SESSION["timeLastAccessed"] ?? 0;
    
  5. Using die() in a web application is not recommended. Try to redesign your code to avoid dying at all.
  6. What is require_once "header.php";? Why are you including some random file in the middle of executing something else?
  7. You never check for the existence of your GET values. How about using filter_input() instead?
  8. Again, the names of your variables don't describe the contents properly. What is $passedInfo and $passedInfoTwo?
  9. strlen(trim($passedInfoTwo)) < 1 means "if non-zero byte long string". You can just do if (!trim($passedInfoTwo))
  10. Why do repeat setcookie('timerValueHolder', 0); several times? You already sent that cookie.
  11. Why is echo "Invalid identification number of your account."; repeated twice? Should it be two different messages? Are they meant to be validation messages? Why does the script continue execution despite having no values?
  12. if (isset($_COOKIE['timerValueHolder']) inside of if ($_COOKIE['timerValueHolder'] >= 0) ???

At this point I lost interest in trying to figure out what you are doing with user input. Cookies, GET, Sesssion. What does it all mean? What are you trying to achieve?

MySQLi

  1. Why are you using mysqli? Why do you have variables for mysqli arguments? Why are these values hardcoded?
  2. You should enable error reporting for mysqli. Add this line before new mysqli:
    mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
    
  3. NEVER check for mysqli connection errors manually and NEVER display them to the user! Remove the whole if statement after new mysqli.
  4. Set the correct charset. The correct one for MySQL should be utf8mb4 unless you are using something else. If you use anything else then why?
  5. mysqli_real_escape_string does not do what you think it does. I don't even know what you think it does. The next few lines of code make no sense at all. Just remove it all.
  6. Your code is wide open to SQL injection! You must use prepared statements. Always bind values as parameters.

Assuming you wanted to perform some kind of existance check in your database the code should look something like this.

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
$mysqli = new mysqli('localhost', 'username from ENV', 'pass from ENV', 'databaseholder');
$mysqli->set_charset('utf8mb4');

$stmt = $mysqli->prepare('SELECT COUNT(*) FROM userconfiguration WHERE membersID=? AND (MemberName<>"" OR ipaddress=?)');
$stmt->bind_param('ss', $passedInfo, $usersIPAddress);
$stmt->execute();
$result = $stmt->get_result();
$exists = (bool) $result->fetch_row[0];

if($exists) {
    // error message to the user
}

Same for the insert. Remove that if statement. Use a prepared statement.

$stmt = $mysqli->prepare('INSERT INTO userconfiguration (ipaddress, membersID, TheirName) VALUES(?,?,?)');
$stmt->bind_param('sss', $usersIPAddress, $passedInfo, $passedInfoTwo);
$stmt->execute();

Security

Your obvious problem is the glaring SQL injection. For this reason alone that code should be scraped and written again. Preferably using PDO this time. You must always bind data using SQL parameters.

I have no idea what you are doing with cookies. I would not trust the user with the cookies and I see no reason to use them in your code at all.

You are inserting data taken from GET. GET as the name suggests should be used to get the data from PHP not add it. To add data use POST method. This prevents crawlers, previews and other stuff like this from accidentally executing your code. GET operations should be idempotent and should not cause side effects.

Your recoveryCode is actually a password that is called something else. You should never ever store passwords on your server. Such things should be hashed and salted. PHP has a function for this called password_hash(). Secret code is not secret if your server knows it.

Your code is also vulnerable to XSS although it's difficult to assess to what degree. Do some research about printing data in documents like HTML. You must properly format the data so that it can never be interpreted as HTML code.

There are also other problems, but I have not enough information to talk about them here. What is identity in your URL? What is idnumber? Are these sequential numbers? If not how much entropy do they have? Can I guess it? Your secretcode certainly does not look like it has enough entropy. How is it created?

HTTP? Surely, this is only for development on localhost, right? If you ever expose this to the internet you must use HTTPS.

Conclusion

This code has plenty of flaws. In fact, it has so many problems that I deem it unfixable. Delete all of it and before you start writing it again spend a number of days reading about web security. There are plenty of good resources online. https://paragonie.com/ has some good articles on their blog. https://stackoverflow.com/ and https://security.stackexchange.com/ also have plenty of good information.

When you start writing this again, use a proper IDE with syntax highlighting and syntax checking. Format the code properly. Read about PHP PSR standards. If you are unsure about something do some reading instead of copying code or trying to guess how it works.

\$\endgroup\$
5
  • \$\begingroup\$ honeestly as harash as you were (LOL) i am happy that u were XD my brother your response was above & beyond helpful. i cant express how f****** appreciative i am of you for taking the initative and putting my code and me in it's place. this is just what i needed. tysm brotha <3, so as we speak I am implementing every single detail u pointed out which needed fixed. ur an angel man im going to le4ave this question open if all else fails but for-reals the time, effort, & patience that went into ur post, i am learning alot and i do not feel confused thanks to u. bless u my man. @Dharman \$\endgroup\$ Nov 8 '20 at 2:13
  • 1
    \$\begingroup\$ @ZERO I would like to add that you should not be accessing any additional resources until all required user input is validated. This mean that you check the quality of $_GET['secretcode']; BEFORE you bother to make any trips to the database. For this reason, you shouldn't even bother connecting to the database before you determine that it is necessary to do so. Also, while in the context of this question point #9 is true, it is not always true. 3v4l.org/tQm31 \$\endgroup\$ Nov 8 '20 at 9:35
  • \$\begingroup\$ @mickmackusa thank you for staying on top still responding to my post. what you've stated i've documented in my sticky-notes & am reading it over it again & again to engrain these fundamentals in my skull obviously dont want to overlook somthing and miss even the slightest detail leaving my code insecure. I surely am starting to grasp what u guys are teaching me. appreciate it. \$\endgroup\$ Nov 8 '20 at 10:33
  • \$\begingroup\$ stackexchange, thank-you for all the help it's nice having people actually provide details that actually improve my programming skills. just for awareness of my post, i'm going to leave it open a little bit longer but as of now rewriting my code with all the help responses is going in my favor. \$\endgroup\$ Nov 8 '20 at 10:36
  • \$\begingroup\$ @ZERO I personally invited Dharman to offer a review on this question because I absolutely knew that he would be able to offer a comprehensive review. I have done this several times before (like this guy who I met through Joomla connections: codereview.stackexchange.com/a/244209/141885). I always like to see people receive great reviews -- I am not always the best person for the job. \$\endgroup\$ Nov 8 '20 at 11:03

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.