Possibility of SQL Injection

Question

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:

<h4 class="text-center"><strong>Filter</strong></h4>

        <div class="panel-group" id="filter" role="tablist" aria-multiselectable="true">
          <div class="panel panel-default">
            <div class="panel-heading" role="tab" id="filterByRace">
              <h4 class="panel-title text-center">
                <a role="button" data-toggle="collapse" data-parent="#filter" href="#filterByRaceBody" aria-expanded="true" aria-controls="filterByRaceBody">
                  Filter By Race
                </a>
              </h4>
            </div>
            <div id="filterByRaceBody" class="panel-collapse collapse" role="tabpanel" aria-labelledby="filterByRace">
              <ul class="list-group">
                <li class="list-group-item">
                  <label for="filterByRaceAmarr"><input type="checkbox" name="race[Amarr]" id="filterByRaceAmarr"/> Amarr</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByRaceCaldari"><input type="checkbox" name="race[Caldari]" id="filterByRaceCaldari"/> Caldari</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByRaceGallente"><input type="checkbox" name="race[Gallente]" id="filterByRaceGallente"/> Gallente</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByRaceMinmatar"><input type="checkbox" name="race[Minmatar]" id="filterByRaceMinmatar"/> Minmatar</label>
                </li>
              </ul>
            </div>
          </div>
          <div class="panel panel-default">
            <div class="panel-heading"  role="tab" id="filterBySP">
              <h4 class="panel-title text-center">
                <a role="button" data-toggle="collapse" data-parent="#filter" href="#filterBySPBody" aria-expanded="true" aria-controls="filterBySPBody">
                  Filter By Skill Points
                </a>
              </h4>
            </div>
            <div id="filterBySPBody" class="panel-collapse collapse" role="tabpanel" aria-labelledby="filterBySP">
              <ul class="list-group">
                <li class="list-group-item">
                  <label for="filterBySP1"><input type="radio" name="sp" value="0:1"/> >1M</label>
                </li>
                <li class="list-group-item">
                  <label for="filterBySP2"><input type="radio" name="sp" value="1:10"/> 1M - 10M</label>
                </li>
                <li class="list-group-item">
                  <label for="filterBySP3"><input type="radio" name="sp" value="10:50"/> 10M - 50M</label>
                </li>
                <li class="list-group-item">
                  <label for="filterBySP4"><input type="radio" name="sp" value="50:100"/> 50M - 100M</label>
                </li>
                <li class="list-group-item">
                  <label for="filterBySP5"><input type="radio" name="sp" value="100:999"/> <100M</label>
                </li>
              </ul>
            </div>
          </div>
          <div class="panel panel-default">
            <div class="panel-heading" role="tab" id="filterByISK">
              <h4 class="panel-title text-center">
                <a role="button" data-toggle="collapse" data-parent="#filter" href="#filterByISKBody" aria-expanded="true" aria-controls="filterByISKBody">
                  Filter By Starting Bid
                </a>
              </h4>
            </div>
            <div id="filterByISKBody" class="panel-collapse collapse" role="tabpanel" aria-labelledby="filterByISK">
              <ul class="list-group">
                <li class="list-group-item">
                  <label for="filterByISK1"><input type="radio" name="sBid" value="0:1"/> >1B</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByISK2"><input type="radio" name="sBid" value="1:10"/> 1B - 10B</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByISK3"><input type="radio" name="sBid" value="10:50"/> 10B - 50B</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByISK4"><input type="radio" name="sBid" value="50:100"/> 50B - 100B</label>
                </li>
                <li class="list-group-item">
                  <label for="filterByISK5"><input type="radio" name="sBid" value="100:999"/> <100B</label>
                </li>
              </ul>
            </div>
          </div>
          <div class="panel panel-default">
            <div class="panel-heading" role="tab" id="filterBySkills">
              <h4 class="panel-title text-center">
                <a role="button" data-toggle="collapse" data-parent="#filter" href="#filterBySkillsBody" aria-expanded="true" aria-controls="filterBySkillsBody">
                  Filter By Skills
                </a>
              </h4>
            </div>
            <div id="filterBySkillsBody" class="panel-collapse collapse" role="tabpanel" aria-labelledby="filterBySkills">
              <div class="panel-body">
                <p>This filter need more room to be displayed than this sidebar can be offer. Please click on the button below to display the skills filter.</p>
                <button class="btn btn-primary btn-block" type="button" data-toggle="collapse" data-target="#filterBySkillsHideAway" aria-expanded="false" aria-controls="filterBySkillsHideAway">
                  Display Skills Filter
                </button>
              </div>
            </div>
          </div>
        </div>
        <input type="hidden" name="token" value="<?=Token::generate();?>" />
        <button type="submit" class="btn btn-primary btn-block">Filter Results</button>
      </form>

    </div>

Finally the Object of my DB Class that queries the DB:

public function query($sql, $params = array()) {
$this->error = false;

if ($this->query = $this->conn->prepare($sql)) {
  $x = 1;
  if ($params && count($params) > 0) {
    foreach($params as $param) {
      $this->query->bindValue($x, $param);
      $x++;
    }
  }
  if ($this->query->execute()) {
    $breakItUp = explode(' ',$sql);
    if ($breakItUp[0] === "SELECT") {
      $this->results = $this->query->fetchAll(PDO::FETCH_OBJ);  
    }
    if ($breakItUp[0] === "INSERT") {
      $this->lastInsertId = $this->conn->lastInsertId();  
    }

    $this->count = $this->query->rowCount();
  } else {
    $this->error = true;
  }
}
return $this;
}

I am not requesting review on the DB Object or the HTML Form. Those are fine. I included them for background more than anything. My question is this:

Am I opening myself up to SQL Injection with the current way I am building the SQL? I am not sure since the all of the parameters are static values. The only problem I can see is if the user uses Chrome or Firefox to change the form values and then submits the form. That may be a way to inject some SQL, but I am not sure. I have no experience with SQL Injection nor how to thwart it.

Please update the title with the purpose of the code instead of a specific issue with it. — ferada, Feb 24 at 17:52
While you edit the title to tell us what your code does, feel free to fix that stray closing brace in the last code block. — Mat's Mug♦, Feb 24 at 17:58
If you have an attacker, the second thing they'll do is try and fuzz the HTML forms - if you already know that's an issue than that's the answer to the injection part. — ferada, Feb 24 at 18:06

tim · Accepted Answer · 2016-02-24 18:45:51Z

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.

Hey Tim, Thanks for your feedback. A few questions to follow up: - What else do you call a table that contains ads for a site that will be promoting or selling something? - Input::get($string) returns $_POST variables and their values are controlled by the forum that was included in my OP. So my question now is, can those value be changed prior to form submission with Chrome Developer or Firebug. — Dave, Feb 24 at 23:06
@Dave if you right click on a field in chrome and take inspect, you can see the form code and change any value you like, you always have to treat any user submitted values with care. — bumperbox, Feb 25 at 1:22
@Dave Yes, POST and GET are 100% user controlled (as are COOKIE, FILES, REQUEST, and parts of SERVER). They can always be whatever a user wants, and cannot be trusted, ever. An attacker could use firebug or similar, but will likely just use an interception proxy or something like wget. And sorry, I somehow completely missed that this was about ads. Because of words like charName, race, SP, characterSet, etc I assumed that it had something to do with role playing. ads is fine. An alternative - to avoid any confusion - may be advertisements. — tim, Feb 25 at 8:31
@Dave btw, get isn't such a great name for a method that returns POST values. post would fit better (because otherwise, what would you call the method that returns GET values?). getPostValue would be even better as it's clearer, but it is too long for my taste (for a method used so often). — tim, Feb 25 at 8:33
@bumperbox, yup I tested that out literally right after I submitted that comment. I just forgot to come back and modify it though. Thanks though. — Dave, Feb 26 at 1:09

asked	7 months ago
viewed	65 times
active	7 months ago

current community

your communities

more stack exchange communities

Possibility of SQL Injection

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged php html mysql pdo sql-injection or ask your own question.

Hot Network Questions

current community

your communities

more stack exchange communities

Possibility of SQL Injection

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php html mysql pdo sql-injection or ask your own question.

Related

Hot Network Questions