RedTeam

When it comes to IT security, one of the things you tell every IT worker, be it the system administrator or the web application developer, is that they should thoroughly read the documentation for whatever they are working with. It doesn’t matter if it’s a new network component or a web application framework you’re programming the new website with, only when you intimately know the intricacies of your tools, you’ll be able to adequately secure them against the big bad world.

Now, the problem with documentation is twofold: First of all, there has to exist some kind of documentation. I’m a Ruby coder, believe me, I know all about missing docs… The next problem is: It has to be accurate. If what is written in the documentation is not true, then you have a problem. Sometimes, documentation is just sloppy in whatever it describes. Sometimes, it is plain wrong.

Inaccurate documentation may be only a nuisance when you try to find out why the config option you set for coloured log output doesn’t do anything, but it can become outright dangerous when it comes to security related documentation. I want to show you what I mean by example. Let’s have a look at… the Typo3 documentation.

Typo3 provides in its core documentation the “Typo3 Coding Guidelines”. To follow them is mandatory for core developers and contributers to the Typo3 core, as the document states. To be fair from the start: The linked documentation is for version 4.1.0 and not directly linked from the website anymore. If you click on the documentation link on their website, you get directed to the current documentation for version 4.3.2. The link is, however, the first one you get with a Google search for “typo3 updatequery”, which may not be such an unusual search request when you want to know how the updatequery() function works. As you may have guessed, the function executes a SQL UPDATE and is part of Typo3’s Database Abstraction Layer (DBAL). The new documentation for 4.3.2 only contains this sentence:

The TYPO3 database should be always accessed through the use of $GLOBALS['TYPO3_DB']. This is the instance of t3lib_db class from t3lib/class.t3lib_db.php.

That said, let me quote the relevant parts from section 1.4 “Database connectivity and DBAL” from the old version 4.1.0:

TYPO3 has been designed to use MySQL from the beginning. With TYPO3 3.6.0 a database wrapper class has been introduced in all of the core and default global extensions. This means that implementation of various Database Abstraction Layers (DBAL) is now possible.
[...]
The wrapper class is called “t3lib_DB” and is instantiated globally as $TYPO3_DB. The class contains three sections of functions
[...]
2. Query building functions: Instead of constructing SELECT, UPDATE, INSERT and DELETE statements directly you can use API functions in the wrapper class for this. This requires a bit more re-design from you but you will get better security in your scripts (prevents SQL injection for UPDATE/INSERT at least) and is step-2 towards database abstraction support in your extensions!

As a developer reading this part of the documentation, I might just think “great, I don’t have to worry about SQL Injection in my UPDATE and INSERT queries if I use the DBAL. How convenient!”. As a pentester, you of course immediately get suspicious. Especially if you searched for “Typo3 prepared statements” before and know that Typo3 doesn’t support prepared statements in its DBAL (yet).

Ok, let’s have a look at the source code. The following sources are from version 4.3.2 and haven’t changed in the relevant parts since 4.1.0. The function UPDATEquery() is defined in t3lib/class.t3lib_db.php:

/**
 * Creates an UPDATE SQL-statement for $table where $where-clause (typ. 'uid=...')
 * from the array with field/value pairs $fields_values.
 * Usage count/core: 6
 *
 * @param       string          See exec_UPDATEquery()
 * @param       string          See exec_UPDATEquery()
 * @param       array           See exec_UPDATEquery()
 * @param       array           See fullQuoteArray()
 * @return      string          Full SQL query for UPDATE (unless  $fields_values
 * does not contain any elements in which case it will be false)
 */
function UPDATEquery($table,$where,$fields_values,$no_quote_fields=FALSE) {
  // Table and fieldnames should be "SQL-injection-safe" when supplied to this
  // function (contrary to values in the arrays which may be insecure).

Wait a minute. Table- and fieldnames should be SQL injection safe? Like in, before I supply them to UPDATEquery()? Why, that’s useful information! Let’s see the body of the function to verify that table and fieldnames are not sanitised:

  if (is_string($where))    {
    if (is_array($fields_values) && count($fields_values))  {

      // quote and escape values
      $nArr = $this->fullQuoteArray($fields_values,$table,$no_quote_fields);

Here, the $fields_values array get escaped. fullQuoteArray() uses Typo3’s fullQuoteStr() internally, which in turn uses mysql_real_escape() to escape a value:

function fullQuoteArray($arr, $table, $noQuote=FALSE)   {
[...]
  foreach($arr as $k => $v)       {
    if ($noQuote===FALSE || !in_array($k,$noQuote))     {
      $arr[$k] = $this->fullQuoteStr($v, $table);
    }
  }
  return $arr;
}

Ok. As we can see, the field names indeed do not get escaped, only the field values. What about $table and $where?

      $fields = array();
      foreach ($nArr as $k => $v) {
        $fields[] = $k.'='.$v;
      }

      // Build query:
      $query = 'UPDATE ' . $table . ' SET ' . implode(',', $fields) .
        (strlen($where) > 0 ? ' WHERE ' . $where : '');

      // Return query:
      if ($this->debugOutput || $this->store_lastBuiltQuery) {
        $this->debug_lastBuiltQuery = $query;
      }
      return $query;
[...]
}

Apparently, the $table and $fields variables are inserted into the query without any further sanitisation. So the comment is right, you have to check for SQL injections in the table- and fieldnames. But, the values in the $where variable are also inserted verbatim! So there’s another unsanitised variable, not even mentioned in the comment.

As you can see, blindly trusting the documentation may lead to dangerous behaviour, like not santising all your inputs in this case. Of course, you can’t check the source code of every security relevant functionality yourself, even if you have access to it. Just have a healthy dose of scepticism if you find a statement like the one in the old Typo3 coding guidelines without any further explanation.

I wrote a similar blog post way back in 2006 titled “Reading the fine Manual”, where we saw that the PHP ereg*() functions are not binary safe. In that case however, the documentation did tell the truth :).