How is this PHP authentication function hackable?

Question

This is a recursive function I wrote to determine whether or not a given user is authorized to view content on a page. It is called in essentially the following fashion:

if(authorize($_SESSION['user']['user_id'], $necessaryClearance)){
    //Output restricted content
} else{
    //Inform user they are not authorized
}

Every user has a clearance level, as well as a clearance status. This allows an authorize function to be called with $clearance as a clearance level the user has to match or beat, a clearance status that a user has to match, or an array of statuses - any one of which the user can match. Generally, the $user_id is pulled from session data ($_SESSION['user']['$user_id'], which is refreshed from a database each page load), and the clearance is set explicitly either on a per-page or per-module basis.

//This function checks if the user is authorized to view the page
//It returns 1 if access is granted and a 0 if access is denied
function authorize($id, $clearance){
 //$clearance == array
    if (is_array($clearance)){
   //if yes Iterate array through Authorize($id, $clearance[])
  foreach($clearance as $userStatus){
   $tally += authorize ($id, $userStatus);

  }
   return $tally;
 //if no check if $clearenance is equal to a string
 }else if (is_string ($clearance)){
  $string = "SELECT status

  FROM users
      WHERE id = '$id'
      LIMIT 1";
  //If result returned.
  if($userData = mysql_fetch_array(Query($string))){
   if($clearance == $userData['status']){
    return 1;
   }else{ 
    return 0;
   }
  } else{
   return 0;
  }
  // if no check if $clearance is equal to a number  
 }else if(is_numeric($clearance)){
  $string = "SELECT level
      FROM users
      WHERE id = '$id'
      LIMIT 1";
  //If result returned
  if($userData = mysql_fetch_array(Query($string))){ 
   // if number is less than or equal to clearance level allow access
   if($userData['level'] <= $clearance){
    return 1;
   }else{ 
    return 0;
   }
  } else{
   return 0;
  }

 }else{
  //if nothing matches the page dies
  die('Authorization has failed.');
 }
}

Are there any glaring security flaws in the code?

Answer 1

Yes. You're not doing any escaping on the $id parameter!

This means that your queries are susceptible to a SQL Injection attack.

Answer 2

You have not provided enough information. Where is $id coming from? Is it a get/post/cookie value? Because if it is then you can just say $id=1. This is called "Insecure Direct Object Reference."

There is also the case SQL Injection. You could inject a simple tautology such as ' or 1=1 or do something more insidious such as ' and 0=1 union select "<?php eval($_GET[e])?>" into outfile /var/www/backdoor.php.

Answer 3

As long as you save the user_id in a variable that in no way is changable by the user (i.e. $_SESSION) and $clearance is not mutable, too, then you should be safe.

Answer 4

SQL Injection is a serious risk and you should do whatever you can to defend against it. Even if you think your $user_id comes from session data, you still have to consider the source of the session data. You say it's the database, but how did it get into the database?

Just code defensively. It's very simply and easy to do in this case -- just coerce $user_id to an integer and you can be sure no extra SQL syntax will come along for the ride as you interpolate it into your query.

Also, it's unnecessary to use recursion for your function. Here's an example of doing the same function in a more simple manner:

function authorize($user_id, $clearance) {
  // coerce to integer to defend against SQL Injection
  $user_id = (int) $user_id;

  $sql = "SELECT status FROM users WHERE id = {$user_id}";
  $userData = mysql_fetch_array(Query($sql));

  $tally = 0;
  foreach ((array) $clearance as $userStatus) {
    if (is_numeric($userStatus)) {
      $tally += ($userData["level"] <= $userStatus);
    } else {
      $tally += ($userData["status"] == $userStatus);
    }
  }

  return $tally;
}

The only thing this simpler code doesn't support is nested arrays in $clearance. But do you really need to support that?

PS: I also recommend you switch to PDO. It's easy to use and supports SQL queries with parameters, which is an even better defense against SQL injection. For example:

    $sql = "SELECT status FROM users WHERE id = ?";
    $stmt = $pdo->prepare($sql);
    $result = $stmt->execute(array($user_id));
    $userData = $stmt->fetch();

Answer 5

Even if you know 100% where $id is coming from right now, it's not safe to assume that that will always be the case. What if your application grows? What if you have more people working on it? What if they call this function with different values beside the $_SESSION values? Sure you might know the ins and outs of your app and you know that that might never happen but it's still bad practice. At the very least, you can use mysql_real_escape_string.

EDIT It's better to secure things at the last point of entry. Otherwise you leave open doors. If our job was to make sure that absolutely no passengers with bombs entered a plane, where would be the safest, most secure place to verify that? In the parking lot outside, at the airport's front door, or right before the passenger boards the plane?