Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Safely calling a function based off user input

Tags:

security

php

I'm trying to create an AJAX script that will take two GET variables, class and method, and map them to methods we've designed (similar to how CodeIgniter acts for ajax, I'm pretty sure). Since I'm relying on user input to determine what class and method to execute I'm worried that there may be some way for a hacker to use that technique to their advantage.

The code:

//Grab and clean (just in case, why not) the class and method variables from GET
$class = urlencode(trim($_GET['c']));
$method = urlencode(trim($_GET['m']));

//Ensure the passed function is callable
if(method_exists($class, $method)){
    $class::$method();
}

Are there any disadvantages or security watch-outs I should be aware of while using this technique?

like image 957
ACobbs Avatar asked Jul 10 '12 13:07

ACobbs


3 Answers

Check if method is allowed to be called by user:

// methods that user can call:
$user_methods = array("method1", "method2", "method3", );

//Ensure the passed function is callable
if(method_exists($class, $method) and in_array($method, $user_methods){
    $class::$method();
}

Otherwise you'll have no control what user will be able to do.

like image 191
Mariusz Jamro Avatar answered Sep 30 '22 01:09

Mariusz Jamro


<?php
class AjaxCallableFunction
{
    public static $callable_from_ajax = TRUE;
}

$class = $_POST['class'];
$method = $_POST['method'];

if ( class_exists( $class ) && isset( $class::$callable_from_ajax ) && $class::$callable_from_ajax ) {
    call_user_func( $class, $method );
}

Combine with some of the other answers for best results. Requires PHP 5.3.0 or higher. You could even implement an interface

<?php
interface AjaxCallable {}

class MyClass implements AjaxCallable 
{
    // Your code here
}

$class = $_POST['class'];
$method = $_POST['method'];

if ( class_exists( $class ) && in_array( 'AjaxCallable', class_implements( $class ) ) ) {
    call_user_func( $class, $method );
}

This approach follows OOP principles, is very verbose (Easy to maintain) and doesn't require you to maintain an array of which classes can be called and which cannot.

like image 37
Brandon Wamboldt Avatar answered Sep 30 '22 01:09

Brandon Wamboldt


Considering you are not passing any arguments, this is relatively safe for now. But i would add a list of valid classes in your IF such as:

//Ensure the passed function is callable
if(method_exists($class, $method)){
    if(in_array($class, array('controller1', 'controller2'))){
        $class::$method();
    }
}

This way, a hacker can't really call any possible class in the framework this way but only the ones you allow him to.

like image 43
Mathieu Dumoulin Avatar answered Sep 30 '22 00:09

Mathieu Dumoulin