Programming

And if I had a gun, with two bullets, and I was in a room with Hitler, Bin Laden and Internet Explorer, I would shoot Internet Explorer twice.

When creating HTML elements with jQuery, there is an elegant syntax that was introduced quite a few versions ago.

$("<div/>", {
    id: 'someDiv',
    class: 'someClass',
    text: 'Some text in the div.'
}).appendTo("#someOtherDiv");
 
// Another example
$("<label/>", {
    for: 'someInput',
    text: 'Label for my Input'
}).appendTo('#selectDialogue');

Simple, elegant and easy to use. This code works fine in Google Chrome and Firefox.

Unfortunately, every developers favorite browser, Internet Explorer, chokes with the following error message:

…expected identifier, string or number…

It took me a while to figure it out but, through trial and error, I discovered that the issue was with the way Internet Explorer handle reserved words. In this case, both examples are using two that IE will simply choke on. Here are the offending lines:

class: 'someClass'
for: 'someInput'

Internet Explorer will not allow you to set these attributes when creating the element. After spending too much time tracking down and figuring out the problem, since IE is so good at explaining itself when it comes to errors, I came to a solution. My first reaction was to simply apply the attribute to the element after it had been generated. After speaking with a colleague about the issue, he suggested putting the attribute name in quotes to see if it would do the trick. Turns out: this worked just fine.

After some digging around, I came across some documentation available from the jQuery folks that describes this problem. Unfortunately, this documentation was not prevalent in any of the searches I was attempting to perform. Here’s what they had to say (emphasis added):

As of jQuery 1.4, the second argument to jQuery() can accept a map consisting of a superset of the properties that can be passed to the .attr() method. Furthermore, any event type can be passed in, and the following jQuery methods can be called: val, css, html, text, data, width, height, or offset. The name “class” must be quoted in the map since it is a JavaScript reserved word, and “className” cannot be used since it is not the correct attribute name.

Using the quoted attribute solution, our code changes to:

$("<div/>", {
    id: 'someDiv',
    'class': 'someClass',
    text: 'Some text in the div.'
}).appendTo("#someOtherDiv");
 
// Another example
$("<label/>", {
    'for': 'someInput',
    text: 'Label for my Input'
}).appendTo('#selectDialogue');

This allows Internet Explorer to go along it’s merry way doing it’s browser impression as best as it possibly can.

I hope this helps post will help others who come across this problem. If you liked this post, please consider subscribing to my feed.

Programming

I consider myself lucky to work at a place where we have someone working QA to ensure the code we’re pushing out isn’t going to blow up. When a development ticket has passed through QA, we get an email to let us know. Usually saving the task of merging code until the end of the day allows me to spend the last bit of time doing something that doesn’t require a huge amount of brain power. Normally this consists of running a merge command and fixing a conflict that pops up every once in a while.

Occasionally things just don’t work out that way.

$ svn merge -r 24936:24937 http://blade00/svnroot/crm/trunk
Conflict discovered in 'soapserver/soap_server_production.wsdl'.
Select: (p) postpone, (df) diff-full, (e) edit,
        (mc) mine-conflict, (tc) theirs-conflict,
        (s) show all options: p
--- Merging r24937 into 'soapserver':
C    soapserver/soap_server_production.wsdl
Conflict discovered in 'soapserver/soap_server_uat.wsdl'.
Select: (p) postpone, (df) diff-full, (e) edit,
        (mc) mine-conflict, (tc) theirs-conflict,
        (s) show all options: p
C    soapserver/soap_server_uat.wsdl
Conflict discovered in 'soapserver/soap_server_staging.wsdl'.
Select: (p) postpone, (df) diff-full, (e) edit,
        (mc) mine-conflict, (tc) theirs-conflict,
        (s) show all options: p
C    soapserver/soap_server_staging.wsdl
Conflict discovered in 'soapserver/soap_server.php'.
Select: (p) postpone, (df) diff-full, (e) edit,
        (mc) mine-conflict, (tc) theirs-conflict,
        (s) show all options: p
--- Merging r24937 into 'soapserver/soap_server.php':
C    soapserver/soap_server.php
Conflict discovered in 'soapserver/soap_server_dev.wsdl'.
Select: (p) postpone, (df) diff-full, (e) edit,
        (mc) mine-conflict, (tc) theirs-conflict,
        (s) show all options: p
--- Merging r24937 into 'soapserver':
C    soapserver/soap_server_dev.wsdl
Summary of conflicts:
  Text conflicts: 5
FFFFFFFFFFUUUUUUUUUUUUUUUUUUU

Ahh… the joys of merging code.

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.