How to secure a template engine in PHP from injection

Question

I am currently trying to create a simple template engine in PHP. The main thing I care about is security, however template tutorials do not. Lets say I have a database table with a username and his description. The user can type whatever he wants there.

My guess would be to use htmlspecialchars() function, to prevent javascript and html injection. But what about 'template code injection'? If my template rule is to replace [@key] to "value", the user can update his description that interferes with my template handler. Should I treat "[", "@", "]" as special characters and replace them with their ascii code when using my set method?

template.php:

class Template {
    protected $file;
    protected $values = array();

    public function __construct($file) {
        $this->file = $file;
    }

    public function set($key, $value) {
        $this->values[$key] = $value;
    }

    public function output() {
        if (!file_exists($this->file)) {
            return "Error loading template file ($this->file).";
        }
        $output = file_get_contents($this->file);

        foreach ($this->values as $key => $value) {
            $tagToReplace = "[@$key]";
            $output = str_replace($tagToReplace, $value, $output);
        }

        return $output;
    }
}

example.tpl:

Username: [@name]
About me: [@info]

index.php:

include 'template.php';
$page = new Template('example.tpl');
$page->set('info', '[@name][@name][@name]I just injected some code.');
$page->set('name', 'Tom');
echo $page->output();

This would display:

Username: Tom
About me: TomTomTomI just injected some code.

The code I used is based on:

http://www.broculos.net/2008/03/how-to-make-simple-html-template-engine.html

Answer 1

I was thinking about it and I think this solution is fastest and simplest:

foreach ($this->values as $key => $value) {
    $tagToReplace = "[@$key]";
    if (strpos($output, "[@$value]") !== FALSE)
      $value = '['.substr($value,1,-1).']';
    $output = str_replace($tagToReplace, $value, $output);
}

It replaces brackets in value with html entity string if [$value] is in output.

Used this html entity list

For future adopters: This kind of solution is OK if Template Parser is implemented by loading non-interpeted/non-evaled file (as is OP's case by loading local file using file_get_contents). However, if it's implemented by INCLUDING PHP view, beware that this check won't handle the case when you put some user-modifiable data from database into view directly (without using parser, e.g. <?=$var;?>) and then use template parser for this view. In that case, parser has no way to know if these data are part of template structure and this check won't work for them. (I don't know how should this case be handled properly.) Anyhow, I think best practice is to never pass sensitive data to template parser, even if you don't use them in your template. When attacker then tricks parser to eval his custom data, he won't get information he didn't already have. Even better, don't use template parser.

Answer 2

Change your function to search in the unchanged template only once for the known keys:

public function output() {
    if (!file_exists($this->file)) {
        return "Error loading template file ($this->file).";
    }
    $output = file_get_contents($this->file);

    $keys = array_keys($this->values);
    $pattern = '$\[@(' . implode('|', array_map('preg_quote', $keys)) . ')\]$';

    $output = preg_replace_callback($pattern, function($match) {
        return $this->values[$match[1]];
    }, $output);

    return $output;
}