WebEdition 6.3.8-s1 SQL Injection Vulnerability

WebEdition CMS is an open source CMS written in PHP that seems to be mostly used by german websites. It came to our attention a few months ago, because another party performed an audit on it and came up with some vulnerabilities. Because we always look for nice PHP bugs for our own PHP and web security trainings we had a very quick look into it and were able to find a number of vulnerabilites that we disclosed to the vendor.

The most serious vulnerability that we discovered was a remote PHP code execution vulnerability that we discussed in a previous blog entry. Because the disclose process took a while for the previous vulnerability we accidentally spend a bit more time in the code and realized a vulnerability in their handling of UPDATE and INSERT by some helper functions they use. This vulnerability allows for SQL injections into UPDATE/INSERT SQL statements that make use of these helper functions. The vendor incorporated a fix for this vulnerability into the release of WebEdition 6.3.8-s2. Unfortunately the vendor was very hard to work with and they decided to not ask us for help or review of their fixes. Therefore as you will learn at the end of this blog posting their fix is incomplete.

As mentioned in our previous blog posting the only way to get this security patch is to use their OnlineInstaller. They still do not have a tarball for you to download. We strictly advise against shipping PHP software in this way, because it requires the document root and all its subdirectories to be writable, which is a very bad idea in terms of security.

In this post we will discuss the SQL injection vulnerability in WebEdition's UPDATE/INSERT helper functions and show how they fixed the problem and discuss why this is incomplete.

The Vulnerability

When you look into how WebEdition handled SQL queries you will sooner or later see that they use some helper functions to handle UPDATE and INSERT statements, if multiple database fields require updating. Here is one example SQL query from bannerclick.php:

<?php
$db->query('INSERT INTO ' . BANNER_CLICKS_TABLE . ' SET ' . we_database_base::arraySetter(array(
    'ID' => intval($id),
    'Timestamp' => sql_function('UNIX_TIMESTAMP()'),
    'IP' => $_SERVER['REMOTE_ADDR'],
    'Referer' => ($referer ? $referer : (isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER'] : '')),
    'DID' => intval($did),
    'Page' => $page
)));

As you can see they use a helper function called we_database_base::arraySetter to convert an array of values into a comma separated list of SQL assignments. The code for this can be found in /we/include/we_classes/database/we_database_base.class.php:

<?php

abstract class we_database_base{

  static function arraySetter(array $arr, $imp = ','){
    $ret = array();
    foreach($arr as $key => $val){
      $escape = !(is_int($val) || is_float($val));
      if(is_array($val) && $val['sqlFunction'] == 1){
        $val = $val['val'];
        $escape = false;
      } elseif(is_object($val) || is_array($val)){
        t_e('warning', 'data error: db-field cannot contain objects / arrays', 'Key: ' . $key, $arr);
      }

      ...
      $ret[] = '`' . $key . '`=' . ($escape ? '"' . escape_sql_query($val) . '"' : $val);
    }
  return implode($imp, $ret);
}

In this code you can see that they use a database escaping function escape_sql_query() to prepare values before they are inserted in between double quotes inside the SQL statement. For completness lets have a look into the escape_sql_query() function although it is not involved in the SQL injection problem we reported. The function can be found in/we/include/we_db_tools.inc.php.

<?php

function escape_sql_query($inp){
  if(is_array($inp)){
    return array_map(__METHOD__, $inp);
  }

  if(!empty($inp) && is_string($inp)){
    return str_replace(array('\\', "\0", "\n", "\r", "'", '"', "\x1a"), array('\\\\', '\\0', '\\n', '\\r', "\\'", '\\"', '\\Z'), $inp);
  }
  return $inp;
}

First of all this function might do the job for western character single byte character sets or even in case of UTF-8, but it is dangerous the moment the database runs with another multibyte character set. Because of this you should always use database specific escaping functions that are aware of the current character set of the database.

But lets ignore that for now, because for the mostly german installations this is not really a problem. The problem is located inside the we_database_base::arraySetter() method itself. Take a look at the following lines of code and let them sink in a bit.

<?php

  $escape = !(is_int($val) || is_float($val));
  if(is_array($val) && $val['sqlFunction'] == 1){
    $val = $val['val'];
    $escape = false;
  } ... {}

  ...
  $ret[] = '`' . $key . '`=' . ($escape ? '"' . escape_sql_query($val) . '"' : $val);

This code has some hardcoded special cases that do not get escaped at all. The exceptions are integer values, floating point values and caller submitted SQL function invocations. Now look at the code again how do they detect a caller submitted sqlFunction? They detect it by seeing that the value provided is actually and array and contains the key sqlFunction which is set to 1.

This should raise an alarm in your head, but if not remember that the values provided are actually from user input. What if that user input is an array instead of a string, what if the array contains a key sqlFunction that is set to 1 and some malicious data in a key called val? Should that ever happen then the code will use the malicious value from the val key and do not escape it at all, which will allow SQL injection.

Now before getting too excited we have to check if this can ever happen and therefore we need to backtrack some calls to arraySetter and evaluate the input parameters. Whilewe_database_base::arraySetter() is called a number of times we can find some problematic case easily inside getBanner.php. But this is not the only place.

<?php

...
$page = isset($_GET["page"]) ? $_GET["page"] : "";
...


    $GLOBALS['DB_WE']->query('INSERT INTO ' . BANNER_VIEWS_TABLE . ' SET ' . we_database_base::arraySetter(array(
        'ID' => intval($id),
        'Timestamp' => sql_function('UNIX_TIMESTAMP()'),
        'IP' => $_SERVER['REMOTE_ADDR'],
        'Referer' => ($referer ? $referer : (isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER'] : '')),
        'DID' => intval($did),
        'Page' => $page)));

As you can see the $page variable is populated directly from user input inside the URL $_GET["page"] and used in the call to we_database_base::arraySetter(). This means if our URL contains the following it will result in SQL injection.

 .../getBanner.php?...&page[val]=MALICIOUS_SQL_INJECTION&page[sqlFunction]=1&...

With a bit of patience you could now evaluate every single call to we_database_base::arraySetter() and find some other potential entrypoint for this SQL injection problem. We leave this excersise to the reader. Well the correct way to fix this problem is to fix the we_database_base::arraySetter() anyway, which is excatly what the vendor attempted. Unfortunately they partially failed.

The Vendor's Fix

Unfortunately the vendor was very unresponsive and we could only make them acknowledge having received our emails by repeatedly emailing them. They also decided to just go with a fix and did not ask us prior to release for a review of said patch. The first part of their fix is located inside the ** function.

<?php

static function arraySetter(array $arr, $imp = ','){
  $ret = array();
  foreach($arr as $key => $val){
    if($key === ''){
      continue;
    }
    $escape = !(is_bool($val));
    if(is_array($val) && sql_function($val)){
      $val = $val['val'];
      $escape = false;
    } ... {}

    $ret[] = '`' . $key . '`=' . ($escape ? '"' . escape_sql_query($val) . '"' : $val);
  }
  return implode($imp, $ret);
}

The new code basically moves the verification of what is a SQL function and what isn't into the function sql_function() which is defined in /we/include/we_db_tools.inc.php. The purpose of this function is twofold. On the one hand it is used for marking a variable as being an SQL function call and on the other hand it validates if a submited variable is an SQL function.

<?php

function sql_function($name){
  static $data = 0;
  if(!$data){
    $data = md5(uniqid(__FUNCTION__, true));
  }
  return (is_array($name) ? isset($name['sqlFunction']) && $name['sqlFunction'] == $data :
          array('sqlFunction' => $data, 'val' => $name));
}

The idea here is that a static PHP variable called $data is used to keep track of a randomly generated md5 hash throughout a PHP request. The value inside the array key sqlFunction is then verified against this randomly generated value. Here uniqid() is called with its second parameter set to true which means it returns a value that is based on the function name, the current timestamp including microseconds and one output of PHP's internal LCG. Someone with a degree in math can maybe tell you how this results in only sooo many different combinations. But we want to look at another aspect of this check.

When you look at the code you will see that it uses the == operator instead of the === operator. This is always a bad idea in security checking code. The == operator performs conversions of the variables if they are not equal to verify if they might be equal in another conversion. Here are some examples of strings that will be considered equal, because PHP will consider them different E notations of 0.

 "0E434" == "0E0" "00E0" == "000000E3423423" "0000000E" == "0E17"

When you look at these strings you will see that all of them start with a number of 0 followed by exactly one E and then some arbitrary number. If you make these strings 32 characters wide they could be potential MD5 hashes. If my math is correct this means with a probability of about one in 306 million MD5 hashes will be of the form any number of zeroes followed by a single E and a decimal number afterwards. This means an attacker can still bypass the SQL injection protection with a probability of 1 in about 306 million. Of course this is a very small likelihood, especially against an arbitrary target on the internet, but people also win the lottery.

The correct fix would be to replace the == equality operator with the identity operator ===.