-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP Catchable fatal errors causes partial rendered template to leak to client #1962
Comments
the solution is to register an error handler turning catchable fatal errors into exceptions, as Twig cleans its output buffer when an exception happens (but try/catch does not help for PHP errors, and there is no equivalent feature for them) |
Hi, are you talking about a fix for Twig when you mention "a solution", or was that a reply to how to fix the testcase? I think that the caller of $twig->render() shouldn't need to concern itself with how twig internally uses output buffering or eval(). Apologies if I misunderstood your comment. |
I'm talking about a fix for your project. I don't think Twig should force registering its own error handler to turn errors into exceptions (as it would break compatibility with all frameworks out there which are registering their own error handlers with fancy features). |
Well, from my point of view as a library consumer, I had no idea that twig's render() method, which isn't supposed to produce any output on "stdout" but just return a string, uses eval() and output buffering internally and is suspectible to trigger PHP catchable fatal errors because of syntax issues in .twig files. For me, it's just another method that reads .twig files and returns a string. It's really quite surprising (and somewhat of a security issue/info leak) that in error cases, part of the string that is expected to be returned and assigned to a PHP variable (for emailing with SwiftMailer to an administrator, for example) instead leaks to the HTTP API client that happened to trigger the PHP code. I do appreciate that PHP fatal error handling is messy, but seeing as it is Twig that chose to go with an eval() based approach to template parsing, I'm leaning towards putting the responsibility of cleaning up also with Twig. ;) At the very least, this should probably be documented explicitly, perhaps under a "Security considerations" header. It would also be nice if there were some default error handler adapters or something that would make it easy to avoid leaking template output to clients in common frameworks (such as silex, which I happen to use). Thanks again for looking at and considering this issue. |
Well, anything that executes PHP code is subject to triggering fatal errors. and Twig is written in PHP And the eval is not about template parsing. It is about loading the code of the compiled class when it is not already in the cache (or when you disabled the cache entirely, as Twig cannot write the code to the disk). It is unrelated to the fatal error. An error handler turning fatal errors into exceptions would look like this: // throw only for critical errors here. You can change this to E_ALL | E_STRICT if you want more debugging capabilities in dev.
// another option is removing this variable entirely and relying on error_reporting() to
// exclude lower levels that you don't want to report, if you want to throw exceptions for
// all reported levels
$throwingLevels = E_ERROR | E_CORE_ERROR | E_COMPILE_ERROR | E_PARSE;
set_error_handler(function($type, $message, $file, $line) use ($throwingLevels) {
// checking the current reporting level here is necessary to avoid breaking the @ operator
if ($type & error_reporting() & $throwingLevels) {
throw new \ErrorException($message, 0, $type, $file, $line);
}
return false; // Error not handled by this error handler
}); If you want more features for your error handler, I suggest you to use the symfony/debug component instead of reimplementing more advanced features in the error handler (once going for advanced features, tricky cases become really tricky). Btw, when using Silex, this looks weird, because it relies on the symfony component already, but does not register the error handling by default (while the Symfony component is meant to be usable in prod now, although with a different config than in dev). I suggest you to open an issue on Silex about that. |
My main concern here is avoiding leaking the output buffer to the HTTP client on errors as a safe default. What about if Template.php's render() method on https://github.com/twigphp/Twig/blob/2.x/lib/Twig/Template.php#L370 is changed from
to
? This seems to prevent automatic flushing to stdout on catchable fatal errors:
while the empty parameter list variant of ob_start() leaks:
I looked at http://php.net/manual/en/function.ob-start.php which says that the output buffer is returned to the client only if the first argument to ob_start(), as a Callable, returns FALSE. ("If output_callback returns FALSE original input is sent to the browser.") |
After further investigation, the problem does not exist anymore in Twig as we are now catching PHP Catchable fatal error. So, leaking in that case does not occur anymore. I suppose the PR is still valid for PHP fatal errors. |
Thank you very much for this. I run into similar issue with my Twig function using ob_start inside, this does the fix. |
Hi, I'm using twig to format HTML emails to be sent from an API endpoint implemented in Silex. I discovered that if a "PHP Catchable fatal error" is triggered during template rendering, the partially rendered template is output to the API client (!). This almost feels like a security issue, as the rendered template is not intended for the API client but for someone else entirely.
This only seems to happen with some errors, for example when specifying an {{ object }} which cannot be converted to a string. Other errors seem to just throw exceptions and php errors but do not leak the template.
Using twig/twig v1.23.3 from composer, here is a sample case:
If you run through the possible templates, you'll see that the fifth template leaks the partial template to the client:
Yields:
And with error reporting for the leaking template:
yields:
(This is on PHP 5.6.17 via homebrew on OSX 10.11, by the way)
The text was updated successfully, but these errors were encountered: