Artificial truth

The more you see, the less you believe.

[archives] [latest] | [homepage] | [atom/rss]

Excerpt of a PHP debugging session while working on Snuffleupagus
Tue 20 August 2019 — download

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 ;)