Is this query secured from sql injection?

Question

I Wonder if this sql query is secured from sql-injection, and if it is ok, or i should modify something.

I tried to bind the id from the GET and than if everything is ok, i use that actual query with that id.

if(isset($_GET['id']) && $_GET['id'] != null) {
  $id = $_GET['id'];

  $stmt = $mysqli->prepare('SELECT id FROM maps WHERE id = ?');
  $stmt->bind_param('i', $id);

  $stmt->execute();

  $result = $stmt->get_result();

  if (mysqli_num_rows($result) == 1)    {
    $row = $result->fetch_assoc();
      $secid = $row["id"];
  } else {
      header("LOCATION: index.php");
  }

  $sql = "SELECT 
  maps.id,
  maps.name,
  maps.description,
  maps.date,
  maps.mcversion,
  maps.mapid,
  maps.category,
  maps.format,
  maps.userid,
  users.username,
  users.rank,
  users.verified,
  users.mc_username,
  (SELECT COUNT(*) FROM likes WHERE likes.mapid = maps.id) AS likes,
  (SELECT COUNT(*) FROM downloads WHERE downloads.mapid = maps.id) AS downloads,
  (SELECT COUNT(*) FROM subscribe WHERE subscribe.channelid = maps.userid) AS subscribes,
  (SELECT COUNT(*) FROM views WHERE views.mapid = maps.id) AS views
  FROM maps
  INNER JOIN users 
      ON maps.userid = users.id
  WHERE maps.id = '$secid'";

  $result = mysqli_query($con,$sql);

  if (mysqli_num_rows($result) > 0) {
      $row = mysqli_fetch_assoc($result);
  } else {
      header("LOCATION: index.php");
  }

} else {
    header("LOCATION: index.php");
}

This makes no sense: `SELECT id FROM maps WHERE id = ?` You're selected the exact value that you *already have*. — David, Jul 09 '19 at 16:47
@David It looks like a misguided way of saying "does this thing exist?" It would be better to combine these and `JOIN` them together. — tadman, Jul 09 '19 at 16:48
The concept itself is not safe from an SQL injection, because you're simply taking the value from the table and putting that directly into your SQL string. If the table value itself is no good (say you retrieved a name, like "O'Connell", and went to do a lookup by name). Instead, you should always use prepared statements. You're already doing it with your query to `maps`, so just take your large query string and replace `WHERE maps.id = '$ecid'"`, with `WHERE maps.id=?`. — RToyo, Jul 09 '19 at 16:48
Since you are using a parameterized query for the only value submitted to the code, Yes you will avoid SQL Injection, — Sloan Thrasher, Jul 09 '19 at 16:49
@tadman, not really. Since $secid used in the 2nd query came from the first query, it is sort of already validated. Also, if the first query fails, the $secid hasn't been defined and the 2nd query will fail. — Sloan Thrasher, Jul 09 '19 at 16:51
@SloanThrasher "Sort of already validated" is exactly what people put in post-breach audit reports. It's weak validation and introduces unnecessary risk when the fix is trivial. — tadman, Jul 09 '19 at 16:52
your question is exactly equal to "do i use parameterised statements and *nothing else*?" - which you don't - look at `$secid` — Franz Gleichmann, Jul 09 '19 at 16:53
*Since $secid used in the 2nd query came from the first query* explains why it is called a "*Second Order* SQL injection" — Your Common Sense, Jul 09 '19 at 16:57
I owerthahinked it, please look at my new answer and rate that. I am very thankful for your help, I use stackowerflow for like 2days and I learned a lot of new things. — Dominik Balogh, Jul 09 '19 at 17:37

score 5 · Answer 1 · answered Jul 09 '19 at 16:47

General rule of thumb: If your query has variable interpolation, like it does with $secid, then probably no.

Use prepared statements with placeholder values to be sure you don't have injection problems. Anything else is something you'll need to investigate and verify manually. Carefully and thoroughly.

Since $secid comes from the database you can't be assured what it is. That could be a VARCHAR column, or if it isn't now, it might be in the future. That's what makes SQL injection a perpetual threat. Assumptions that are well-founded today can prove dangerous later on.

In this particular case there's no reason to not use placeholder values. The first query, which is of dubious utility, does. The second can and should use exactly the same approach with ? appearing instead of a value.

As a way of forcing yourself to write safe code, only use single quoted strings when defining queries. This way any accidental SQL injection becomes harmless, you'll instead get literal $ values showing up in your database instead of user data, and these are very easy to spot in testing. SQL injection bugs aren't unless you go looking for them.

score 0 · Accepted Answer · answered Jul 09 '19 at 16:57

This is a better solution right?

if(isset($_GET['id']) && $_GET['id'] != null) {
  $id = $_GET['id'];

  $stmt = $mysqli->prepare("SELECT 
  maps.id,
  maps.name,
  maps.description,
  maps.date,
  maps.mcversion,
  maps.mapid,
  maps.category,
  maps.format,
  maps.userid,
  users.username,
  users.rank,
  users.verified,
  users.mc_username,
  (SELECT COUNT(*) FROM likes WHERE likes.mapid = maps.id) AS likes,
  (SELECT COUNT(*) FROM downloads WHERE downloads.mapid = maps.id) AS downloads,
  (SELECT COUNT(*) FROM subscribe WHERE subscribe.channelid = maps.userid) AS subscribes,
  (SELECT COUNT(*) FROM views WHERE views.mapid = maps.id) AS views
  FROM maps
  INNER JOIN users 
  ON maps.userid = users.id
  WHERE maps.id = ?");

  $stmt->bind_param('i', $id);

  $stmt->execute();

  $result = $stmt->get_result();

  if (mysqli_num_rows($result) == 1)    {
    $row = $result->fetch_assoc();
  } else {
      header("LOCATION: index.php");
      die();
  }

} else {
    header("LOCATION: index.php");
    die();
}

Thank you for your answer! :) – Dominik Balogh Jul 22 '19 at 19:42 — Dominik Balogh, Jul 22 '19 at 19:42

Is this query secured from sql injection?

2 Answers2