Thoughts about the object reviving threat

[dcz]

Hello,

I first posted this in the security tracker, but bantu told me that since this was not a security issue with the current phpBB code, this should rather be discussed publicly here.

So here is my original post :

While investigating upon unserialize security concerns, it came to my mind that phpBB probably should prevent any object wake up while reading the file cache.
Not that phpBB currently uses classes with the concerned magic methods, but since the cache class is not meant to cache object it seems to me a good practice to enforce this in the acm_file.php.
The cache/ directory being the "easiest" to write in, someone could alter an existing cache file (or create one supposed to exist at one point) without necessarily having the full control over the server and possibly exploit the object reviving capabilities of unserialize.

Here is an detailed example of a successful use of this threat : http://www.suspekt.org/2009/12/09/advis ... erability/

After testing ways to make sure that unserialize would not treat serialized object, I came up with the following function :

Code: Select all

/**
* mixed safe_unserialize(string $serialized)
* Safely unserialize, that is only unserialize string, numbers and arrays, not objects
*
* @param string $serialized
* @return mixed
*/
function safe_unserialize($serialized) {
    // unserialize will return false for object declared with small cap o
    // as well as if there is any ws between O and :
    if (is_string($serialized) && strpos($serialized, "\0") === false) {
        if (strpos($serialized, 'O:') === false) {
            // the easy case, nothing to worry about
            return @unserialize($serialized);
        } else if (!preg_match('/(^|;|{|})O:[0-9]+:"/', $serialized)) {
            // in case we did have a string with O: in it,
            // but it was not a true serialized object
            return @unserialize($serialized);
        }
    }
    return false;
}

Benchmarking this shows that unserialize is only 10% faster. This makes this proposal for a safe unserialization a lot faster than the ones relying on a full replacement of the original function (being pretty much all around 10 times slower than the original).

Using this function instead in acm_file.php would ensure that the object reviving threats would be inapplicable.

I understand that you may argue that this is not truly a phpBB security issue, but even if object waking cannot currently be exploited with vanilla phpBB, it's not necessarily the case with all phpBB installations and / or with upcoming phpBB versions, and since it's not a performance issue either, it would still fell like a good practice to me to enforce the obvious : phpBB does not cache objects.

So it's obviously a detail, but as we all know, evil is hiding into details. And this would only mean a couple more lines and two code changes, so the real question left in my mind is why not do it ?

[Oleg]

If you care about security this much then you can run phpbb under its own user account, at which point nobody else will be able to write to phpbb's cache.

What would be an example security issue that your solution prevents from being exploited?

The link you provided deals with deserializing user input, which is entirely different from deserializing phpbb's generated output.

On a side note, I continue to be amazed by the number of convenient security holes built into php.

Because the filter wrapper can be stacked several times before data
is written to the actual file it is possible to construct payloads
that do not only destroy the show stopper but also get rid of all the
bytes.

All this while trying to write a file.

[dcz]

Well, this all came from my recent acknowledgement of such wicked unserialize exploit that made me audit my code again. And if I did not find any way to use such exploit, I still preferred to state the obvious wherever possible when object where not desired as a result of an unserialize call. And I did it also for non user input, since the 10% extra work is worth it to me to feel safe that I will never revive, even harmlessly, any object by mistake or after someone manipulated some data (we can even imagine a MITM between the web server and the SQL server after all).

Then, I decided to report this here just because it seems to me a good practice to enforce the obvious : phpBB's cache does not cache objects.
My point is simple :
a) A very unlikely event with potentially high consequences must never be under estimated (atomic plants are here to remind us this every day)
b) If not only by principle (which for me is enough here, especially if the run time cost is so close to none), the fact that the phpBB's cache directory is many time publicly writeable, and that phpBB is often used with mods, and the fact that phpBB is often used with more php around (the whole site) does nothing but increase the probability that such attack becomes possible (small probability time many cases = higher probability).

Your point about running phpBB under its own user is not a solution for many hosting cases, and when applicable, in many cases the server itself will still need some rights on these.

This being said, I know that you could argue that since phpBB vanilla is not concerned, there is no reason to do anything, but, this reminds me about the duplicated auth_option bug : http://www.phpbb.com/bugs/phpbb3/ticket ... t_id=41835

It's a good example of not enforcing the obvious (auth_option uniqueness in db) This never was a bug triggered by phpBB, but it still ended up affecting phpBB installations in the real world. And, after some discussions, ended up taken into account and fixed despite the fact it never affected the phpBB code.

These two cases are similar in the sense that they deal with stating the obvious on rather critical matters (auths are critical, and so is server integrity) and can be fixed with very small changes (in the present case, there is not even to adapt the updating script).

In the end this all goes down to (at least in my mind) :
Why not enforcing the obvious before it ever gets actually exploited (even if it never does), especially when the "fix" is so tiny and costless at run time ?

I honestly cannot find any reason why not doing it. Especially if we keep in mind the high level of sophistication of the example exploit which IMHO makes it rather hard to reach certainty that this will never be exploitable in any site using phpBB.

The tiny probability of this issue to ever be exploited would go down to zero, preventing an unlikely but very high risk.

[Noxwizard]

but since the cache class is not meant to cache object it seems to me a good practice to enforce this in the acm_file.php.

The cache system currently doesn't restrict what you can put in it, so if a MOD happens to cache objects, users using the file system cache would not get the full benefit of having a cache.

As for attackers writing malicious serialized entries, that's somewhat questionable as a defense for this argument. In the cases that I have seen, the most common change to the cache files is that malicious code is appended at the bottom. In the case of cache data files, they are overwritten fairly quickly as $cache->get will fail on the unserialize and the file will usually be refreshed with a new and correct copy of the data. Being sneaky and rewriting the serialized object here will allow the attack to proceed, yes. The main problem is with the cached template files. These are guaranteed to execute the attacker's code (unless debug_extra is enabled). If the attacker knows that data files are protected, they will just write to the template files.

So the question is, what is it that this fix is trying to protect? It's not the problem of the cache being writable as a whole, but a very targeted instance of cache exploitation. When I see these cases arise, it is a widespread append to all writable files, not one that's highly targeted. So the protection provided by this fix against current attacks is very minute.

[dcz]

The cache system currently doesn't restrict what you can put in it, so if a MOD happens to cache objects, users using the file system cache would not get the full benefit of having a cache.

I'm not so sure that storing objects is relevant with all acm's, and even less that in case they all allow it, they would all revive them the same way as unserialize. So I'm not so sure that the mod caching object case is relevant. If object caching does makes sense in all acm's, it's not necessarily a reason to allow it under all circumstances, and at least, the object reviving should be consistent among acm's (which I believe is not the case so far).

You made a good point about the writeable cache/ directory case, there is very little point to care about serialized cache files when you have templates ones. Though, it may not be even necessary to go there to activate the threat. A bad mod could cache some user input after all (going through request_var is not enough in such case since serialized object do not need htmlspecialchars to be set), or just use some serialized user input.
For example, http://www.phpbb.com/support/srt/ is using serialized user input. I don't know about the code behind it, but I'm pretty sure that the benefit of a phpBB safe_unserialize function would be far from null. request_var was introduced as a way to enforce safe user input retrieval, why not doing the same with unserialize, even if only to enforce good coding practices ?

[Oleg]

I'm not so sure that storing objects is relevant with all acm's, and even less that in case they all allow it, they would all revive them the same way as unserialize. So I'm not so sure that the mod caching object case is relevant.

If it is possible to deserialize an object as part of an exploit, then it is possible for someone to legitimately be deserializing objects. No?

If object caching does makes sense in all acm's, it's not necessarily a reason to allow it under all circumstances, and at least, the object reviving should be consistent among acm's (which I believe is not the case so far).

I was under the impression that all acms have the same user-visible behavior. If this is not the case we should probably document it somewhere.

[dcz]

If it is possible to deserialize an object as part of an exploit, then it is possible for someone to legitimately be deserializing objects. No?

Of course. The cache point of view may not have been the bast way to evaluate the need to think about this threat.

The problematic indeed needs to be treated at at higher level : Do we always want to support object unserialization ?

For example, http://www.phpbb.com/support/srt/ is using serialized user input. I don't know about the code behind it, but I'm pretty sure that the benefit of a phpBB safe_unserialize function would be far from null. request_var was introduced as a way to enforce safe user input retrieval, why not doing the same with unserialize, even if only to enforce good coding practices ?

IMHO, a simple wrapper with an extra option to allow (or not) objects in serialization process makes sense in any php application using it.

[Oleg]

IMHO, a simple wrapper with an extra option to allow (or not) objects in serialization process makes sense in any php application using it.

I suppose the paranoid part of me can agree with this, but I would like to check whether any modifications are serializing objects before disabling this capability. Can you think of a test we can run on code in moddb to ascertain which modifications serialize objects?

[dcz]

Well, for user input, it's rather easy, you could just strpos($mod_code, 'unserialize') during the mod validation process to find out if the mod is itself using unserialize, and if so issue a warning to have the user use the phpbb_unserialize function with proper $allow_obj setting :

Code: Select all

/**
* unserialize wrapper, only unserialize objects when requested
*
* @param string $serialized The serialized string
* @param bool $allow_obj Allow object unserialization or not
* @return mixed
* @author dcz at phpbb-seo dot com
*/
function phpbb_unserialize($serialized, $allow_obj = false)
{
    // unserialize will return false for object declared with small cap o
    // as well as if there is any ws between O and :
    if (is_string($serialized) && strpos($serialized, "\0") === false)
    {
        if ($allow_obj)
        {
            return @unserialize($serialized);
        }
        else if (strpos($serialized, 'O:') === false)
        {
            // the easy case, nothing to worry about
            // let unserialize do the job
            return @unserialize($serialized);
        } 
        else if (!preg_match('/(^|;|{|})O:[0-9]+:"/', $serialized))
        {
            // in case we did have a string with O: in it,
            // but it was not a true serialized object
            return @unserialize($serialized);
        }
    }
    return false;
} 

Then, for caching, it's trickier. A working roadmap would be to add safe unserialization support in 3.1 and highlight the new coding practice in the changelog for mod authors. This would mean to at least add an $allow_obj (defaulting to false) variable in cache::get.
Mod validation could use a custom phpbb_unserialize (for example defined with an if (defined('IN_VALIDATION'))) that would log a trace to all calls with objects, or simpler to have this validation function trigger an error every time the tested mod attempts to store an object without the proper $allow_obj input, or simply list all calls to cache->put in the mod's code to let the human validator check them manually.

Defaulting to disallow object should though already be, together with proper announcement of the new functions, a good way to get mod authors attention on the matter, a mod using object serialization would stop working in a rather obvious way unless the author would have properly set the new option.

We can also wonder if object should be allowed right from serialization (the real matter comes with unserialize, but while we're at it), which could be something like :

Code: Select all

/**
* serialize wrapper, only serialize objects when requested
*
* @param mixed $input The variable to serialize
* @param bool $allow_obj Allow object serialization or not
* @return mixed
* @author dcz at phpbb-seo dot com
*/
function phpbb_serialize($input, $allow_obj = false)
{
    // start with serializing
    $result = @serialize($input);
    if ($result && !$allow_obj) 
    {
        if (strpos($result, 'O:') !== false)
        {
            // we may serialized an object
            if (preg_match('/(^|;|{|})O:[0-9]+:"/', $result))
            {
                // we did
                return false;
            }
        }
    }
    return $result;
}

With this one also, cache::put should also use an $allow_obj variable (defaulting to false).

Generally speaking about caching, all acm's should use these two functions to prevent any surprise before they actually cache anything to enforce the principle (and guarantee that objects will indeed always be revived the exact same way when desired).

Last thing that could be argued is the serialize(false) case. If we where to want a special handling for this case (I'm really not sure it is useful), we could return null for errors and test the serialized result, if $result === "b:0;" => return false else return real result if !== false else return null, but this would mean not to properly handle the serialize(null) case. My opinion would be that returning false like in the suggested code is ok.

Another thing that could be done to enforce (to death if I may so speak) the object serialization policy would be to always trigger an error when object are being serialized/unserialized without the proper $allow_obj input. That would be an ultimate way to warn mod authors.

Code: Select all

/**
* unserialize wrapper, only unserialize objects when requested
*
* @param string $serialized The serialized string
* @param bool $allow_obj Allow object unserialization or not
* @return mixed
* @author dcz at phpbb-seo dot com
*/
function phpbb_unserialize($serialized, $allow_obj = false)
{
    // unserialize will return false for object declared with small cap o
    // as well as if there is any ws between O and :
    if (is_string($serialized) && strpos($serialized, "\0") === false)
    {
        if ($allow_obj)
        {
            return @unserialize($serialized);
        }
        else if (strpos($serialized, 'O:') === false)
        {
            // the easy case, nothing to worry about
            // let unserialize do the job
            return @unserialize($serialized);
        }
        else if (!preg_match('/(^|;|{|})O:[0-9]+:"/', $serialized))
        {
            // in case we did have a string with O: in it,
            // but it was not a true serialized object
            return @unserialize($serialized);
        }
        else 
        {
            trigger_error('Tried to unserialize an object while not allowing it', E_USER_ERROR);
        }
    }
    return false;
}
/**
* serialize wrapper, only serialize objects when requested
*
* @param mixed $input The variable to serialize
* @param bool $allow_obj Allow object serialization or not
* @return mixed
* @author dcz at phpbb-seo dot com
*/
function phpbb_serialize($input, $allow_obj = false)
{
    // start with serializing
    $result = @serialize($input);
    if ($result && !$allow_obj)
    {
        if (strpos($result, 'O:') !== false)
        {
            // we may serialized an object
            if (preg_match('/(^|;|{|})O:[0-9]+:"/', $result))
            {
                // we did
                trigger_error('Tried to serialize an object while not allowing it', E_USER_ERROR);
            }
        }
    }
    return $result;
}

[DavidIQ]

Out of 700+ MODs in the MODDB to say that there are more than a handfull that use object serialization at all is probably an over estimation...this isn't done much if at all.