php code acceptable from security point of view?

Question

Consider the following code snippet. Is this code acceptable from a security standpoint? Assume that the $action and $data variables are designed to be accepted from the user and register_globals is enabled.

<?php

if(common::IsUserAdmin($userID))
 {
 $isAdmin = true;
 }
   $data = common::Validate_And_Return_Input($data)
   Switch($action)
    {
     case “add”:
     common::addSomething($data);
     break;

     case “delete”:
     if($isAdmin)
     {
    common::deleteSomething($data);
         }
    break;
   case “edit”:
   if($isAdmin)
   {
    common::editSomething($data);
   }
   break;
  default:
  echo “Bad action.”;
  }
?>

Answer 1

As you didn't show any code: from a security standpoint, there is nothing to secure. So just zip it up into a file and store it away to let it rot for 10 years until you delete it.

If you actually even intend to run that on a server connected to the internet, you should follow the bare minimum from the suggested security topics in the PHP manual including to disable register globals.

If you finally managed that (there are more topics), you might be even able to actually post code examples that do show some of your data handling instead of hiding that away behind no-saying function names. Validate for what? Return to where?

So actually, there is not much to say about your code here, as there isn't much code.

Hope this was helpful.

Answer 2

Obviously register_globals is better off (security wise). If you can, disabled it. However, if that is not an option (legacy systems, etc) here is some feedback.

Change to the $isAdmin check:

// This prevents register_globals from overwriting $isAdmin
$isAdmin = common::IsUserAdmin($userID);
$data = common::Validate_And_Return_Input($data)

// The rest of the code
// ....

The switch is a good method of filtering out unwanted data in $action. That's fine.

Also if you're expecting a set number of options from the user, check them against the list to ensure that they are safe:

$allowed = array('a', 'b', 'c', 'd');
if (in_array($user_input, $allowed))
{
    // Do your stuff. $user_input is safe
}

And finally take advantage of type-casting variables that you know are (or expect to be) integers/floats to ensure that you're getting what you expect:

$sanitized_input_int = (int)$user_input_int;
$sanitized_input_float = (float)$user_input_float;