ZF-11817: magicKey generated in _recursiveJsonExprFinder lacks sufficient distinction
Description
I'm submitting this as major because it is a class of error that often has security implications.
Zend_Json::_recursiveJsonExprFinder() uses the following code to generate magic keys
bq. $magicKey = "____" . $currentKey . "_" . (count($javascriptExpressions));
These keys are short and highly predictable. Constructing an input that will cause an incorrect replace is trivial:
bq. array('foo'=>'___event_0', 'event'=>new Zend_Json_Expr('function(){}'));
The old system of including the key in the match wouldn't work either, since this input would still break:
bq. array('foo'=>array('event'=>'___event_0'), 'event'=>new Zend_Json_Expr('function(){}'));
The above examples only cause the result to be wrong, but I'm fairly certain that a more elaborate construct could be used to "break out" in certain cases (at which point this would become a security issue).
At a minimum something like the following should be used instead:
bq. $magicKey = "____" . $currentKey . "" . (count($javascriptExpressions)) . '' . md5($value->__toString()) . mt_rand() . time();
The base is the same, but it includes a couple of additions that dramatically reduce the chances of incorrectly matching rogue data: md5($value->__toString()) - This tightly couples the key to the actual value it will be replaced with. This is still predictable, but the odds of an accidental match with legitimate data effectively drop to zero.
mt_rand() - Makes the key volatile. For malicious data to exploit the key replacement it would need to predict the value of mt_rand. This actually isn't sufficient; mt_rand is not cryptographically secure, and depending on the server's other outputs an attacker may be able to get enough data to predict all future outputs of mt_rand. Ideally something like openssl_random_pseudo_bytes should be used, but mt_rand is better than nothing.
time() - Limits the window of time where a malicious input can function. microtime would be better but doesn't work on all systems.
Comments
Posted by Satoru Yoshida (satoruyoshida) on 2011-10-26T09:54:55.000+0000
Set component and Auto-reassign