PHP Catchable fatal errors causes partial rendered template to leak to client

[fluff]

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:

<?php
require('vendor/autoload.php');

class SomeClass { public $value1; public $value2; }
$templates = array(
  'SECRET MAIL CONTENTS no leaks: {{0/0}}',
  'SECRET MAIL CONTENTS no leaks: {{invalid}}',
  'SECRET MAIL CONTENTS no leaks: {{invalid()}}',
  'SECRET MAIL CONTENTS no leaks: {{invalid',
  'SECRET MAIL CONTENTS no leaks: {{obj.invalid}}',
  'SECRET MAIL CONTENTS template DOES leak: {{obj}}',
  'SECRET MAIL CONTENTS no leaks: {{constant(PHP_VERSION)}}',
);
$idx = $_SERVER['argv'][1];
echo "\n\nTrying template $idx\n";
$twig = new Twig_Environment(new Twig_Loader_Array(array('index' => $templates[$idx])));
$obj = new SomeClass();
$htmlForAdmin = $twig->render('index', array('obj' => $obj));
echo "\nDone with template $idx, thanks.\n\n";

If you run through the possible templates, you'll see that the fifth template leaks the partial template to the client:

for i in 1 2 3 4 5 6 7; do php -d error_reporting=0 testbug.php $i; done

Yields:

Trying template 1
Done with template 1, thanks.
Trying template 2
Trying template 3
Trying template 4
Done with template 4, thanks.
Trying template 5
SECRET MAIL CONTENTS template DOES leak: 
Trying template 6
Done with template 6, thanks.
Trying template 7

And with error reporting for the leaking template:

php -d error_reporting=8191 testbug.php 5

yields:

Trying template 5
PHP Catchable fatal error:  Object of class SomeClass could not be converted to string in /private/tmp/twig_bug/vendor/twig/twig/lib/Twig/Environment.php(403) : eval()'d code on line 20
PHP Stack trace:
PHP   1. {main}() /private/tmp/twig_bug/testbug.php:0
PHP   2. Twig_Environment->render() /private/tmp/twig_bug/testbug.php:18
PHP   3. Twig_Template->render() /private/tmp/twig_bug/vendor/twig/twig/lib/Twig/Environment.php:347
PHP   4. Twig_Template->display() /private/tmp/twig_bug/vendor/twig/twig/lib/Twig/Template.php:366
PHP   5. Twig_Template->displayWithErrorHandling() /private/tmp/twig_bug/vendor/twig/twig/lib/Twig/Template.php:355
PHP   6. __TwigTemplate_6927891b70918e9a5f4eaf6d00ccd7cf0bfc98b7f781b770f99869a64797c516->doDisplay() /private/tmp/twig_bug/vendor/twig/twig/lib/Twig/Template.php:381
SECRET MAIL CONTENTS template DOES leak: 

(This is on PHP 5.6.17 via homebrew on OSX 10.11, by the way)

[stof]

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)

[fluff]

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.

[stof]

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).
Twig already takes care of cleaning after itself when an exception happens. But PHP errors are just a messy system, as anything trying to deal with them impacts the whole project (as error handlers are global)

[fluff]

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.

[stof]

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.

[fluff]

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

ob_start()

to

ob_start(function($in){
  return true; // Prevent leaking output buffer to client on fatal errors
})

?

This seems to prevent automatic flushing to stdout on catchable fatal errors:

php -r 'ob_start(function($in){return true;}); echo "the output buffer is not leaking\n"; echo new stdClass();'

PHP Catchable fatal error:  Object of class stdClass could not be converted to string in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0

while the empty parameter list variant of ob_start() leaks:

php -r 'ob_start(); echo "the output buffer is leaking\n"; echo new stdClass();'

PHP Catchable fatal error:  Object of class stdClass could not be converted to string in Command line code on line 1
PHP Stack trace:
PHP   1. {main}() Command line code:0
the output buffer is leaking

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

[fabpot]

fixed by #2995 (thanks @fluff for the trick)

[fabpot]

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.

[yellow1912]

Thank you very much for this. I run into similar issue with my Twig function using ob_start inside, this does the fix.