OWASP A1:2017 – Injection

If you think this post’s titles sounds a bit weird for SEO, then know that it’s intentional. I did this to prevent blog posts under my PHP Basic category to directly appear under search engine results. This blog post and the following ones in this category are primarily for my own reference. I believe that “understanding” something well is a lot easier than trying to explain it to somebody. To be able to explain something well, you’ve to dig hard on the topic and improve your understanding about it. Writing about something forces me to do my own research on the topic and as a result I end up learning a lot about it. After finishing this series I’ll be more than capable of finding security bugs easily in PHP software which in turn will get me those coool CVEs.:-)

Before following this guide on injection, I highly recommend doing a quick read on injection from OWASP from this link. Also, I’ll assume you already have signed up for a free account on Secure Code Warrior or have a Business/Enterprise subscription with them. If you’re going the free route, then sign up with the PHP Basic language and then follow along with this guide. Also, the order in which I’m going to explain the challenges maybe different from yours. You can use the Skip button provided to arrive at a challenge matching mine. All that being said, let’s get started.

Table of Contents

Challenge 1: SQL Injection

Locating Vulnerability

As the challenge is about SQL injection we’ll be specifically looking for dynamic SQL queries in the app’s source code which directly take user supplied input without proper validation or sanitization before passing it to the database. As such, in the challenge you’ll see two files marked ⚠️(containing vulnerability), i.e., PersonalCabinetController.php and Report.php. We can safely ignore the PersonalCabinetController.php as it contains no SQL queries which we’re looking for. Next, in Report.php we can see that SQL queries are made via functions to save reports, get reports and delete reports. While the saveReport and getReport functions use prepared statements, the last one, i.e., deleteReport function doesn’t and is the one marked as vulnerable. The following code snippet shows the deleteReport function. Note, I’ve highlighted the lines 86 and 88 as it is done in the challenge.

   
    
/**
     * @return bool
     */
    public static function deleteReport($reportID): bool
    {
        $query = self::$database->exec('DELETE FROM reports WHERE id='. $reportID);

        if ($query === true) {
            Logger::warning('Report deleted');
            return true;
        }

        Logger::error('Report was not deleted');
        return false;
    }
}

Here, the $query variable is vulnerable as it is used to take user supplied input by adding $reportID variable to the SQL query without any validation or sanitization. Here, there are two big security issues with the function:

  1. A check for a valid user is not implemented. As such, anyone can delete any user’s reports without any authentication.
  2. The $reportID variable is not sanitized/parameterized and any malicious user can modify it to inject his own SQL queries into it. For e.g., the following curl request will delete all reports from the reports table in the database:-
curl "http://example.com/Report.php?id=99999 or '1'='1"

When the above request is made, the following things will happen in the backend:-

$reportID = "99999 or '1'='1"
$query = self::$database->exec('DELETE FROM reports WHERE id=99999 or '1'='1');

As 1 will always be equal to 1, the above query will succeed even if there is no report with id “99999” and in turn delete everything from the reports table in the database. Note, while testing this I accidently deleted everything from wp_usermeta table in my local WordPress installation. 😂

Identifying Solution

The best defense against SQL injections in PHP is through prepared statements using PHP Data Objects(PDO) which provides a data-access abstraction layer to fetch data from the database. If you don’t mind digging further into PDO in PHP, then one great resource on PDO and SQL injection in PHP is PHPDelusions. Also, if you want a more generalized SQL injection prevention guide, then the good old Bobby Tables is the way to go. There are other ways to prevent SQL injection in PHP, but I’m going to only cover Prepared Statements using PDO layer as that is the method used in the solution. Prepared Statements ensure that a malicious user cannot change the intent of a query, even if he inserts SQL commands into it. Out of all the solutions only one implements both an authenticated user check and a prepared statement. In the following code, I’ve highlighted the lines with changes made to the original vulnerable code.

/**
     * @param string $hashKey
     * @param int $reportID
     * @return bool
     */
    public static function deleteReport(string $hashKey, int $reportID): bool
    {
        $user = User::getCurrentUser($hashKey);

        $query = self::$database->prepare(
            'DELETE FROM reports WHERE user_uid=:uid AND id=:id'
        );
        $query->bindValue(':user_uid', $user->uid);
        $query->bindValue(':id', $reportID);

        return $query->execute();

        Logger::warning('Report deleted');
    }
}

Here two things are important:

  • First, an user check is implemented using the getCurrentUser function defined in User class in User.php and then stored in the $user variable. The value of $user variable is then used to bind(using bindValue function) to the :user_uid placeholder parameter which is used in the SQL statement to prepare the statement.(Note, I think there is a slight mistake in the challenge and the placeholder parameter in the bindValue function should be :uid instead of :user_uid.) This makes sure that only the user who owns the report can delete it if he is authenticated. Also, the getCurrentUser function is also secured with a prepared statement while fetching authentication details of the current user as can be seen from the code snippet below:-

/**
     * @param string $hashKey
     * @return mixed
     */
    public static function getCurrentUser(string $hashKey): bool
    {
        $query = self::$database->prepare('SELECT * FROM users JOIN token_users WHERE auth_token = :auth_token');
        $query->bindValue(':auth_token', $hashKey);
        $query->execute();

        $currentUser = $query->fetch(PDO::FETCH_OBJ);

        $date = new DateTime();

        if (!password_verify($hashKey, $currentUser->auth_token)
            && $date->format('Y-m-d H:i:s') > $currentUser->expires_at
        ) {
            Logger::error('User doesnt exists');
            return false;
        }

        return $currentUser;
    }

  • Second, the power of prepared statements is such that now if a hacker tries to use the curl command from above, the value of report id in the curl command, i.e., the entire string “99999 or '1'='1” will be binded to the “:id” placeholder parameter and then gets compared to the actual value of the report id in the database, i.e., something that doesn’t exists! Now, when the SQL query in the prepared statement finally gets executed, the query will result in a failed response of 404 NOT FOUND!

Challenge 2: SQL Injection

Locating Vulnerability

Following our methodology from Challenge 1, we’ll be looking for SQL queries in the source code that doesn’t use prepared statements or does any sort of user input validation/sanitization. As such, after going through all the code marked with ⚠️, you’ll find that the getReports function in Report.php doesn’t use prepared statements and executes the raw SQL query without validating/sanitizing the $reportID variable.

/**
     * @param array|null $reportIDs
     * @return mixed
     */
    public static function getReports(?array $reportIDs = null)
    {
        if ($reportIDs !== null) {
            foreach ($reportIDs as $reportID) {
                $sql = 'SELECT * FROM reports WHERE id=' . $reportID;
                $queryObject = self::$database->query($sql);
            }

            return $queryObject;
        }

        $sql = 'SELECT * FROM reports';

        return self::$database->query($sql);
    }

Here, the difference is that an empty array $reportIDs is created and reports($reportID) are appended to it. But, as before there are two big security issues here. First, there is no user check, which means anybody can fetch all users reports without the need for any authentication, for e.g., on line 66 a query is done to fetch all the reports but it was not specified that for which user the reports must be fetched, as such when the function gets called all reports from reports table in the database is fetched. Second, there is no validation/sanitization applied on the user supplied input via the $reportID variable which means the same curl command I explained in Challenge 1 can be used to fetch all users reports.

Identifying Solution

As explained in the solution for Challenge 1, the best defence against SQL injection in PHP is using a PDO layer. As before, out of all the solutions only one implements both an authenticated user check and a prepared statement. I’ve highlighted the secure code changes made to the original vulnerable code in the following code snippet.

/**
     * @param string $hashKey
     * @param array|null $reportIDs
     * @return mixed
     */
    public static function getReports(string $hashKey, ?array $reportIDs = null)
    {
        $user = User::getCurrentUser($hashKey);

        if ($reportIDs !== null) {
            $query = self::$database->prepare(
                'SELECT * FROM reports WHERE user_uid=:uid AND id=:id'
            );
            foreach ($reportIDs as $reportID) {
                $query->bindValue(':uid', $user->uid);
                $query->bindValue(':id', $reportID);
                $query->execute();
            }
            return $query->fetch(PDO::FETCH_OBJ);
        }

        $query = self::$database->prepare(
            'SELECT * FROM reports WHERE user_uid=:uid'
        );
        $query->bindValue(':uid', $user->uid);
        $query->execute();

        return $query->fetch(PDO::FETCH_OBJ);
    }

Here, again the $getCurrentUser function, as explained in Challenge 1, is used to do an user authorization check before fetching reports from the database. Also, there are a couple of neat steps taken here to secure the code. First, there is a clear use of PDO with prepared statements which is a sign of secure code. For e.g., now a report is only fetched if an user is authenticated and the report belongs to him, i.e., he is authorized to access it. Second, the query to fetch all reports from the reports table is now associated with an user check where the reports pertaining to an authenticated user is fetched from the reports table.

Challenge 3: Path Traversal

A Path Traversal or Directory Traversal is a type of attack which can be leveraged to access files outside webroot folder. Insufficient validation/sanitization of user supplied input for file names can lead to path traversal vulnerability. For e.g., an user’s profile picture is stored in the “/var/www/app/images” folder, a malicious user can make a GET request to his own profile pic but instead of fetching the profile pic he can manipulate the URL to access a file outside that folder with “dot-dot-slash (../)” sequences. Take a look at the curl request below:-

curl "http://example.com/user/profile/image.php?image=../../../../etc/passwd"

The above request will return the contents of unix passwd file on the server if the webapp has not implemented any form of validation on user supplied filename. As such, an attacker can read any files on the underlying host with the privileges of the web service user(like www-data).

Locating Vulnerability

As this challenge involves accessing files, we’ll be looking for vulnerable functions in the source code that handle user supplied input filename without any proper validation/sanitization. After auditing the source code, you’ll arrive at the uploadPhoto function in PersonalDataService.php which uses the superglobal $_FILES file upload variable to handle user supplied images. The vulnerable code is as follows:-

 /**
     * @return bool
     */
    public function uploadPhoto(): bool
    {
        return User::addPhoto($_FILES['file']['name']);
    }
}

The superglobal $_FILES variable has a few properties. In the above code, the ['file'] property indicates the name of the input field where the user will enter the path to the file on his local machine and the [‘name’] field indicates the original filename of the file provided by the user. The vulnerability here is that an user provided filename is used to upload and store the file without any validation of the filename he submits or path/directory where it should be stored. Also the lack of proper validation can allow an attacker to modify or access files outside user’s directory. If the code is modified to implement an check to force the file to be stored and fetched from a specific valid path, for e.g., an user’s directory inside the webroot folder, then the user won’t be able to traverse his own directory on the webserver by providing malicious input filenames. If this sounds confusing, then just be patient as once I explain the solution everything will be clear.

Note:- the addPhoto function in the above code which can be found in application/models/User.php uses prepared statement to store the path to the location of user uploaded image and just inserts an reference – of the user profile pic to its physical location on the server – into the database. Also, in the above code there is no check for file extension or file size, which can lead to other serious security issues themselves.

Identifying Solution

As this is a path/directory traversal vulnerability we should be looking at code which implements a “valid path” check where the image will get stored along with other image file related security checks. As such, there is only one solution that implements a check if a valid path for the image exists or not. Note, I’ve highlighted the changes from the original vulnerable code.

    /**
     * @param string $hashKey
     * @param Request $request
     * @return bool
     */
    public function uploadPhoto(string $hashKey, Request $request): bool
    {
        $post = $request->getParsedBody();
        $user = User::getCurrentUser($hashKey);
        $userDir = $this->service->getUserDir($user);
        $name = basename($_FILES['photo']['tmp_name'] . '-' . $user->uid);
        $fileSize = $post['photo']['size'];

        if ($userDir && CheckDataService::isValidImage($fileSize) && $this->service->isValidPath($hashKey)
        ) {
            $destination = $userDir . 'images';
            move_uploaded_file($name, $destination);
            return User::addPhoto($destination);
        }

        return false;
    }
}

The above code snippet has a lot of security features added into it. I’ll try to explain the code line by line.

  • First, on line 79 an user check is implemented using the getCurrentUser function from User class which retrieves an user from database based upon whether they are authenticated or not. The value is stored into $user variable which’ll be used later.
  • An check for an existing user directory of the authenticated user(determined via $user variable in line 79) is made on line 80 using the $getUserDir function from CheckDataService.php.
/**
     * @param User $user
     * @return false|string
     */
    public function getUserDir(User $user)
    {
        if (!$user && $user->id === null) {
            return false;
        }

        return Config::getConfig()['users_storage'] . $user->id . DIRECTORY_SEPARATOR;
    }
  • In line 81, instead of using an user provided filename as input, an temporary filename is allocated to the uploading file with the help of ['tmp_name'] property of $_FILES variable. This helps in preventing directory traversal when using an user provided filename with (../) sequences. The value of the filename is stored into the $name variable.
  • The $filesize variable is used to get the size of the image file from the request body in line 82. This will be used later to add an check for file upload size limitations.
  • Next, an if statement on line 84 is used to check whether the user’s directory exists with the $userDir variable and checks whether the file being uploaded is withing file upload size restrictions limit via the isValidImage(see code below) function of CheckDataService class and finally it checks whether the path is a real path and validates it with the users directory on the server with the help of isValidPath(see code below) function from the same class. When all the three conditions in the if statement are met, the code block from line 86 to 88 is carried out where the image file is moved to its predetermined destination, i.e, the user’s directory on the webserver with the help of move_uploaded_file() function and a reference to its location is added in the database via the addPhoto function. This ends up preventing the user from accessing or modifying files outside his own directory on the server.
    /**
     * @param int $fileSize
     * @return bool
     */
    public static function isValidImage(int $fileSize): bool
    {
        return !self::checkMimeType() && $fileSize > 2097152 === false;
    }

    /**
     * @return bool
     */
    public static function checkMimeType(): bool
    {
        $fileType = mime_content_type($_FILES['photo']['tmp_name']);

        return (\in_array($fileType, ['jpg' => 'image/jpeg', 'png' => 'image/png',], true));
    }

    /**
     * @param string $hashKey
     * @return bool
     */
    public function isValidPath(string $hashKey): bool
    {
        $user = User::getCurrentUser($hashKey);

        $realpath = rtrim(realpath($this->getUserDir($user)), DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR;

        if (($safe_root = $this->getUserDir($user)) !== false) {
            return !(strpos($realpath, $safe_root) !== 0);
        }

        return false;
    }

Note:- I didn’t explain something very important which can lead to potential RCE on the server. I leave this as a note for my future self when I’ve to refer this post. Can you guess from above code snippet what it is? Hint:- It involves file extensions.

Conclusion

Secure coding practices can drastically improve the security of a webapp but one must not solely rely on it to prevent Injection attacks. As a first line of defense, I recommend using a Web Application Firewall(WAF) which will block the malicious code even before it reaches your server. A guide from me on WAF is long due, but the WAF I’m using in my prod server is an extremely complex beast and explaining it in detail will require me to write multiple posts as I’m unable to think of a way to explain it in a single post.









P.S:- Man, explaining security in code is so time consuming. Honestly, writing about the first challenge was all fun and glory but then the fun slowly started fading as I wrote the other ones. My initial idea was to give each challenge it’s separate post which would’ve reduced the pressure on me and I’d have written every challenge more comfortably but then I decided against it and the end result was this huge monstrosity of a post. I’m sure I could’ve written better than this, but at this point I don’t care. On the bright side, I learned so much that I think by the end of writing about all the OWASP Top 10 categories, I’ll be a PHP security expert and getting all those coool CVEs which could possibly get me an InfoSec job.:) TL;DR. If anybody comes across this post and manages to read it this far, then thanks for reading.

Leave a Reply

Your email address will not be published. Required fields are marked *