0

So our ancient web app manages to pass an annual code review with minimal red flags. One of those possible security risks was this function below that gets a user's session info.

private function getJobTrackerSessionInfo()
{
    // Get session data from database
    $query ='SELECT `a_session` FROM `sessions` WHERE id = ?';
    $a_session = $this->db->getOne($query,null,array(session_id()));

    //not logged in returns false
    if(empty($a_session)) return array();

    //convert session stuff from the database into a php array
    $a_session = str_replace("{","array(",$a_session);
    $a_session = str_replace("}",")",$a_session);
    $a_session = str_replace(";\$D","",$a_session);
    @eval($a_session);

    return $D;
}

This private function call is done after a CSRF token verification. I'm new to PHP and SQL database calls. I can understand it, but as far as writing securely, I'm out of my element. This is currently a web app running in like PHP 5, but we do plan on bringing it past 7+ by the summer.

I was reading through this Stackoverflow thread here: How can I prevent SQL injection in PHP?

It may be dated, but the user replies below had a lot of useful information on depreciated and up to date preventative methods.

Is there a good updated solution to prevent any sort of injection during this specific query and eval?

Edit: Removing the eval() function call seems to be what could help secure this more, but the rabbit hole goes deeper. getOne() is supposed to be called and populate a_session. However, I can't seem to find where the heck the getOne() function lives. I did find that it belongs to some sort of sql dev api that can be reviewed here: https://www.php.net/manual/en/mysql-xdevapi-collection.getone.php

Edit 2: From dumping $a_session, I found that it is code written to apply an array of values to $D as a string meant to execute in eval() Okay, that seems obvious, but it isn't just containing $_SESSION info parameters, but also user and token info. I guess the author's original thought process was to maybe keep those sensitive user values hidden when assigning and returning them?

Could I just parse the string, assign array(a bunch of values) to a value, and just do $D = $myParsedArray. $myParsedArray would still be interpreted as a string though, right?

I need to think on this one. I'll post updates with any progress.

Powermaster Prime
  • 402
  • 1
  • 2
  • 16
  • 2
    _"Is there a good updated solution to prevent any sort of injection"_ Yeah, don't use `eval()`. – Alex Howansky Jan 26 '22 at 16:30
  • How does the string `$a_session` looks like from `$this->db->getOne`? – ikhvjs Jan 26 '22 at 16:31
  • or create a new modified function that does the same as that –  Jan 26 '22 at 16:32
  • I think you should udpate the `getOne` function to return a pure php array instead of using `eval` – ikhvjs Jan 26 '22 at 16:39
  • When it comes to protecting against SQL injections, then that post is till 100% relevant/valid. – M. Eriksson Jan 26 '22 at 16:52
  • I'll investigate removing the ```eval()``` function call, but I ran into an even weirder issue I'll have to edit my question with. I can't find ```getOne``` anywhere in our file directory. getOne seems to belong to some sort of devAPI I found information on here: https://www.php.net/manual/en/mysql-xdevapi-collection.getone.php I have no idea where this function is being called from in our web app. I'll have to return with some more info as I find it. – Powermaster Prime Jan 26 '22 at 16:53
  • What class is `$this->db` an instance of? – Alex Howansky Jan 26 '22 at 17:00
  • ```$this->db``` is an instance of the ```db_connect()``` function in our ```database.php``` file. Speaking of, db_connect is also depreciated, and due for an update as we transition to a newer PHP version – Powermaster Prime Jan 26 '22 at 17:12
  • 1
    Your `.getOne()` method must be declared in a database class in your application. The `.getOne()` method you use has different parameters from the one in php's [mysqlx](https://www.php.net/manual/en/book.mysql-xdevapi.php) API to [MySQL's document store](https://dev.mysql.com/doc/refman/8.0/en/document-store.html) functionality. At any rate it looks like your `.getOne()` uses bind parameters. That's the right way to avoid SQL injection. But `eval()` is still very dangerous indeed, as it runs whatever code you give it. – O. Jones Jan 26 '22 at 17:16
  • 1
    You can't have an instance of a function, it has to be an instance of a class. The db_connect() function instantiates a new object and returns it. What type of object? – Alex Howansky Jan 26 '22 at 17:19
  • Oops, my apologies, ```db_connect()``` is returning a variable called ```$mdb2```. For context, ```$mdb2 = MDB2::singleton();``` MDB2 seems to be some sort of package the web app is using: https://pear.php.net/package/MDB2 Could be why I can't find getOne() anywhere either? – Powermaster Prime Jan 26 '22 at 17:31
  • If all you're getting there is a straight-up serialized `$_SESSION`, simply `$_SESSION = unserialize($a_session);` would reinstate a given session's variables? Really need to know what you're receiving to know what to do with it. In any case, `eval` doesn't seem to be the answer. – Markus AO Jan 26 '22 at 18:21
  • Hey Markus, I dumped the ```$a_session``` variable, its basically a string of code that makes ```$D``` equal to a array of ```$_SESSION``` parameters, as well as user specific info, like username, email, userid, CSRFToken, and a few other things. – Powermaster Prime Jan 26 '22 at 18:35

0 Answers0