tags:

views:

113

answers:

7

Edit: Great points all around, dedicated templating language is obviously the way to go. Thanks!

I wrote up this quick class to do templating via PHP -- I was wondering if this is easily exploitable if I were ever to open up templating to users (not the immediate plan, but thinking down the road).

class Template {

private $allowed_methods = array(
    'if', 
    'switch', 
    'foreach',
    'for', 
    'while'
);

private function secure_code($template_code) {
    $php_section_pattern = '/\<\?(.*?)\?\>/';
    $php_method_pattern = '/([a-zA-Z0-9_]+)[\s]*\(/';
    preg_match_all($php_section_pattern, $template_code, $matches);
    foreach (array_unique($matches[1]) as $index => $code_chunk) {
        preg_match_all($php_method_pattern, $code_chunk, $sub_matches);
        $code_allowed = true;
        foreach ($sub_matches[1] as $method_name) {
            if (!in_array($method_name, $this->allowed_methods)) {
                $code_allowed = false;
                break;
            }
        }
        if (!$code_allowed) {
            $template_code = str_replace($matches[0][$index], '', $template_code);
        }
    }
    return $template_code;      
}

public function render($template_code, $params) {
    extract($params);
    ob_start();
    eval('?>'.$this->secure_code($template_code).'<?php ');
    $result = ob_get_contents();
    ob_end_clean();
    return $result;     
}

}

Example usage:

$template_code = '<?= $title ?><? foreach ($photos as $photo): ?><img src="<?= $photo ?>"><? endforeach ?>';
$params = array('title' => 'My Title', 'photos' => array('img1.jpg', 'img2.jpg'));
$template = new Template;
echo $template->render($template_code, $params);

The idea here is that I'd store the templates (PHP code) in the database, and then run it through the class which uses regular expressions to only allow permitted methods (if, for, etc.). Anyone see an obvious way to exploit this and run arbitrary PHP? If so, I'll probably go the more standard route of a templating language such as Smarty...

+4  A: 

Sure..

$template_code = '<?= `rm -rf *`; ?>';

Edit:

Can't think of anything else right away. But you should know your scope is compromised IF render is ever called more than once on the same Template instance.

For example, if you render('<?php $this->allowed_methods[] = "eval"; ?>') .. then that instance of Template will have eval as an acceptable function for the next render .. ;)

Matt
Clever. And even if you fix this case, there are probably others... probably safer just to *not* let them write PHP.
Mark
Ah, yes. I'll filter out backtick operators... can you think of anything else?
Kunal
@Kunal - see my edit
Matt
Wow, great point. Thanks Matt.
Kunal
+1  A: 

I am wrong in thinking something like this would work?

<?php
/* ?> trick your parser by using a comment */
// do whatever unfiltered

If you really want to do something like this, use the tokenizer to parse the source. I don't recommend it though. In fact, I discourage it!

AlReece45
+1 you're right. `render('<?php /* ?> */ echo("lol"); ?>')` works. Take note, Kunal, there are many many ways to compromise this :)
Matt
Haha, noted. Real templating language it is.
Kunal
+4  A: 

This isn't a good idea. Even if you fix the immediate security holes, there will undoubtedly be others that you missed. I'd say if you really want to give users this ability, use an actual templating language such as Smarty, or write your own. PHP is great as a templating language for internal use, but not as an open one that all your users can use. There are just so many ways this can get exploited that even if you could catch them all, you'd do more work than writing a real templating engine.

musicfreak
+2  A: 

you can use { and } with variables and run any function.

$template_code = '<?php $f = "phpinfo"; ${"f"}(); ?>';

Also, because you are just running the code, it would have access to all variables that the function render can access. including calling global to modify variables from pretty much anywhere or super globals such as $_SESSION (one such option might be session variables holding login information, and using javascript to post through ajax to another site).

$a = "hello";
$template_code = '<?php global $a; $a = "test"; ?>';
$params = array('title' => 'My Title', 'photos' => array('img1.jpg', 'img2.jpg'));
$template = new Template;
echo $template->render($template_code, $params);
echo $a;
Jonathan Kuhn
Forgot about `${"f"}();` notation - good point.
Matt
A: 

I understand this may or may not directly answer your question, but I hope you'll consider it more carefully after reading this. I'm not telling you what to do, just advising :)

Even if this is for internal use, PHP is such a flexible scripting language (well, not as flexible as JavaScript I'd bet but you get the idea) that you will invariably never be able to secure your template engine 100%. Also, there can be innocuous-looking code that accidentally exploits holes in your engine. Even the engineers at Automattic have decided simply not to allow custom themes at WordPress.com at all since WordPress themes are pretty much just PHP as well. Not to mention, parsing mere HTML with regex is nauseating enough to think about, let alone a server-side language like PHP.

Long story short, you can try, but I'd highly recommend against it no matter the use case.

BoltClock
A: 

another one, abusing the allowed functions by making a variable with the same name as an allowed function and the value of any function name.

$template_code = '<?php $if="phpinfo"; $if(); ?>';
Jonathan Kuhn
You should just edit your existing answer ..
Matt
+1  A: 

If you have users contributing content, and these users aren't totally trustworthy, I'd recommend whitelisting instead of blacklisting markup.

For example, consider the Markdown formatting allowed by Stack Overflow. It supports a very short list of formatting options, and everything else is considered to be literal text. Most users are happier with a simple interface instead of an interface that says "Write any code you want! But be careful not to break the app!"

My rule of thumb is: allow users to enter data and content; never allow users to enter code.

Bill Karwin