spotting sql injection vulnerabilities for dummies

Question

I am a beginner at PHP and SQL. I was asking a question here unrelated to sql injection and user MarcB said that this code was vulnerable to injection.

function create_group($name, $description, $invites){
global $link;
  $name = mysqli_real_escape_string($link, $name);
  $description = mysqli_real_escape_string($link, $description);
  $names = mysqli_query($link, "SELECT `group_name` FROM `groups` WHERE `group_name` = '$name'");
$descriptions = mysqli_query($link, "SELECT `group_description` FROM `groups` WHERE `group_description` = '$description'");
  if(mysqli_num_rows($names) == 0 && mysqli_num_rows($descriptions) == 0) {
    mysqli_query($link, "INSERT INTO `groups` (`group_name`, `group_description`) VALUES ('$name', '$description')");
  } else {
    echo 'Group with that name/description already exists.';
  }

  $result = mysqli_query($link, "SELECT `group_id` FROM `groups` WHERE `group_name` = '$name'");

  foreach($result as $resul) {
    foreach($resul as $resu) {
      $group_id = $resu;
    }
  }
foreach($invites as &$invite) {
  $idres = mysqli_query($link, "SELECT `user_id` FROM `users` WHERE `username` = '$invite'");
    foreach($idres as $idarr) {
      foreach($idarr as $id) {
        mysqli_query($link, "INSERT INTO `group_members` (`group_id`, `user_id`, `confirmed?`) VALUES ('$group_id', '$id', 0)");
      }
    }
  }
echo 'Group created!';
}

I think I am in the (quite large) set of people who are beginners and know what sql injection is but find it very hard to spot vulnerabilities that are not HUGELY obvious (like a username and password being sent purely over a GET request).

I wondered if anyone could offer tips to these people and also help me with this bit of code. Obviously, the latter is less important than the former as I, and many others, will want to know how to spot them ourselves.

Thank you in advance. I hope this is helpful to other people.

Switch everything to use prepared statements/bind variables, and it ceases to be an issue — Mark Baker, Jul 12 '14 at 09:41
@MarkBaker they come from a post request, so I should add a mysqli_real_escape_string; I can't believe I didn't spot that. Also, I'm just going to look into prepared statements and binding variables now (I am a big beginner) — Petrus, Jul 12 '14 at 09:45

score 0 · Accepted Answer · answered Jul 12 '14 at 10:26

Any value that is incorporated in an SQL statement without proper processing may be susceptible as a vector to SQL injection. The means of proper processing depends on where the value is incorporated in the statement and how it is intended to be interpreted within the statement.

You would trace back the path the value takes within the code and see how it’s processed.

Looking at your example code, there are six statements which all have parameters. They are either passed as parameters to the function or originate from database queries.

The first four statements use the variable values $name and $descriptions, which got send through mysqli_real_escape_string and are properly inserted into MySQL string literals. These look fine.

However, the fifth statement uses $invite, which comes from the function argument $invites and is not properly processed. If $invites can be influenced by the user, which is true for the case it originates from a POST request, that statement is susceptible to SQL injection. Applying mysqli_real_escape_string on $invite as well would be a solution.

The last statement, although I suspect that it’s ever reached since you can’t enumerate a MySQL result resource, does also have two variables, which may lack of proper processing in case they are not integer values.

If you’re a developer, you should switch to parameterized statements as prepared statements offer. There you pass the statement and its parameters separately and the database takes care of proper processing for you so you don’t have to worry.

spotting sql injection vulnerabilities for dummies

1 Answers1