patternphpMinor

Possibility of SQL Injection

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

sqlinjectionpossibility

Problem

I am requesting a review of a portion of a 600 lines of code. This portion of the code process a filter that is farther down the page. It takes in the parameters and formats them into a query. Now I have included two bits. First the code, and then the portion of my DB class that makes the call to the database.

Code:

if (Token::check(Input::get('token'))) {

      $sql = "SELECT ads.id, ads.charName, ads.charId, ads.adTitle, ads.adDescription, ads.adStartingBid, ads.adCurrentBid FROM ads, characterSheet WHERE ads.charId = characterSheet.characterID AND ads.adPublished = 1";
      $stmt = "";
      if (Input::exists('race')) {
        $race = array_keys(Input::get('race'));
        if (count($race)==1) {
          $stmt .= " AND characterSheet.race = '{$race[0]}'";
        } else {
          $x = 1;
          $stmt .= " AND (";
          foreach ($race as $y) {
            $stmt .= "characterSheet.race = '".$y."'";
            if ($x < count($race)) {
              $stmt .= " OR ";
            }
            $x++;
          }
          $stmt .= ")";
        }

      }
      if (Input::exists('sp')) {
        $sp = explode(":", Input::get('sp'));
        $spMultiplier = Config::get('filter/spMultiplier');
        $lowSP = $sp[0] * $spMultiplier;
        $highSP = $sp[1] * $spMultiplier;
        $stmt .= " AND characterSheet.skillPoints BETWEEN {$lowSP} AND {$highSP}";
      }
      if (Input::exists('sBid')) {
        $sBid = explode(":", Input::get('sBid'));
        $sBidMultiplier = Config::get('filter/iskMultiplier');
        $lowSBid = $sBid[0] * $sBidMultiplier;
        $highSBid = $sBid[1] * $sBidMultiplier;
        $stmt .= " AND ads.adStartingBid BETWEEN {$lowSBid} AND {$highSBid}";
      }
      $sql .= $stmt;

    }

The Filter:

```
Filter

Filter By Race

Amarr

Solution

SQL Injection

Am I opening myself up to SQL Injection with the current way I am building the SQL?

Yes, you are very likely open to SQL injection.

Here are code pieces that look vulnerable:

$race = array_keys(Input::get('race'));  
[...]  
$stmt .= " AND characterSheet.race = '{$race[0]}'";
[...]  
foreach ($race as $y) {  
    $stmt .= "characterSheet.race = '".$y."'";

$race[0] as well as $y are user controlled and inserted directly into a query, opening you up to SQL injection.

These two queries are likely not vulnerable, as the partly user controlled values lowSP and highSP are created via *, but they still do not follow best practice:

$stmt .= " AND characterSheet.skillPoints BETWEEN {$lowSP} AND {$highSP}";
$stmt .= " AND ads.adStartingBid BETWEEN {$lowSBid} AND {$highSBid}";

I am not sure since the all of the parameters are static values.

What do you mean static values? Input::get($string) returns something like $_GET['string'], right? Then it's not static. Just because your HTML says that the values can only be pre-defined strings does not mean that anyone has to actually follow that.

An attacker can send whatever they want to your server. You should never trust user input, at all.

SQL Injection: Solution

You need to use prepared statements whenever you put variables in queries.

It doesn't matter what the variables hold, if you think that the values are probably not user controlled, or only partly user controlled, or may be safe. Because they may actually be safe right now, but if you need to think about it in every query, you will make a mistake eventually. Also, code changes. Variables that are safe right now may not be tomorrow.

Luckily for you, you already have the query function which uses prepared statements, so you should use it.

For example, this:

$stmt .= " AND characterSheet.race = '{$race[0]}'";

would become this:

$stmt .= " AND characterSheet.race = :race";


And then later:

$this->query($stmt, [":race" => $race[0]]);


Misc

Your indentation is inconsistent. Try to use the same amount of spaces everywhere (and don't use 2 spaces, it's not enough; if your code is so nested that you think you need 2 spaces, your code is too nested).

Don't shorten variable names, it makes code hard to read. What's a sp?

Short variable names are almost never good. x and y are acceptable in some situations (eg coordinates), but not the way you are doing it. ads is also very short and unclear, as is s (in sBid).

return early. If you rewrite the first if to if (!Token::check(Input::get('token'))) { return "something"; }` you already reduce your code from 5 levels to 4. You can do the same for the exists check on race and save another level.

Code Snippets

$race = array_keys(Input::get('race'));  
[...]  
$stmt .= " AND characterSheet.race = '{$race[0]}'";
[...]  
foreach ($race as $y) {  
    $stmt .= "characterSheet.race = '".$y."'";

$stmt .= " AND characterSheet.skillPoints BETWEEN {$lowSP} AND {$highSP}";
$stmt .= " AND ads.adStartingBid BETWEEN {$lowSBid} AND {$highSBid}";

$stmt .= " AND characterSheet.race = :race";

$this->query($stmt, [":race" => $race[0]]);

Context

StackExchange Code Review Q#120991, answer score: 4

Revisions (0)

No revisions yet.