Tag Archive for programming

August 18th, 2011
One comment

No, PHP Errors and Notices Do Not Need to be Encoded

Programmers giving other programmers questionable advice is never a good thing. Because of this, I was sort of surprised when I finished reading a post from Neal Poole that hit the front page of Hacker News. Appearing on Hacker News as “Cross-Site Scripting? In PHP Notices? It’s more likely than you think,” his blog post is entitled: “Cross-Site Scripting via Error Reporting Notices in PHP.” Mr. Poole, who proclaims an interest in web-application security, goes on to explain the issue and comes to an awful conclusion while also providing some equivocal information and advice/examples.

The post begins with the following passage:

A PHP application that displays error reporting notices and contains specific code patterns may be vulnerable to a cross-site scripting attack. I’ve confirmed this issue on PHP 5.2.17 and a snapshot of PHP 5.4 (I assume it affects other versions of PHP as well). This issue was filed as Sec Bug #55139 back in July, but it was recently closed as “bogus” by a member of the PHP team, making the report public.

Beginning with the first sentence: a PHP application that has display_errors enabled should never be in production. The PHP manual explains this and, among PHP developers, it is common practice to ensure display_errors is never enabled on a production system. Providing unintended information to users is one consequence and error output will commonly clutter up a page or completely break styling.

Continuing towards the sentence about filing the bug, it appears Mr. Poole filed the bug in July. Providing examples of issues in the bug report, he went on to show how he believed XSS was a problem with the handling of errors and that sanitizing the output should be performed. After going on to say that he was going to publicly discuss his supposed issue on the 26th of this month, a response was delivered:

Sorry, but your problem does not imply a bug in PHP itself. For a list of more appropriate places to ask for help using PHP, please visit http://www.php.net/support.php as this bug system is not the appropriate forum for asking support questions. Due to the volume of reports we can not explain in detail here why your report is not a bug. The support channels will be able to provide an explanation for you.

Seemingly reacting to this response, Poole’s blog post appeared the next day instead of the aforementioned 26th.

Continuing on, Poole writes:

When display_errors is enabled and a PHP notice is generated, none of the text of the notice is HTML-encoded.

Providing two good reasons for this is not difficult and I am sure there are others:

  1. I don’t want to see anything to do with HTML if I’m not doing web-based programming with PHP (such as CLI).
  2. I want my error messages to provide me with precise information. I do not want an error message to arbitrarily encode problematic code. If the user provided <s>test</s> then I want to see <s>test</s>. I do not want to see &lt;s&gt;test&lt;/s&gt; since that is not what was submitted and, possibly, not what the problem is.

Succinct error messaging allows for proper debugging and reproduction of issues with code. By being unsure as to what the actual input was due to mangled messages, this becomes a more difficult task. If you want your error message output to be styled or look different, PHP provides you with tools to do so like set_error_handler.

Moving forward, we come to somewhat bizarre paragraph:

But display_errors needs to be enabled!
Yes, display_errors needs to be enabled. Yes, that means your server is potentially leaking sensitive information (including absolute paths). Yes, that’s not something you should be doing on a production server. But under normal circumstances, that’s the extent of any security issues. A sysadmin or a developer who makes the decision to enable display_errors has no expectation that doing so also (potentially) opens up their site to cross-site scripting.

No, Mr. Poole. A decent sysadmin has better things to worry about than PHP configurations. Regardless, sysadmins should be perfectly aware of the stupidity of letting display_errors remain enabled on a production environment; the same should be said for any developer worth their salt. There simply isn’t a valid excuse and this is a perfect example of why configurations differ between development and production environments.

Explaining examples of where this is an issue sounds like a pretty good idea. Let’s take a look at the example that Poole provided:

As a proof of concept, I identified a location in WordPress (/wp-admin/upload.php) containing a vulnerable code pattern. It looked like:

if ( isset($_GET['message']) && (int) $_GET['message'] ) {
   $message = $messages[$_GET['message']];
   $_SERVER['REQUEST_URI'] = remove_query_arg(array('message'), $_SERVER['REQUEST_URI']);
}

In that case, an attacker could supply $_GET['message'] as 1<script>alert(1)</script>, triggering an alert box.

How is this a proof of concept of an undefined index or notice? By checking isset() before performing actions upon $_GET['message'], like casting it to an int, we alleviate the possibility of a notice occurring. I’m not sure where this can throw a notice for an undefined index or a notice at all. Perhaps a review in operator precedence would be helpful to see that this is the case? I thought maybe you were talking about the remove_query_arg() function call that may be producing a notice but then you mention a revision that didn’t even touch that function so I can only assume you took offense to the if()s.

Pointing to a revision in the WordPress source, Poole says:

However, the code has been updated in SVN and is no longer vulnerable. I want to extend my thanks to the WordPress developers for addressing this issue.

Are you sure? The revision note simply says that the change was made to “Clean up message handling logic in upload.php.” It doesn’t appear to be any sort of security fix. Indeed, if we examine the op codes performed for both the isset() and empty() functions we can find that it’s the same: ZEND_ISSET_ISEMPTY_DIM_OBJ. Both of these functions can be used to check for the existence something to ensure a notice isn’t thrown when trying to access that which doesn’t exist; bringing me to my final issue with the blog post.

Neal Poole, who works for a security firm, comes to the following conclusion as a fix for their reported issues:

Conclusion
Don’t enable display_errors in production: it can now cause cross-site scripting as well as information disclosure.

Giving us a common sense recommendation is hardly a fix for the issues you’ve described above. For every one of your examples, there is a solution that is offered in PHP that is the correct fix. Recommending programmers turn off their error display without actually addressing the bad code is not good advice at all and something I would hardly expect from someone who works in web application security.

Here are the conclusions you should have come to:

  • Undefined index notices: use isset() or empty() to ensure the property or variable is usable.
  • Constant already defined notices: use defined(). Really though, why you would be assigning dynamic constants based on user input is a whole ‘nother bag of smelly code.
  • Undefined function (which throws a notice and a fatal): use function_exists() to make sure you’ve actually got a function available if the code is keen on using dynamic function names.

Regarding your other example:

<?php
// undefined variable
error_reporting(E_ALL);
echo $$_GET['var'];

Deprecating register_globals in 5.3 was a good thing and removing it from 5.4 is even better. Reintroducing the issue through the misguided use of a variable variable is an awful idea.

Attempting to address what you may think is an issue is admirable, but I would hope users could take away the more important issues that I’ve brought up in my response to what you presented:

  1. Having display_errors enabled on a production system is a terrible idea, discouraged in the manual of PHP, and commonly known to be an amateur mistake. Adjusting PHP to make this an acceptable practice is not a solution and should not be considered a bug.
  2. There are programmatic solutions available that would prevent any of your code conditions from occurring in the first place. These are the correct solutions to the problems you’ve provided. Stopping the notices from occurring at all is a much better solution than simply making them not appear.

Finding developers like this in security positions or programming positions seems like it is becoming a troubling trend.

Please, fix the problems instead of hiding them.

Category: Programming | Tags: , ,
August 15th, 2011
No comments

Zend Framework and the Zend_Db_Select::where() and ::orWhere() Problem

To say that I dislike Zend Framework, would be an understatement. While it is powerful and capable of quite a bit, I find it to be a bit overkill for many (if not most) tasks and bloated in so many ways. But, I’m not a huge fan of frameworks to begin with. As programmers, we find ourselves having to work with things we dislike all to often. Providing decent documentation and examples allows for this process to be a bit more enjoyable. Zend Framework documentation is, mostly helpful but it definitely has it’s issues.

When using the Zend_Db_Select (I’d rather be using PDO) object, I came across the necessity to execute a MySQL query with some OR conditions. This seems like a simple enough task especially considering the availability of an orWhere() function to use instead of a where() function. Reading the description of the functionality from the manual reveals the following:

If you need to combine terms together using OR, use the orWhere() method. This method is used in the same way as the where() method, except that the term specified is preceded by OR, instead of AND.

Changing AND to OR is the only thing this function provides. Searching around for gripes about the functions yields various complaints regarding the relative uselessness of the orWhere() for many cases.

Let’s say you want to find instances of a record that is active (active = 1) and where the date is set to null (0000-00-00) or in the future (> 2011-08-09). When using the where() and orWhere() functions, it feels somewhat natural to try something like this after reading the documentation:

$select = $db->select()
             ->from("some_table")
             ->where("active = ?", 1)
             ->orWhere("my_date > ?", "2011-08-09");
             ->orWhere("my_date = ?", "0000-00-00")

Unfortunately, this produces a query that looks like this:

SELECT * FROM some_table 
WHERE (active = 1) 
OR (my_date > '2011-08-09') 
OR (my_date = '0000-00-00')

This would return any records where active is 1 or my_date is in the future or my date is null. Not quite what we’re looking for. Progressing from this, one might consider switching the first orWhere() to a where() to add in the appropriate AND:

$select = $db->select()
             ->from("some_table")
             ->where("active = ?", 1)
             ->where("my_date > ?", "2011-08-09");
             ->orWhere("my_date = ?", "0000-00-00")

Trying that doesn’t get us there either:

SELECT * FROM some_table 
WHERE (active = 1) 
AND (my_date > '2011-08-09') 
OR (my_date = '0000-00-00')

Perhaps the documentation from Zend can shed some light on this problem. (A side note to the Zend documentation folks, who still struggle to make documentation that doesn’t suck: it isn’t that hard to create documentation where it’s possible to highlight only code without grabbing the line numbers. Please consider looking into this. Thanks.)

The manual lets us know that the Zend_Db_Select is completely useless for even a moderately useful OR query and tells us to do something like this:

$minimumPrice = 100;
$maximumPrice = 500;
$select = $db->select()
             ->from('products',
             ->where("price < $minimumPrice OR price > $maximumPrice")

This wouldn’t be so bad except for the fact that it doesn’t use query parameterizing and doesn’t take into account the fact there’s a pretty good chance we won’t be dealing with straight integers all the time. I find it to be a weak and lacking example. Trying to rouse someone from the Zend Framework team to understand and possibly address the insufficiencies provides a response just as useless:

The where() and orWhere() methods of Zend_Db_Select support a majority of common queries. This class is not able to implement a method interface to the entire SQL language.

Complex queries that are not supported by Zend_Db_Select must be written in SQL and submitted as a string to the Zend_Db_Adapter_*::query() method.

Translating this, from my perspective, sounds a bit like: “We figured those helper methods would be useful for most things but, since they aren’t, we’re going to blame the complexities of the SQL language and tell you to use the base query method to generate your own queries.”

Adding insult to injury, the codebase I currently work on further abstracts database functionality by providing a search() method to our model’s data adapters to grab things from the database using arbitrary bits of the model or, even other models. Querying simple things such as columns already available could be done by passing an array to the search() method containing a key name for the column and, either the value you wanted it to be equal to, or a different operator with a value associated with it. This would intelligently determine the operator and value to check against the column by providing a simple array like:

array (
    'active' => 1, 
    'my_date' => array (
            'operator' => '>', 
            'value' => '2011-08-09'
        )
    )
);
 
/**
   * Results in:
   *
   * $select->where('active = ?', 1);
   * $select->where('my_date > ?', '2011-08-09');
   */

In the example above, we’d find records where active was equal to 1 and startDate was not equal to ’0000-00-00′. While this worked fine for simple searches, it failed to provide the accessibility to queries where an OR statement was needed.

Supplying a fix that follows a similar style and also takes into account the necessity of correct quotations around values (that weren’t INTs) didn’t prove too hard. Modified search parameter arrays could be provided like so:

$date = date('Y-m-d');
$searchArray = array (
    'active' => 1,    
    'my_date'   => array (
        'operator'  => 'OR',
        'value'     => array(
            array (
                'operator' => '>',
                'value'    => $date
            ),
            array (
                'operator' => '=',
                'value'    => '0000-00-00'
            )
        ),
    )
);

Within our search method, we would check to see if the operator was set to OR. If so, the code would go through a simple construction process to add the appropriate AND query with our parenthesis in the right place and it looked something like this:

if ($operator == 'OR') {
    if (is_array($val)) {
        $queryParts = array();
        foreach ($val as $orArray) {
            $orValue = $orArray['value'];
            $orOperator = $orArray['operator'];
            $queryParts[] = sprintf('%s %s %s', $column, $orOperator, $this->db->quote($orValue));
        }
        $select->where(implode(' OR ', $queryParts));
    }
}

Finally we were able to have our code generate the SQL we were looking for:

SELECT * FROM some_table 
WHERE (active = 1) 
AND (my_date > '2011-08-09' OR my_date = '0000-00-00');

While this doesn’t fix the broken and rather useless Zend_Db_Select::orWhere() issue, it is a useful workaround.

Hopefully others can read and investigate this methodology to find the path towards database adapter enlightenment in the fogginess that is Zend_Db_Select. Personally, I find I always have to keep my flashlight handy when trudging through the long, twisted and dark tunnels that are Zend Framework. Good luck, traveler!

Category: Geekery, Programming | Tags: , , ,
August 2nd, 2011
No comments

PHP Developers Finally Deprecating ext/mysql in Favor of mysqli or PDO

Perhaps it is because of the recent slew of high profile SQL injection attacks, or perhaps because it was just finally time to bite the bullet and make the switch. PHP internal emails have revealed that there is finally an official movement to deprecate the ext/mysql and mysql_* family of functions from PHP.

For me, this couldn’t have come any sooner. Although the community has been advocating a change for a long time, much of the documentation and tutorials that exist on the Internet still point to using mysql_* functions instead of their more current alternatives. Many times throughout the day, nearly every day, new PHP developers ask questions in the ##php IRC chat on irc.freenode.net and are delivered a swift !+mysql is old indicating that:

mysql_* functions are no longer recommended for new development. For new projects use mysqli (php.net/mysqli) or PDO (php.net/PDO). The MySQL developers developed guidelines and a script for migrating your code http://forge.mysql.com/wiki/Converting_to_MySQLi. Most people here will recommend PDO: http://wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers

You can find the full email if you want to see the text in it’s entirety but the main points for suggesting things move along are:

  • Softly deprecate ext/mysql with education (docs) starting today
  • Not adding E_DEPRECATED errors in 5.4, but revisit for 5.5/6.0
  • Add pdo_mysql examples within the ext/mysql docs that mimic the current examples, but occasionally introduce features like prepared statements
  • Focus energy on cleaning up the pdo_mysql and mysqli documentation
  • Create a general “The MySQL situation” document that explains the situation

Which should you choose? Should you use PDO or mysqli?

The personal preference of this author is definitely PDO. The possibility of interoperability with other databases is nice, but ability to use exceptions to handle issues with the database is really the selling point for me. Exception handling and other features means that PDO is recommended more often than mysqli_* functions for new development. This is reflected in responses to the mailing list.

To educate yourself on the issue, you can check out additional information about the differences between the options in the PHP manual. For a quick overview, the table near the bottom of the page in the manual is a nice, quick reference:

  PHP's mysqli Extension PDO (Using PDO MySQL Driver and MySQL Native Driver) PHP's MySQL Extension
PHP version introduced 5.0 5.0 Prior to 3.0
Included with PHP 5.x yes yes Yes
MySQL development status Active development Active development as of PHP 5.3 Maintenance only
Recommended by MySQL for new projects Yes – preferred option Yes No
API supports Charsets Yes Yes No
API supports server-side Prepared Statements Yes Yes No
API supports client-side Prepared Statements No Yes No
API supports Stored Procedures Yes Yes No
API supports Multiple Statements Yes Most No
Supports all MySQL 4.1+ functionality Yes Most No

However, my opinion and the opinion of other PHP developers on the matter, may not make a huge difference when it comes to future development. Johannes Schlüter, an engineer on the MySQL/Oracle team, responded to the PHP mailing list with some interesting information about which direction their team is headed:

I’m not sure the current PDO is “the” alternative. We (= MySQL/ORACLE) focus mostly on mysqli, that’s the extension providing access to all current and future features of MySQL. True, many features could be added to PDO but there are two design decision in PDO which make this bad:

  • The parser used for identifying statement place holders is very basic, as it is implemented in PDO core, not the drivers, which leads to FRs like #54929 or the famous LIKE issue.
  • driver-specific functions are implemented by using __call() which means there is no good introspection mechanism to check whether a feature is available or not in the current setup.

Besides these two items there are every now and then reports on PDO_mysql which in fact are caused by limitations in the PDO design which can’t be bypassed by the driver implementation.

A good abstraction layer would certainly be good for the language but for now we (=MySQL/ORACLE) consider mysqli the preference.

It sounds like we will have to wait and see which will be the accepted alternative for future development (if any is ever considered accepted). A huge tipping point would be adding exception handling in the mysqli_* family of functions but that doesn’t appear to be an important development direction at the moment. For now, I will continue to happily use PDO and await the day for users to start wondering why most of the MySQL/PHP tutorials on the web are throwing E_DEPRECATED warnings.

For more information about converting your code to use the new libraries or to learn about them, consider the following links:

It’s about time.

Category: Geekery, Programming | Tags: , ,
« Older Entries   Recent Entries »