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

— Programming

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.

Related Posts