7
\$\begingroup\$

Coming back for another look at my project after I've reworked a few of the key points made in my previous post here.

So far, I've rewritten most of my code to address SQL Injection. I've created a class called DBManager to connect to my database and then create prepared statements and execute them.

Right now there is some duplication as I will be adding in some error logging in the near future.

The focus here should be on Security of my code. That being said, I'm not opposed to anything you have to say.

DBManager

private $db;

//create database connection - find constructive way to log errors without database if connect fails
public function __construct() {
    $this->db = new PDO('mysql:dbname='.getenv('DB_DATABASE').';host='.getenv('DB_HOST'),getenv('DB_USERNAME'), getenv('DB_PASSWORD'));
    date_default_timezone_set('America/New_York');
    $this->connectErrors();
}

//custom query generator - calls queryErrors to execute
//helper function to only expose what's necessary
public function query($sql,$bindings,$queryEnum) {
    $result = $this->queryErrors($sql,$bindings,$queryEnum);
    return $result;
}

//calls createPreparedStatement to execute, logs to database based on result
//delete and update not converted, but shows echo logging for some of the data that would be logged
private function queryErrors($sql,$bindings,$queryEnum) {
    if($queryEnum == 1) {
        //select
        $result = $this->createPreparedStatement($sql,$bindings);
    }
    else if($queryEnum == 2) {
        //insert
        $result = $this->createPreparedStatement($sql,$bindings);
    }
    else if($queryEnum == 3) {
        //delete
        if(!$result = $this->db->query($sql)) {
            echo "Sorry, the website is experiencing technical difficulties.";
            echo "Error: Our query failed to execute and here is why: \n";
            echo "Delete Query: " . $sql . "\n";
            echo "Errno: " . $this->db->errno . "\n";
            echo "Error: " . $this->db->error . "\n";
            return null;
        }
    }
    else if($queryEnum == 4) {
        //update
        if(!$result = $this->db->query($sql)) {
            echo "Sorry, the website is experiencing technical difficulties.";
            echo "Error: Our query failed to execute and here is why: \n";
            echo "Update Query: " . $sql . "\n";
            echo "Errno: " . $this->db->errno . "\n";
            echo "Error: " . $this->db->error . "\n";
            return null;
        }
    }
    else {
        //unknown
        $result = null;
    }

    return $result;
}

//connect errors currently echo to show that something went wrong
//likely return to error view to contact someone for assistance
private function connectErrors() {
    if (mysqli_connect_errno()) {
        //insert into errors table
        echo 'Error: Could not connect to the database.';
        echo "Errno: " . $this->db->errorCode() . "\n";
        echo "Error: " . array_values($this->db->errorInfo()) . "\n";
    }
}

//dynamic binding of parameters
private function createPreparedStatement($sql,$bindings) {
    $stmt = $this->db->prepare($sql);
    if($stmt === false) {
        echo 'Wrong SQL: invalid prepare statement';
    }
    $params = array();
    for($i = 0; $i < count($bindings); $i++) {
        $params[] = & $bindings[$i];
    }
    $result = $stmt->execute($params);
    if($result === false) {
        return null;
    }
    return $stmt->fetchAll();
}

This class is then connected in my PhishingController where I have a few methods (still a few redundancies) written to track the email or website connection.

PhishingController

//coming from the route, checks for valid user and calls the webbug
public function webbugEmailRedirect($id) {
    $urlId = substr($id,0,15);
    $db = new DBManager();
    $sql = "SELECT USR_Username FROM gaig_users.users WHERE USR_UniqueURLId=?;";
    $bindings = array($urlId);
    $bindingTypes = array('s');
    $result = $db->query($sql,$bindings,QueryEnum::SELECT);
    print_r(array_values($result));
    $username = $result[0]['USR_Username'];
    //check if user exists
    $this->webbugExecutionEmail($urlId,$username);
}

//base execution where duplicate methods get called
private function webbugRootExecution($urlId,$strLocation) {
    $db = new DBManager();
    if(!empty($_SERVER['REMOTE_ADDR'])) {
        $ip = $_SERVER['REMOTE_ADDR'];
        $host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
        $reqpath = $_SERVER['REQUEST_URI'];
        $projectID = substr($reqpath, $strLocation);
        $projectID = ltrim($projectID, '0');
        $projectID = rtrim($projectID, '.');
        $projectID = intval(strval($projectID));
        $sql = "SELECT PRJ_ProjectName FROM gaig_users.projects WHERE PRJ_ProjectId=?;";
        $bindings = array($projectID);
        $result = $db->query($sql,$bindings,QueryEnum::SELECT);
        print_r(array_values($result));
        $projectName = $result[0]['PRJ_ProjectName'];
        $date = date("Y-m-d");
        $time = date("H:i:s");
        return array($ip,$host,$projectName,$date,$time);
    }
    return null;
}

//email specific execution
private function webbugExecutionEmail($urlId,$username) {
    $db = new DBManager();
    $data = $this->webbugRootExecution($urlId,29);
    print_r(array_values($data));
    if(!is_null($data)) {
        $sql = "INSERT INTO gaig_users.email_tracking (EML_Id,EML_Ip,EML_Host,EML_Username,EML_ProjectName,
    EML_AccessDate,EML_AccessTime) VALUES (null,?,?,?,?,?,?);";
        $bindings = array($data[0],$data[1],$username,$data[2],$data[3],$data[4]);
        $db->query($sql,$bindings,QueryEnum::INSERT);
    }
}

As a reminder, I'm still testing most of this so I have echos and print_rs seeded throughout my methods to make sure that data is being passed and presented as it should be.

\$\endgroup\$
3
  • \$\begingroup\$ it would be better to use a framework such as Django that is easy to use and automatically protects against SQL injection, XSS, CSRF, Clickjacking, etc. \$\endgroup\$ Jul 7, 2016 at 16:50
  • \$\begingroup\$ @CrazyPython: I'd prefer to stick with what I'm comfortable with for now. While learning new frameworks and languages is ideal, this project is my senior thesis and I don't want to overextend myself with risks when I it can be done with what I already know. Thanks though! \$\endgroup\$
    – Trojan404
    Jul 7, 2016 at 16:53
  • 3
    \$\begingroup\$ definitely consider when you're in the real world - in Django you don't write SQL queries, you use OOP. Ex. Employee.objects.filter(<sql query filters>) All lazily evaluated too. Python is much easier to write and PHP is prone to security problems by nature. \$\endgroup\$ Jul 7, 2016 at 16:57

2 Answers 2

9
\$\begingroup\$

✔ Using environment variables

This is a great way to reduce the risk of credentials being leaked through version control. Kudos.

✘ No PHPDoc comments

Although pedantic, your method comments aren't structured to a way that will generate documentation for you (or others). For example, having the following as your comment structure will help both readability and debugging.

/**
 * createPreparedStatement
 * This method will prepare the query ($sql), the bind the parameters, execute the query, then return the result set.
 *
 * @param   string      $sql        The query to be prepared and executed
 * @param   array       $bindings   An array of query parameters
 * @return  var                     Either a result set array, NULL, or string.
 */

✘ Functions aren't strictly using return

Having a mixture of return and echo within your methods is a weird approach. Normalise this to only have return. Also having a mixture of return types (null, array) in one method will cause some weird logic elsewhere in your code.

✘ Exposing details

Your queryErrors() (plus connectErrors()) method is exposing some details that may help an attacker. Consider moving the current output (raw $sql, $this->db->errno, $this->db->error) to a developer log, and returning a HTTP/1.1 503 Service Unavailable or HTTP/1.1 400 Bad Request to the client with a generic error page (perhaps with an error code (that isn't generated by the DB layer) to help you traceback to what went wrong if the end-user reports it.)

Also, you're outputting very technical errors ("Wrong SQL: invalid prepare statement") which will be no use to an end-user.


Overall, the application is secure against SQL Injections. Though I cannot see any restriction (perhaps it's elsewhere in the code) to stop users from enumerating through the users table - which (using the webbugEmailRedirect looks like it sends an e-mail. Users receiving an e-mail that they didn't initiate through your application that isn't a newsletter might cause some concern for users).

I would also suggest you space your code out a bit, splitting method calls and lines below by a line for readability. For example;

public function webbugEmailRedirect($id) {
    $urlId = substr($id,0,15);

    $db = new DBManager();
    $sql = "SELECT USR_Username FROM gaig_users.users WHERE USR_UniqueURLId=?;";
    $bindings = array($urlId);
    $bindingTypes = array('s');
    $result = $db->query($sql,$bindings,QueryEnum::SELECT);

    print_r(array_values($result));
    $username = $result[0]['USR_Username'];

    //check if user exists
    $this->webbugExecutionEmail($urlId,$username);
}

It may also be worth while, since query() (which calls queryErrors()) can return null or an array is to check the key [0] exists in $result before blindly using it.

\$\endgroup\$
6
  • \$\begingroup\$ Would there be a better way to track the null then? Currently the idea is that it can return null to check if something went wrong. Would it be better to be returning false if something went wrong and null only if it executed but failed to return? \$\endgroup\$
    – Trojan404
    Jul 7, 2016 at 14:46
  • \$\begingroup\$ You need to ensure it's consistent. Right now, a method can either; echo a string and return false;, return null;, or return $array;. Usually, if there is an error, have the method throw new Exception(); (which eliminates the echo and return null;) and have just 1 return type. If, for example, a user is not found, return array(); (return an empty array), and check a key exists before using it. If it doesn't exist (ie: the array is empty), then you can handle "User not found". \$\endgroup\$
    – ʰᵈˑ
    Jul 7, 2016 at 14:48
  • \$\begingroup\$ It seems you have experience in this type of work, to an extent. What would be the best way to log errors - both connection and SQL errors? For connection, would just returning a contact support be sufficient or should I do something along the lines of generate an email to support with the error info anytime an error is generated. \$\endgroup\$
    – Trojan404
    Jul 7, 2016 at 15:01
  • \$\begingroup\$ That depends on how big your team is. Usually, I'd write to an error log with a backtrace, and e-mail the developers. Having "contact support" may be something to consider if it happens for a prolonged time (ie: your database becomes corrupt) - but your support may just be you (as well as yourself being a developer), so that's your call. Displaying a HTTP/1.1 503 Service Unavailable page is also something you could do if your team isn't big enough to handle support and development. But for an issue this large, e-mail the developers! \$\endgroup\$
    – ʰᵈˑ
    Jul 7, 2016 at 15:08
  • \$\begingroup\$ I love the well-marked nature of your reply! I feel that even just briefly glancing at it I've learned some things. \$\endgroup\$
    – Brian Risk
    Jul 7, 2016 at 21:02
4
\$\begingroup\$
private function webbugRootExecution($urlId,$strLocation) {
    $db = new DBManager();
    if(!empty($_SERVER['REMOTE_ADDR'])) {
        $ip = $_SERVER['REMOTE_ADDR'];
        $host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
        $reqpath = $_SERVER['REQUEST_URI'];
        $projectID = substr($reqpath, $strLocation);
        $projectID = ltrim($projectID, '0');
        $projectID = rtrim($projectID, '.');
        $projectID = intval(strval($projectID));
        $sql = "SELECT PRJ_ProjectName FROM gaig_users.projects WHERE PRJ_ProjectId=?;";
        $bindings = array($projectID);
        $result = $db->query($sql,$bindings,QueryEnum::SELECT);
        print_r(array_values($result));
        $projectName = $result[0]['PRJ_ProjectName'];
        $date = date("Y-m-d");
        $time = date("H:i:s");
        return array($ip,$host,$projectName,$date,$time);
    }
    return null;
}

You ought to flip this sort of thing, so that the guard clauses are more apparent and less in the way. So handle the bad case first...

private function webbugRootExecution($urlId,$strLocation) {
    $db = new DBManager();

    if(empty($_SERVER['REMOTE_ADDR'])) {
        return null;
    }

    $ip = $_SERVER['REMOTE_ADDR'];
    $host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
    $reqpath = $_SERVER['REQUEST_URI'];
    $projectID = substr($reqpath, $strLocation);
    $projectID = ltrim($projectID, '0');
    $projectID = rtrim($projectID, '.');
    $projectID = intval(strval($projectID));
    $sql = "SELECT PRJ_ProjectName FROM gaig_users.projects WHERE PRJ_ProjectId=?;";
    $bindings = array($projectID);
    $result = $db->query($sql,$bindings,QueryEnum::SELECT);
    print_r(array_values($result));
    $projectName = $result[0]['PRJ_ProjectName'];
    $date = date("Y-m-d");
    $time = date("H:i:s");

    return array($ip,$host,$projectName,$date,$time);
}

And the rest afterwards.

By doing this, (and by adding whitespace) it becomes clear that you're creating a DBManager that you're not gonna use, in certain cases. So do your guard clauses first, and THEN instantiate objects.

private function webbugRootExecution($urlId,$strLocation) {
    if(empty($_SERVER['REMOTE_ADDR'])) {
        return null;
    }

    $db = new DBManager();
    $ip = $_SERVER['REMOTE_ADDR'];
    $host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
    $reqpath = $_SERVER['REQUEST_URI'];
    $projectID = substr($reqpath, $strLocation);
    $projectID = ltrim($projectID, '0');
    $projectID = rtrim($projectID, '.');
    $projectID = intval(strval($projectID));
    $sql = "SELECT PRJ_ProjectName FROM gaig_users.projects WHERE PRJ_ProjectId=?;";
    $bindings = array($projectID);
    $result = $db->query($sql,$bindings,QueryEnum::SELECT);
    print_r(array_values($result));
    $projectName = $result[0]['PRJ_ProjectName'];
    $date = date("Y-m-d");
    $time = date("H:i:s");

    return array($ip,$host,$projectName,$date,$time);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.