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.

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

— Geekery, Programming

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!

In Defense of Hipmunk’s Usability

— Technology, This Turbulent World

Visiting one of my daily input sites, Hacker News, I came across a blog post that I took offense to with the link: “Hipmunk needs to ramp up on usability.” Hipmunk is not my site, so why would I take offense? Because Hipmunk is the Harvey Dent to my Bruce Wayne. Because I believe in Hipmunk.

Although the title of your blog post was: “Why I don’t use Hipmunk..”, the title to the link provided to Hacker News was: “Hipmunk needs to ramp up on usability.” The title of your post was far more accurate. Hipmunk has incredibly usability in my opinion and the opinion of many others. Your opinion of the usability is your own and upon reading your post, is probably in the minority. Finding flights was a huge pain in the rear before Hipmunk came around. While some search providers tried to make things easier, no one has nailed it like Hipmunk has. I’ve traveled often my whole life. Hipmunk was, to me, a breath of fresh air when it arrived and it has only gotten better. As I was reading through the gripes of the author, I couldn’t help but think: “oh come on, really?”

Let’s see if I can successfully come to the defense of the site I have grown to know and love.

1. Default sorting by Agony: What exactly is Agony, in layman terms? I have no idea. There is some vague explanation on the site about it being some sort of a combined metric, but I find it hard to visualize or connect to anything that matters to me.

Did you even bother to look at the FAQ? You know, the thing that is commonly included for services in case you have a question, that might be frequently asked. I don’t see the vagueness in the description that is the third item down and easily visible on that page (emphasis added):

What is Agony, and why would I want to sort by it?

We know that price isn’t the only factor that goes into purchasing a flight. While other sites sort by price, Hipmunk automatically sorts results by “Agony,” which is primarily a combination of price, flight duration, and number of stopovers.

Not only is the description readily available, the agony sorting methodology is far superior to other sorting methods, in my opinion. Frequent travelers often have a threshold at which layovers and connecting flights are worth a few more bucks to avoid countless hours of sitting in an airport. If price is really your main concern, the cheapest flight is always highlighted and it’s quick and easy to switch the view to sort by price. In my experience, it is nearly sorted by price anyway, so I find it odd that this would really get in your way so often that it would be of concern.

2. Price: The pricing is vague. What does From $639 mean to a price-conscious traveler? Not clear. I can’t tell whether the price is only for the onward journey or also includes the return journey. I also can’t tell if it’s $639, or something higher (because of the From). Do I have to actually go through multiple screens and select the onward and return flights just to find the exact price? That’s too much work..

Hovering your mouse over the price clearly provides the tip that the price is the total including the round trip. Desiring a different return flight on the next screen may change that price so, yes, it’s “From $639” if you happen to select the flights that make that price; this is not a problem of usability, it’s the truth. I have never experienced confusion with this. To answer your question: no, you do not have to go through many screens to find the exact price because it was listed right there at the beginning.

3. Selecting onward/return flights in 2 different steps: Being able to pick onward and return flights separately might sound neat in theory, but it’s confusing for the average traveler. I want simple options. On Hipmunk, how do I know what options United will give me for the return flight if I pick the 6 am onward? What if I pick delta? Do I have to click on each individual flight to see what the return flight options are? This taxes my brain and is frustrating.

I’ve found this to be one of the nice parts about Hipmunk and many of my friends I have shown Hipmunk to have agreed. If this truly taxes your brain and is frustrating, then I would have to question your capacity to purchase flights online at all. Clearly shown at the top are the two different dates you have chosen with a 1 and a 2 next to them. The first day of your trip is highlighted. Upon making your selection, the second day is highlighted and you choose a return flight. Not only is there a clear indication that you are moving from the first leg of your trip to the next, but there’s help to let you know that it may be a two step process. I don’t think you do a very good job of speaking for “the average traveler.”

4. Flight duration: visualizing this is supposed to be Hipmunk’s strength, but it’s not helpful to me. I find the hours explicitly stated on Kayak (“8h 51m”) much more comprehensible than having to estimate the duration by looking at Hipmunk’s excel type UI.

Even if you are incapable of doing a bit of math or using graphs to determine values, the necessary information is provided: when do you leave and when do you get there? Using the graphical layout, Hipmunk also provides, at a glance, important information that Kayak does not. Specifically: how much of that total time Kayak is quoting is spent sitting in an airport waiting for my next flight? I can’t tell at a glance. With Hipmunk showing me the various airport codes I will be staying in and color coding them differently, I can tell rather quickly how much time I’ll be sitting around waiting for my flights and which airports I’m going to be stuck in.

5. Filtering: Hipmunk falls short here too. Their “Non-stops” filter is grouped together with sorting options, making it hardly noticeable. On Kayak, I can specify the number of stops I am comfortable with (my wife only likes non-stops, I’m ok with 1 stops). I can also specify my take-off times for both legs (say early evening for onward flights and late evenings for return flights). It’s really useful to use the departure/arrival time sliders to view flight availability, while also observing how that affects price.

I agree with a single point of this statement: grouping “Non-stops” with sorting options is not a good place for it. However, this does not mean Hipmunk does a bad job of filtering. Because of the strength of the visual layout, filtering becomes less of a necessity. This is especially true when there are enough sorting options to make the layout appear in a way that is easy to absorb. Most concerned with when you leave? Sort by Departure.

That being said: you can still filter based on time rather easily so I don’t know where Kayak has an advantage here. Want to leave after 8:00am? Select “No early morning departures” from the drop down menu. Finding that 8:00am isn’t specific enough? Simply drag the time filtering bar from the left to the appropriate time. You can do the same with the bar on the right.

If there is concern for the number of stops you have to make, Hipmunk shows it visually quite easily and provides that all-important additional information Kayak does not: how long do you have to sit at that stop? If I have two stops but one of them is a 30 minute layover where I may miss my flight due to delay or find myself running from one gate to another, Hipmunk shows this to me. Kayak does not provide me with this information.

6. Modifying a flight search: Kayak has a prominent Change your search link on the left sidebar above the filters. Clicking on it opens up a lightweight dialog box that I can use to change my current flight options and rerun search. Hipmunk has a New Search link at the top, hardly visible(faded colors), which opens a new tab. What if I only wanted to change the return date on the current search? I don’t really want to go to a new tab and type everything again.

Did you even bother to open a new tab? It keeps your previous search in there. There is no “type everything again” needed. Open a new tab, make your switch, and go. Your old search is still quickly available so you can compare the results of what you previously searched to what you’re searching now. Does Kayak do that? I don’t think so.

7. “Live Help: Offline”: unnecessary button on Hipmunk. Why not just remove this when live help is unavailable?

While I’m all for removing visual clutter, I don’t find this distracting and I actually think this is a good thing for new users, even if it is offline, for a couple of reasons:

  • It lets the user know that the site is legitimate enough to offer instantaneous help when it is needed. This provides a sense of comfort and satisfaction. It is good to know there are people there to assist you (albeit not all the time) and that this isn’t just a site out there being a meta search engine. Kayak doesn’t even have a link to contact them.
  • Upon clicking on the link, you are presented with a form that allows you provide feedback or ask questions. Again: something Kayak seems to be lacking completely.

An adequate argument was not presented that would indicate Hipmunk has usability issues from the author of the aforementioned blog post. I would like to reiterate that the actual title of your post is far more accurate than the one you provided for Hacker News. You provided reasons why you (who doesn’t seem to be an average traveler or someone who recognizes usability concerns well) don’t use Hipmunk, not reasons why Hipmunk has usability issues.

Constant improvements with Hipmunk are readily seen while Kayak remains the same and feels archaic. I won’t go into why I think Kayak is horrible but during my usage of the site while writing this post I have discovered I am extremely glad I don’t have to use it. Why do I have to see 7 results for the same airline and price with slight variations in time (that aren’t necessarily sorted by time) with unexplained differences in the “total time” it takes for a flight listed? Why do I need to see pop up windows for practically every action I take? Why would I want to see comparison searches from other sites? Oh gosh… don’t get me started.

I believe in Hipmunk.