jquery.php plugin security optimization (backend)

Question

This is the backend to my jquery.php plugin which handles requests to make function calls and execute PHP code that is sent in various forms. After requests are processed they're sent back to the client side as a JSON object with a type flag set so JavaScript can convert the returned data to the correct type for the user.

So far I have some configuration variables which control the security, allowing us to work in a blacklist or whitelist mode.

I'm looking for constructive advice related to creating code that will allow me to detect things like infinite loops and race conditions as well as anything else I haven't thought of related to security.

To provide a bit of context related to how the backend is used here is a JSON block that is sent to the server as it exists in JavaScript:

var codeBlock = {
    opts: 
    [
        {
            http: {
                method: "GET",
                header: "Accept-language: en\r\n" +
                        "Cookie: foo=bar\r\n"
            }
        }
    ],
    context: 
    {
        stream_context_create: ['opts']
    },
    contents: 
    {
        file_get_contents: ['http://www.YourDomain.com/', false, 'context']
    },
    html: 
    {
        htmlentities: ['contents']
    }
}

In the above code block I'm telling PHP that I want to call file_get_contents. In order to do that I need to define an options array named $opts that is passed to stream_context_create. The returned results from stream_context_create are then to be stored in a variable named $contents. We then store the returned results from file_get_contents into a variable named $contents. Finally we convert the returned data from file_get_contents into the variable $html.

Here's our backend request_handler.php

// Set our error reporting level
error_reporting(0); 

/* The blacklist array is used when the SEC_MODE configuration variable is set to 'blacklist'. When configured
 * to use the blacklist all functions listed in the array will be disabled.
 */
$blacklist = array(
    '`',
    'create_function',
    'escapeshellcmd',
    'exec',
    'include',
    'include_once',
    'passthru',
    'pcntl_exec',
    'phpinfo',
    'popen',
    'require',
    'require_once',
    'shell_exec',
    'system'
);

/* The whitelist array is used when the SEC_MODE configuration variable is set to 'whitelist'. When configured
 * to use the whitelist only functions listed in the array will be enabled.
 */
$whitelist = array(
    // 'strlen',            // (e.g. Allowing the strlen function)
    // 'highlight_string'   // (e.g. Etc...)
);

/* The jquery.php plugin allows for the execution of code provided by the client. This functionality should
 * be disabled by default. The 'exec' mode uses the eval() construct which can be very dangerous.
 */ 
$config = array(
    'EXEC'          => true,
    'SEC_MODE'      => 'blacklist',
    'LISTS'         => array(
        'blacklist' => $blacklist,
        'whitelist' => $whitelist
    )
);

/* The first data we look for from the client is which method request is being made. The method request is one
 * of our modes of operations and tells us which "method" to use in our switch statement.
 */
$method_request = $_POST['method'] ? $_POST['method'] : false;
if ( $method_request ) {

    switch ( $method_request ) {

        // The call "method" handles our requests to use PHP functions.
        case 'call' :

            // Retrieve the function requested and any arguments to be passed to it
            $func_request = $_POST['func'] ? $_POST['func'] : false;
            $func_args = $_POST['args'] ? $_POST['args'] : false;

            // Based on the security mode we use either our blacklist or whitelist.
            switch ( $config['SEC_MODE'] ) {
                case 'blacklist' :
                    if ( function_exists($func_request) 
                         && !in_array($func_request, $blacklist) ) {
                        $function = $func_request;
                    } else {
                        $function = false;
                    }
                    break;

                case 'whitelist' :
                    if ( function_exists($func_request) 
                         && in_array($func_request, $whitelist) ) {
                        $function = $func_request;
                    } else {
                        $function = false;
                    }
                    break;
            }

            // Convert our parameters string and convert it into an array
            $args_arr = json_decode($func_args, false);

            // Call the requested function if permitted
            if ( $function !== false ) {
                $call = $function;
                echo parse_type( call_user_func_array($call, $args_arr) );
            }
            break;

        // The exec "method" handles requests to execute PHP code strings
        case 'exec' :
            if ( $config['EXEC'] === true ) {

                 // We receive code to be executed by the user
                 $code_string = $_POST['code'] ? $_POST['code'] : false;

                 // Prefix our code with return to prevent NULL from being returned.
                 echo parse_type( eval( $code_string ) );
            } 
            break;

        // The block "method" handles requests to execute JSON blocks of PHP code
        case 'block' :

            // Retrieve our JSON string and convert it to an array
            $php_obj = $_POST['pobj'] ? json_decode( $_POST['pobj'] ) : false;

            // Pass our JSON decoded PHP objects array to our execution handler
            print parse_type( parse_php_object( $php_obj, $config ) );               
            break;
    }

}

/**
 * Converts PHP objects to arrays by typecasting.
 * @param {object} Object A self referencing PHP object.
 */
function object_to_array( &$object ) {
    if ( is_object( $object ) ) {
        (Array)$object;
    }
}

/**
 * Iterates over an array containing PHP and handles calls to enabled functions and executes them.
 * @param {phpObj} array A JSON decoded array of representational PHP.
 * @return {*} Will return the results of the last function call passed in through phpObj.
 */
function parse_php_object( $arr, $config ) {

    // We define a pointer array that contains reference names to parameter placeholders
    // that will be replaced by real data.
    $pointers = array();

    foreach ( $arr as $k => $v ) {

        // Create variable definition with our first level array keys
        ${$k} = $v;

        // Populate our pointers index
        $pointers[$k] = $k;

        // When a value is an object we attempt to call functions defined within
        if ( is_object( ${$k} ) ) {

            // Convert our function object to an array
            $funcArr = (Array)${$k};

            // Use the first key of the function array as our function name to call
            $func_name = array_keys($funcArr);
            $func_name = $func_name[0];

            // Get the array of arguments to parse to our arguments array
            $func_args = $funcArr[$func_name];

            // Create an array to store the arguments to pass to our function call
            $args_arr = array();

            // Now we iterate over our function arguments looking for reference strings
            foreach ( $func_args as $arg ) {

                // We compare against the keys in our pointers index which was created above
                if ( array_key_exists( $arg, $pointers ) ) {

                    // This is now a reference to ${$k}, the originally defined definition, the returned
                    // result of the last sucessful function call
                    $p = ${$arg};

                    // We push our arguments onto the args_array which will be passed to our function call
                    array_push( $args_arr, $p );

                } else {

                    // We push our arguments onto the args_array which will be passed to our function call
                    array_push( $args_arr, $arg );
                }
            }


            // Based on the security mode selected, use either our blacklist or whitelist.
            switch ( $config['SEC_MODE'] ) {
                case 'blacklist' :
                    if ( function_exists( $func_name ) 
                         && !in_array( $func_name, $config['LISTS']['blacklist'] ) ) {
                        $function_allowed = true;
                    } else {
                        $function_allowed = false;
                    }
                    break;

                case 'whitelist' :
                    if ( function_exists( $func_name ) 
                         && in_array( $func_name, $config['LISTS']['whitelist'] ) ) {
                        $function_allowed = true;
                    } else {
                        $function_allowed = false;
                    }
                    break;
            }

            // Call the requested function if permitted
            if ( $function_allowed === true ) {

                // Reassign our variable the returned value of a function call so that further function calls can
                // search for the existence of pointers and then use the updated variable definitions. This logic
                // takes advantage of the procedural nature of PHP and the order of the sub-blocks in the php object. 
                ${$k} = call_user_func_array( $func_name, $args_arr );
            } else {
                return ("Function you requested $func_name has been disabled by backend configuration.");
            }
        }

        // When we're not an object we're something else like an array, string, int, etc. If we're an array we need
        // to recursively iterate over ourselves to convert any objects into arrays.
        else {  
            if ( is_array( ${$k} ) ) {
                array_walk_recursive( ${$k}, 'object_to_array' );
            }
        }

    }

    // Return the returned result from our final function call
    return ${$k};
}

/**
 * Detects the type of a returned result and encodes a type identifier along with the data into a JSON object.
 * @param {*} data Result data from a PHP function call.
 * @return {Object} JSON encoded result data with type field.
 */
function parse_type( $data ) {

    $results = array(
        'type' => '',
        'data' => NULL
    );

    switch ( true ) {

        case is_int( $data ) :
            $type = 'int';
            break;

        case is_float( $data ) :
            $type = 'float';
            break;

        case is_string( $data ) :
            $type = 'string';
            break;

        case is_object( $data ) :
            $type = 'object';
            break;

        case is_array( $data ) :
            $type = 'array';
            break;

        case is_bool( $data ) :
            $type = 'bool';
            break;

        case is_null( $data ) :
            $type = 'null';
            break;

        default :
            $type = 'default';
    }

    $results['type'] = $type;
    $results['data'] = $data;
    return json_encode( $results );
}

Answer 1

I can't really help you with your specific questions, but I do have some suggestions.

The first, and most crucial, bit of advice, especially if you are worried about security, is not to turn off your error reporting. Instead you should turn it on all the way. While developing it is essential to know where your errors are and to fix them. This will help fill in those security holes as well as make your application more efficient. After development is done you still do not turn off error reporting, you hide it. Log those errors, that way if anything happens that shouldn't you will be able to recreate the scenario with your logs and remove the problems.

Now, I believe it was in version 5.3 that PHP began allowing you to use short ternary. So if your version is 5.3 or higher you can shorten that ternary like so:

$method_request = $_POST[ 'method' ] ?: FALSE;

However, this really isn't all that helpful here because you have not verified that that element exists. You are relying on your error reporting level to ignore the errors this is generating. Instead, your ternary should look like this:

$method_request = isset( $_POST[ 'method' ] ? $_POST[ 'method' ] : FALSE;

And then you should sanitize it before attempting to use it. Any user information, or possible user information (POST, GET, COOKIES, etc...), should always be sanitized. Of course you could do it all at once with filter_input() if you really wanted. This will make that ternary unnecessary. filter_input() automatically returns a FALSE value if the requested element doesn't exist.

$method_request = filter_input( INPUT_POST, 'method', FILTER_SANITIZE_STRING );

Using an if statement and then a switch with the same condition is redundant. Remove the if statement. Since you don't have an else statement to go with it, then there is no need to do anything else. If you decide later to add an else statement, you can just add a FALSE or default case to your switch statement. It will accomplish the same thing.

//if( $method_request ) {//redundant, a switch is essentially an if statement
switch( $method_request ) {//the switch holds the item to be compared
    case FALSE ://the case holds the comparison
        //an else clause would go here
    break;
    default :
        //default can also be used for an else clause
    break;
}

Your code is violating a few different principles here. Key among them are: "Don't Repeat Yourself" (DRY) and the Arrow Anti-Pattern. The first should be rather self explanatory: Your code shouldn't repeat. Creating additional functions to handle repetition or similar tasks will help out with this. The second principle is about heavy and unnecessary indentation. Your code is quite heavily indented when it needn't be. Removing that if statement I suggested earlier will help with this, as will creating additional functions.

A purely aesthetic change would be to remove all of those internal comments. If your code is self-documenting, your variables clear and understandable, and you are following core principles, then your code should be able to speak for itself. Anything else should be limited to doccomments. All these internal comments do is add clutter.

Variable-variables are usually considered a security risk, but, as it stands, I don't think they can be avoided. I will point out however, that the curly braces around the entity variable is unnecessary. So, unless it helps with clarity, you can remove them. But that's up to you. There really isn't a standard when it comes to variable-variables because they themselves aren't standard.

${$k} = $v;
//is the same as
$$k = $v;

Your parse_type() function is recreating a function that already exists in PHP. You can just use gettype() to accomplish the same thing.

$type = gettype( $data );

Now, all that being said, I don't really understand your need for this convoluted workaround. It seems like you should be able to accomplish the same thing, minus the headache and potential security risks, by simply using pure PHP. For example, you could pass that IP address to your page via an AJAX or normal GET request and have that page get the contents of the address and return it. There doesn't seem to be any need for the abstractness of variable-variables, exec(), and eval(). This just seems overly complicated, but maybe I'm missing something.