A couple of months ago (well, actually more than a year ago!), while polishing snuffleupagus for its first release, my intern and I encountered an "interesting" issue: our hardened random feature was failing in certain environments, and we struggled to understand why.
To spice things up, we only managed to reproduce it with binaries without debug symbols and on an ArchLinux machine.
Here is the function in charge of everything random-related:
/* This function is needed because `rand` and `mt_rand` parameters * are optional, while the ones from `random_int` aren't. */ static void random_int_wrapper(INTERNAL_FUNCTION_PARAMETERS) { zend_long min, max, result; switch (EX_NUM_ARGS()) { case 0: min = 0; max = PHP_MT_RAND_MAX; break; case 1: // LCOV_EXCL_BR_START ZEND_PARSE_PARAMETERS_START_EX(ZEND_PARSE_PARAMS_QUIET, 1, 1); Z_PARAM_LONG(min); ZEND_PARSE_PARAMETERS_END(); // LCOV_EXCL_BR_END max = PHP_MT_RAND_MAX; break; case 2: default: ZEND_PARSE_PARAMETERS_START_EX(ZEND_PARSE_PARAMS_QUIET, 0, 2); Z_PARAM_LONG(min); Z_PARAM_LONG(max); ZEND_PARSE_PARAMETERS_END(); } if (min > max) { if (php_random_int_throw(max, min, &result) == FAILURE) { return; } } else { if (php_random_int_throw(min, max, &result) == FAILURE) { return; } } RETURN_LONG(result); }
We started by checking that it was indeed called when
rand was used in PHP,
and it was the case. So we ran the failing test in GDB to investigate further.
Since it struggled to find the function, we couldn't put a breakpoint on it;
so we slapped the good ol' __asm__("int3"); right at its beginning to trigger a
SIGTRAP, giving us a way to investigate what's going on.
It seemed that the function was returning early, but this is weird, because there
is only a single return, and it is at the very end of it, and two others that
are after calls to php_random_int_throw that would throw a warning before
returning if something went wrong (hence the throw in the name).
So we took at look at the control flow graph of the function, exported here thanks to radare2:
<@@@@@@>
f t
┌──────┘ └──┐
│ │
__5574__ __5600__
f t v
┌────┘ └───────│────┐
│ │ │
__5579__ │ │
t f │ │
┌────┘ └─┐ │ │
│ │ │ │
__5690__ │ │ │
v │ │ │
│ ┌──────┘ │ │
│ │ │ │
│ │ │ │
__5589__ │ │
f t │ │
┌──────┘ └──┐ │ │
│ │ │ │
__5591__ __5670__ │ │
v f t │ │
└───┐ │ └───────────────┤
│ │ │ │
│ __5681__ │ │
│ v │ │
└─┌───────┘ │ │
│ │ │
│ │ │
__5598__ │ │
f t │ │
┌────┘ └────┬───────────────┘ │
│ │ │
__55a1__ __560a__ │
t f t f │
┌─────┘ └────┬──────┘ └─────┐ │
│ │ │ │
__5648__ __55c2__ __5624__ │
v v v │
└──────────────────────┬────┴────────────┘
│
v
__55ef__
There are 3 arrows at the end that are going to the terminal node, but oddly
enough, there are 2 other ones on the right that are going there too. It seems
that there is something fishy in the ZEND_PARSE_PARAMETERS_START_EX macros.
#define ZEND_PARSE_PARAMETERS_START_EX(flags, min_num_args, max_num_args) do { \ const int _flags = (flags); \ int _min_num_args = (min_num_args); \ int _max_num_args = (max_num_args); \ int _num_args = EX_NUM_ARGS(); \ int _i; \ zval *_real_arg, *_arg = NULL; \ zend_expected_type _expected_type = IS_UNDEF; \ char *_error = NULL; \ zend_bool _dummy; \ zend_bool _optional = 0; \ int error_code = ZPP_ERROR_OK; \ ((void)_i); \ ((void)_real_arg); \ ((void)_arg); \ ((void)_expected_type); \ ((void)_error); \ ((void)_dummy); \ ((void)_optional); \ \ do { \ if (UNEXPECTED(_num_args < _min_num_args) || \ (UNEXPECTED(_num_args > _max_num_args) && \ EXPECTED(_max_num_args >= 0))) { \ if (!(_flags & ZEND_PARSE_PARAMS_QUIET)) { \ zend_wrong_parameters_count_error(_flags & ZEND_PARSE_PARAMS_THROW, _num_args, _min_num_args, _max_num_args); \ } \ error_code = ZPP_ERROR_FAILURE; \ break; \ } \ _i = 0; \ _real_arg = ZEND_CALL_ARG(execute_data, 0);
After a lot of exploration (protip: ctags is your friend.),
we didn't find anything suspicious that would trigger an early return. But notice how the do {
aren't closed: this is why this macro has a friend called
ZEND_PARSE_PARAMETERS_END() that you must call when you're done parsing
arguments. It's looking like this:
#define ZEND_PARSE_PARAMETERS_END_EX(failure) \ } while (0); \ if (UNEXPECTED(error_code != ZPP_ERROR_OK)) { \ if (!(_flags & ZEND_PARSE_PARAMS_QUIET)) { \ if (error_code == ZPP_ERROR_WRONG_CALLBACK) { \ zend_wrong_callback_error(_flags & ZEND_PARSE_PARAMS_THROW, E_WARNING, _i, _error); \ } else if (error_code == ZPP_ERROR_WRONG_CLASS) { \ zend_wrong_parameter_class_error(_flags & ZEND_PARSE_PARAMS_THROW, _i, _error, _arg); \ } else if (error_code == ZPP_ERROR_WRONG_ARG) { \ zend_wrong_parameter_type_error(_flags & ZEND_PARSE_PARAMS_THROW, _i, _expected_type, _arg); \ } \ } \ failure; \ } \ } while (0) #define ZEND_PARSE_PARAMETERS_END() \ ZEND_PARSE_PARAMETERS_END_EX(return)
Still nothing suspiciou… wait. The failure parameter of the macro is used as a
statement near the end of it! This is where our returns are coming from!
The (ghetto) solution is the call ZEND_PARSE_PARAMETERS_END_EX((void)0)
instead of ZEND_PARSE_PARAMETERS_END,
and this is exactly what we did!
If you know a better way to do it, please tell us!
Until next time, see you in your php stack ;)