0


I am currently developing a Slack app in PHP. I'm trying to make the app as secure as possible, that's for sure. So far, I have done the following:

  1. Verified the requests using signing secrets
  2. Making sure the requests are less than 5min old (thinking of shortening this time)
  3. In the few cases where there is user input, that input has been checked against a whitelist.

Now I'm here asking a question because it has come to the part of using SQL. I am somewhat familiar with basic SQL (INSERT, SELECT, DELETE, UPDATE, etc.) and so far I was simply putting the user input after being checked. I did some research about parametrized queries and I saw the following points:

Pros:

  • Huge improvement in security (since the parameters get sent separately to the query; apparently this allows no chance for SQL injection.
  • Speed increase (since you're only sending the parameters to the query instead of a fully formed query, it is faster).

Cons:

  • If you're only running the query once, it doesn't provide much speed benefit (which applies to my case).
  • Doesn't support dinamically built queries (I'm not sure if this applies to me, but what I'm doing is running a specific query depending on the user who calls my app). Further clarification at the end.

Also, in some of the queries I have been using there are variables which are sent as part of the POST request. For instance, this piece of code:

<?php 
$user_id = $_POST['user_id'];
$sql = "SELECT * FROM table_name WHERE UserID = '". $user_id . "'";
{...} // Rest of code to connect to the database, execute the query and the like
?>

My question is: should I parametrize SQL queries when the variable is not dependant on user input, but it's not fixed?

Clarification:
In my job there are several positions, and what I am developing is an app for is to help the different people find who is available to give them an off day. But for instance, people who are an "officer" can't substitute a "manager", if you understand my example. However, there is an intermediate role, who can be both a "manager" and "officer". The role of each user is determined by higher ups, all I'm doing is storing those roles and showing certain info depending on which role you have.

3
  • It’s unclear if you consider POST user input. Your example query is vulnerable to attack. For consistency and to make no assumptions of security, I’d argue for always using parameterized queries. – Jarrod Christman Dec 30 '20 at 20:33
  • I don't think those cons are true. You could just have different backend logic dependant on user role that runs the appropriate query, all while using parameterized queries. Agreed that your POST example is vulnerable. – multithr3at3d Dec 30 '20 at 23:23
  • @JarrodChristman It comes from Slack, and now that I think about it, you're correct. I'm using the desktop app, but someone using a browser could forge a post request with whatever contents they'd wish – Jonathan Tadeo Leiva Dec 31 '20 at 7:29
2

Use parameterised queries everywhere. If you can put data in a parameter, you should put that data in a parameter, regardless of whether it comes from a trusted location or not. String concatenation should be avoided wherever possible, and you should consider using a security linter or other static analysis tool to check over your codebase for instances where you've got string concatenation in queries. There's a decent list of such tools here.

The code you showed is trivially exploitable. In fact I'd go as far as saying that it's pretty much the textbook example of code that is vulnerable to SQL injection. The POST variable is user input - it comes from their browser, which is untrusted, and it can be changed to contain whatever they want.

However, even if the data isn't coming from an untrusted location, you should still use parameterised queries.

Consider this psuedocode, which you might find in any commerce web app:

<?php
function findProductIdBySKU($sku)
{
     $id = $db->selectValue("SELECT id FROM products WHERE sku = ?", $sku);
     if (is_null($id))
         return -1;
     return $id;
}

/* ... */

$productId = findProductIdBySKU($_GET['sku']);
if ($productId == -1)
{
    showNotFound();
    die();
}
$query = "SELECT name, model, description, price FROM products WHERE id = " + $productId;
$productFields = $db->queryRow($query);

/* ... */

?>

You've got this findProductIdBySKU function which takes a product SKU string, looks it up in the products table using parameterised queries (good!), then checks if the product was found and returns -1 if not. The calling code takes a request parameter ("sku") and passes it to this function. If the return value is -1 it displays and error and exits. If the returned product ID is not -1, i.e. the product was found, then it concatenates that product ID into another SQL query and gets the product data based on that.

This looks safe because the concatenated query only uses data that's already in the database, and ID value is presumably just a number. However, this code is a vulnerability waiting to happen.

Now imagine that the company that runs this site partners with a new supplier, as a reseller. These products are shown on a special page and when a customer buys one of these new products it needs to go through a separate process. The developer decides to make the new products identifiable with a special SKU suffix. The existing checkout code calls findProductBySKU in order to work out what to display, so the developer figures the easy solution is to just modify that function slightly:

function findProductIdBySKU($sku)
{
     $id = $db->selectValue("SELECT id FROM products WHERE sku = ?", $sku);
     if (is_null($id))
     {
         if (str_ends_with($sku, '-RS'))
         {
             return $sku;
         }
         return -1;
     }
     return $id;
}

The checkout code uses this function properly. Everything else continues to work. However, the developer just introduced a vulnerability. Consider what now happens in the original code:

$productId = findProductIdBySKU($_GET['sku']);
if ($productId == -1)
{
    showNotFound();
    die();
}
$query = "SELECT name, model, description, price FROM products WHERE id = " + $productId;
$productFields = $db->queryRow($query);

/* ... */

If the attacker puts -RS on the end of the string they pass into the sku GET parameter, findProductIdBySKU searches (using parameterised queries) for a product matching that SKU in the database, which it doesn't find. The is_null check comes back true, but now instead of just returning -1 the code checks if the string ends with -RS. It does, so the function returns the string that was passed in. The calling code checks if the return product ID is -1. Well, it isn't, because the function returned a string. It now concatenates that string into the query.

An attacker can exploit this by sending a request where the sku parameter contains this:

1 AND 1=0 UNION SELECT username, password, email, is_admin FROM users LIMIT 0,1; -- -RS

When concatenated into the query, you get this:

SELECT name, model, description, price FROM products WHERE id = 1 AND 1=0 UNION SELECT username, password, email, is_admin FROM users LIMIT 0,1; -- -RS

Let's write that out a little more clearly, to better understand how it works:

SELECT name, model, description, price FROM products WHERE id = 1
AND 1=0
UNION SELECT username, password, email, is_admin FROM users
LIMIT 0,1; -- -RS

The first line is obvious: it selects some fields from the products table where the product id is 1. This is what the query would normally do if there was no SQL injection going on.

The second line, AND 1=0, adds a condition to this first part of the query that never evaluates to true. It's asking to find a product in the table where 1 is equal to 0. This means no rows are returned from this part of the query.

The next line is where the fun starts. UNION is used to perform a second query, and append the results of that query directly into the normal results. It also remaps the columns in order, so whatever column you select first from the union part of the query goes into the name column in the results, and whatever you select second goes into the model column in the results, etc.

The injected union query selects the username, password, email, and is_admin columns from the users table. These fields go into the name, model, description, and price fields of the results. Since the first query has no rows, due to the AND 1=0 constraint, the only rows returned will be from the users table!

Finally, there's a LIMIT clause. A limit of 0,1 returns only the first row from the results. By repeating the SQL injection and incrementing the first number (i.e. 1,1, 2,1, etc.) this allows the attacker to get a different row from the users table each time, effectively dumping the whole thing.

All of this is possible because of an oversight. The oversight occurred because the code assumed that a particular piece of data was trusted, and that it would always be trusted. But applications evolve, and the developer making the changes in a year's time likely won't remember (or even know!) about the assumptions that were made when the code was originally written. I've tested thousands of applications over the years and many of the security issues I've found have occurred due to this exact type of oversight. If you write your code to always assume that data is untrusted, wherever possible, and never use dangerous patterns like query concatenation (except where absolutely necessary and with lots of careful input validation) then you're much less likely to introduce vulnerabilities later. Even less so if you run security tooling over your code whenever you make changes (e.g. in a CI/CD environment) and get regular code reviews by security professionals.

3
  • First off, many thanks for your answer. It was amazingly clear for a relative begginer like me. I am changing all my queries to be parametrized now. I just have one more question: pastebin.com/63h8S49G Would this $sqlfinal be ok to send as a parametrized query? Because that was my second "Cons". Otherwise, thanks a lot again – Jonathan Tadeo Leiva Dec 31 '20 at 9:27
  • @JonathanTadeoLeiva You can concatenate query language together, based on whatever logic you need, as long as all the actual data going into the query is parameterised. The code you showed doesn't appear to need it, but if (for example) you had an optional filter by the day, you could have an if statement that checks if the filter is present and, if so, concatenate ` AND Day = :shiftDay` to your query then call mysqli_stmt_bind_param again to bind the day value in as another parameter. – Polynomial Dec 31 '20 at 18:08
  • @JonathanTadeoLeiva Since you're looking into this now, it might be worth taking a look at the OWASP Top 10 project and reading up on the types of vulnerabilities that are listed there. A basic understanding of SQLi, XSS, CSRF, and direct object referencing (missing auth check) issues will help a lot in terms of developing safer applications. – Polynomial Dec 31 '20 at 18:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.