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

I've also taken the time to comment on the blog post itself. Reproducing my comment here:

---------------

Thanks for the response! I just have a couple points to make:

---

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

Yes. It was a reaction to the bug being marked public, which meant anyone could view the full details of the issue.

---

"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."

As I pointed out on Hacker News, the CLI interpreter does not HTML encode fatal errors, warnings, or notices. The CGI interpreter DOES encode fatal errors and warnings, but not notices. As such, your points aren't really applicable to the situation.

---

"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."

1. You're using a logical fallacy to make your point: http://en.wikipedia.org/w/index.php?title=No_true_Scotsman&#...

2. Many shared hosts do enable display_errors by default.

3. I'm not arguing that this is a feasible attack to pull off against most applications: it most certainly is not. I'm arguing that it shouldn't be possible to pull it off at all, even in the small percentage of applications that might be affected. There is no good reason PHP should be displaying this information unsanitized.

---

"How is this a proof of concept of an undefined index or notice?"

You seem to have missed the line

    $message = $messages[$_GET['message']];

---

"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."

First you tell me my recommendation is common sense, then you tell me it's bad advice. Make up your mind ;-)

Anyway, as I said on Hacker News, my original report only focused on the "Undefined index" message, which is a fairly benign issue (albeit a sign of sloppy coding). The other examples were discovered later and were only mentioned to demonstrate that PHP notices in general had an issue.

I completely disagree with your assessment of my recommendation though. Telling people to disable display_errors prevents the issue at hand, the cross-site scripting, from ever occurring. In contrast, telling people to go find and fix any PHP notices in their code is a reactive solution: it only works until the next piece of sloppy code gets checked in, at which point the site is vulnerable again. And of course, as you've said repeatedly, display_errors should not be enabled in production in any case.

I want to comment about this conclusion, lest anyone reading it actually follow the author's advice.

You should absolutely be hiding notices/errors from appearing. This is orthogonal to whether or not the errors should be allowed to occur. When during the course of assessing a web application I (or anyone who does this for a living) gets an error message generated by the framework or server (like ASP.NET's yellow screen of woe), that's an issue (albeit usually a low severity issue). If that error can display user input in a non-sanitized fashion, it's a more severe error.

The recommendation isn't to "make sure the application doesn't generate any errors" (although, hey, that's a great thing to shoot for), it's to capture those errors and not display them to the user (usually by displaying a generic error message that doesn't leak useful information to an attacker).

This includes things like disabling debug functionality.

Oh, and http://drupal.org/node/1168756