3
\$\begingroup\$

I am using preg_replace_callback(), and I am concerned that I might have some sort of security risk. I am hoping to get a review of the below substitute() function. The purpose of the function is to parse a user provided template with user provided data. A quick description on how it is used is shown below.

The user posts data to the server, which is saved in a database (escaped for SQL injection of course). Note that the application will limit which array indexes are allowed (i.e. firstname, lastname, and phone only in this example).

$userProvidedString='Hello {?firstname?} {?lastname?}, Our database shows that your phone number is {?phone?}.';

$userProvidedArray=array(
    0 => array('firstname'=>'John','lastname'=>'Doe','phone'=>'(555) 222-1212'),
    1 => array('firstname'=>'Jane','lastname'=>'Doe','phone'=>'(555) 333-1212'),
    2 => array('firstname'=>'Mike','lastname'=>'Moe','phone'=>'(444) 222-1212'),
);

Later the data is pulled from the database, and parsed as follows:

foreach($userProvidedArray AS $contact)
{
    $customMessage=substitute($userProvidedString,$contact);
}

My question relates to the substitute() function. Specifically, does my use of preg_replace_callback() pose any security issues? Previously, I was using

$template_new= preg_replace('/\{\?(\w+)\?\}/e', '$contact["$1"]', $userProvidedString);

which uses the /e flag which I assume can be dangerous.

Does that danger still remain? Any other recommendations for it? I am currently using PHP 5.3.18, but will hopefully upgrade to 5.4 soon.

function substitute($template, $values, $deliminator='{??}') {
    //Replaces occurrences of {?name?} with $values['name] if it exists in $values.  Deliminators can be changed
    $deliminator="\\".implode("\\",str_split($deliminator));
    $half = (int) ( (strlen($deliminator) / 2) );
    $pattern = '/'.substr($deliminator, 0, $half).'(\w+)'.substr($deliminator, $half).'/';
    return preg_replace_callback(
        $pattern,
        function ($matches) use ($values) {
            if (isset($values[$matches[1]])) {
                return $values[$matches[1]];
            }
            return $matches[0];
        },
        $template);
}
\$\endgroup\$
3
\$\begingroup\$

Escaping the RegExp

You should never do manual escaping, if an appropriate function exists. For RegExp, this is preg_quote. Another risk is that you'r escaping before splitting, so you might destroy an escaping sequence. Better do

function substitute($template, $values, $delimiter = '{??}')
{
    $half    = (int) ( (strlen($delimiter) / 2) );
    $begin   = preg_quote(substr($delimiter, 0, $half), '/');
    $end     = preg_quote(substr($delimiter, $half), '/');
    $pattern = '/' . $begin . '(\w+)' . $end . '/';

Escaping the Values

The user provided values maust be escaped according to the context, where they are used. In this case it is HTML, so you must suppress unwanted tags, especially scripting tags. You can use strip_tags() or htmlspecialchars() for that purpose. I prefer the latter, because the user's value is not changed.

    return preg_replace_callback(
        $pattern,
        function ($matches) use ($values) {
            if (isset($values[$matches[1]])) {
                return htmlspecialchars($values[$matches[1]]);
            }
            return $matches[0];
        },
        $template);
    }
}

Conclusion

With these two small changes you have done everything to make that function save.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.