4
\$\begingroup\$

I've inherited a class in a project which is purposed to execute a function that exists in POST data from a specified class. I've tried to clean this up as much as possible as well as secure it against SQL injection but I'm wondering if I could have done a better job or written this better. Any assistance would be much appreciated!

class RegistrationProceduresController
{
    public function __construct($username, $password, $host, $port, $dbname)
    {
        $SP = new RegistrationProceduresModel($username, $password, $host, $port, $dbname);

        // Sanitize all the incoming POST data and move into an array of its own
        $sanitized = array_map(array($this, 'sanitize'), $_POST);

        // Check whether the method passed via our POST data is a public function within the model, or even exists
        // at all. If not then skip to logging.
        if(method_exists($SP, $sanitized['function']))
        {
            try
            {
                // Execute the function we passed via our post data against the model class post-sanitization.
                $SP->$sanitized['function']();
            }
            catch(exception $ex)
            {
                json_encode("Exception when trying to execute a sanitized function in registration controller: " . $ex->getMessage());
            }
        }
        else
        {
            try
            {
                $val = isset($sanitized['function']) ? $sanitized['function'] . " does not exist in model."  : "was not specified";

                json_encode("Function " . $val);
            }
            catch(exception $ex)
            {
                json_encode("Threw an error after failing to evaluate whether a $_POST method exists. Review registration controller. " . $ex->getMessage());
            }
        }
    }

    /// <summary>
    /// Takes an input and returns it with all HTML characters stripped.
    /// Protects against SQL injection.
    /// </summary>
    /// <param name="$input">Any string based input</param>
    public function sanitize($input)
    {
        try
        {
            return htmlspecialchars(trim($input));
        }
        catch(exception $ex)
        {
            json_encode("Failed to sanitize POST data array in registration controller: " . $ex->getMessage());
        }

        return null;
    }
}

This project is using POST data to call a function since it's being initialized by JavaScript. If there's a better way to call a PHP function from within JavaScript I would definitely, definitely value feedback on it!

\$\endgroup\$
  • \$\begingroup\$ Whatever you are trying to do, there has to be a better way than to use $_POST data to call a function! besides, your syntax is ambiguous: should PHP evaluate $SP->$sanitized['function'](); as $SP->{$sanitized}['function'](); where $sanitized is a string, containing the name of a property, that has been assigned an array with a function key that has been assigned a lambda, or is it $SP->{$sanitized['function']()};: a lambda returning a string, that is a property of $SP or is it $SP->{$sanitized['function']}();? use the latter to disambiguate \$\endgroup\$ – Elias Van Ootegem Dec 19 '13 at 9:03
  • \$\begingroup\$ Also json_encode a string: not a good idea. json_encode(array('error' => 'your custom string', 'msg' => $ex->getMessage(), 'code' => $ex->getCode())); is what you want \$\endgroup\$ – Elias Van Ootegem Dec 19 '13 at 9:05
  • \$\begingroup\$ @EliasVanOotegem See edit for justification - I'd love to know a better way! \$\endgroup\$ – Michael A Dec 19 '13 at 9:29
4
\$\begingroup\$

Well, I'm going to post it as an answer, simply because comments aren't the right place for this:

See edit for justification

You can't justify using $_POST data to call functions or methods in a script. never trust the network, POST data is coming from the network, never trust it. Full stop.
It's a piece of cake to send something like __toString, __invoke, __sleep and other magic-methods. They might exist, so they might pass the method_exist check you do. However, what they do is something different entirely.

You're basically calling a method on an object. That implies a finite list of methods that exist and are callable (there's a function called is_callable to chekc just that, BTW).
A safer way might be to have a property (array) containing a white-list of methods that can be invoked through JS requests:

$psMethods = array(
    'ajaxMethod1',
    'ajaxMethod2'
);

As an added bonus, you could use the method names as keys, and assign those keys a couple of arguments that the method might require.
The downside of this (a static array) is that you'll have to add and remove items from this array as you write your code. Don't worry however, help is at hand thanks to get_class_methods:

private $SP = null;
private $spMethods = array();//empty
//in constructor, or wherever you assign $this->SP
public function __construct(SPClass $sp)
{
    $this->SP = $sp;
    $methods = get_class_methods($sp);
    foreach($methods as $method)
    {
        if (substr($method, 0, 5) === 'ajax_')
        {
            $this->spMethods[] = substr($method,5);
        }
    }
}

Here, any method that has to be callable through an ajax request has to be given an arbitrary prefix (ajax_ in this case). That's how you can determine what methods are valid.
Then, when you receive your data:

if (in_array($_POST['function'], $this->spMethods)) //valid post data
    return $this->SP->{'ajax_'.$_POST['function']}();
else

What to do in the else cases? the choice is yours, really: either provide a default, fallback method in $this->SP:

public function ajax_fallback(array $post = null)
{
    return array('invalid/unknown request action', $post);
}

throw an exception or return a valid json encoded string (json_encode should be passed at least an array, strings needn't be encoded)

Resist the temptation to guess what $_POST['function'] should've been. If its contents is invalid, that can only mean one of the following things:

  • there's an error in either your PHP or JS code, in which case: fix it
  • clients trying to exploit your ajax call, in which case, don't try to help them by guessing, throw an exception. Amateur hackers aren't worth the code that'll only end up making it easier for them to actually achieve something
  • connection problems (unlikely), trying to work your way around those is pointless, so don't

Other considerations:
If you are going to post (part of) a function name to your server, don't call that parameter function. Everybody knows what a function is, so it doesn't take long for some spotty teen to realize which post param is worth trying to exploit!
Sending data over ajax isn't secure anyway, I'd not send a function name, but given the info you've provided, I can't tell which is the better way to go. It might be worth while to hash the function names, and compare the hash value to an array of hashes. The chances someone guesses a hash of (part of) a function name, especially when you add salt are very low indeed:

    $this->SP = $sp;
    $methods = get_class_methods($sp);
    foreach($methods as $method)
    {
        if (substr($method, 0, 5) === 'ajax_')
        {
            $this->spMethods[] = sha1(
                'YOUR_sup3rh3r03-0F=çH01ce'.substr($method,5);
            );
        }
    }

and just hard-code, or send these hashes over through a predefined ajax request.
Those teen-would-be-hackers are easily discouraged when they realize they have to use strings like "ff45204b9bbf938f7c10a734ed4e05434203790d", without knowing what they are :-)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Will review this in the morning and bubble it up to the project manager - thank-you! \$\endgroup\$ – Michael A Dec 19 '13 at 11:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.