2
\$\begingroup\$

As we know, using PDO and prepared statements, binding a table or column using bindParam is not possible. So for this, I've figured out to use function parameters to do it by replacing the column names with the parameters, as shown in the example bellow.

function search($filter,$input,$type,$order,$sort,$limit){
    $st=$this->conn->prepare("SELECT Title,Type,Youtube,Score,Ratings,Singer,ID
    FROM song WHERE $filter LIKE CONCAT('%',?,'%')
    AND Type=? ORDER BY $order $sort LIMIT $limit");
    $st->bindparam(1,$input);
    $st->bindparam(2,$type);
    $st->execute();
    return $st->fetchall();
}

And I call this function here:

$result=$search->search($_POST['filter'],$_POST['input'],$_POST['type'],$_POST['order'],$_POST['sort'],$_POST['limit']);

Where the values come from:

  • $filter----select
  • $input----input
  • $type----radio
  • $order----select
  • $sort----radio
  • $limit----input

As you see the table I'm selecting from is fix, that's not optional, but I have the option to choose which column I want the result to be ordered by($order), which column I'd like to search($filter), I want it to be ascending or descending($sort), and how many I'd like to see($limit).

And this code works fine, but I'm not sure if it's a safe way to do this. Are there some kind of risks of this solution, or it's safe to leave it like this?

\$\endgroup\$
3
\$\begingroup\$

Are there any risks of using function parameters for column names in prepared statement?

Well, of course you shouldn't. "Function parameters" is not a protection measure in any sensible way. It's just a transport, to deliver some value into a function. As is.

Are there some kind of risks of this solution,

ANY of them. All kinds of SQL injection are welcome in this code.

Sorry for the harsh preface but I wanted to make it clear and make sure there are no such illusions left.

So you have to change the approach entirely. Luckily, it is not hard to implement.

  • For the field name and order by, you will need a white list filtering.
  • For the limit, as it's not a column name, but a data literal, simply add it through a placeholder.

I've got an article that explains the white list approach in detail, Adding a field name in the ORDER BY clause based on the user's choice

Using a helper function from this article,

function white_list(&$value, $default, $allowed, $message) {
    if (empty($value)) {
        return $default;
    }
    $key = array_search($value, $allowed, true);
    if ($key === false) { 
        throw new InvalidArgumentException($message); 
    }
    return $value;
}

we can make your function 100% safe

function search($filter,$input,$type,$order,$sort,$limit)
{
    $fields = ['name', 'text', 'whatever'];
    $filter = white_list($filter, $fields[0], $fields, "Incorrect filter name");
    $order  = white_list($order, $fields[0], $fields, "Incorrect order name");
    $sort   = white_list($sort, "ASC", ["ASC","DESC"], "Invalid ORDER BY direction");

    $sql = "SELECT Title,Type,Youtube,Score,Ratings,Singer,ID
    FROM song WHERE $filter LIKE CONCAT('%',?,'%')
    AND Type=? ORDER BY $order $sort LIMIT ?"
    $st = $this->conn->prepare($sql);
    $st->execute([$input, $type, $limit]);
    return $st->fetchall();
}

Just edit the list of fields allowed (or make two lists if they have to be different for the filter and order) and you're set.

\$\endgroup\$
  • \$\begingroup\$ I thought the title would make no much sense. English is not my native language, and I didn't really know how to ask it. Couldn't even have asked it properly in my own language, let alone a foreign one. BTW, thanks for the help, I thought this code would be vulnerable to SQL injections, but didn't really know how I can defend against it in this case. TBH, what I found the most interesting in your answer is the way you bind the parameters in the execute method. I had no idea it can be done like this. It definitely looks better, I'll surely use this method in the future! \$\endgroup\$ – K. P. Oct 4 at 12:15
  • \$\begingroup\$ Didn't you see the execute() in my answer then? @K.P. \$\endgroup\$ – mickmackusa Oct 4 at 12:16
  • \$\begingroup\$ @K.P. The phrasing of the title makes a perfect sense, it is understandable and clear. It's the premise you expressed in it (implying that passing a variable as a function parameter would mitigate some risks somehow) is wrong. \$\endgroup\$ – Your Common Sense Oct 4 at 12:18
1
\$\begingroup\$

You should be validating and sanitizing the variables that are not being parameterized.

When there is a strict set of acceptable values, write an array of whitelisted values and check against that.

If input is coming from a text type input or textarea, then it will be harder to validate, and you may have to settle for stripping html or characters which you deem to be out-of-bounds.

You need to be as ruthless as possible without damaging the user experience because people that may attempt to compromise your system will not be pulling any punches.

Some examples:

  1. There is only one sort value worth receiving / using in your query. ASC is just syntactic sugar. Because the form provides a radio input field, you don't need to bother with trimming whitespace.

    $sort = $_POST['sort'] == 'DESC' ? 'DESC' : '';
    
  2. Unless you are accepting the offset and row count as a single value, the limit value MUST be an integer. Since your form provides a text input field for this, it is possible that a well-intended user may accidentally pass whitespace. For this reason, it would be a sensible inclusion to trim the value before validating it.

    $limit = ctype_digit(trim($_POST['limit'])) ? (int)$_POST['limit'] : '';
    
  3. filter and $order come from select input fields. This means you know exactly which values to expect. A lookup array will do the job in a tidy fashion.

    $filterLookup = ['filter1', 'filter2', 'filter3'];
    $filter = in_array($_POST['filter'], $filterLookup) ? $_POST['filter'] : '';
    

    In your project structure, you might even design a single reference point for filter and order column values so that everything stays in sync. You wouldn't want your lookup array to deviate from your form's select options -- that would lead to user frustration.


As for how you treat values that do not fall within the predicted range -- that is up to you. You should inform the user when the request could not be processed due to invalid/missing data and let them know how they can fix up their submission. In some cases, you may wish to simply ignore faulty values and proceed with the query construction with that segment omitted -- again, that is your call.

p.s. Consider these adjustments for the sake of readability (while this largely comes down to personal style, it is generally a good idea to write code that doesn't require horizontal scrolling to read):

$result = $search->search(
    $_POST['filter'],
    $_POST['input'],
    $_POST['type'],
    $_POST['order'],
    $_POST['sort'],
    $_POST['limit']
);

and

function search($filter, $input, $type, $order, $sort, $limit) {
    $stmt = $this->conn->prepare(
        "SELECT Title, Type, Youtube, Score, Ratings, Singer, ID
         FROM song
         WHERE $filter LIKE CONCAT('%', ?, '%')
           AND Type = ?
         ORDER BY $order $sort
         LIMIT $limit"
    );
    $stmt->execute([$input, $type]);
    return $stmt->fetchall();
}

Regarding your comment about sanitizing limit values, I recommend a min-max sandwich after casting the input as an integer... (Demo)

$inputs = ['7', '54', '3', 'foo'];
foreach ($inputs as $input) {
    echo "---\n$input >>> ";
    $sanitized = min(max((int)$input, 5), 50);
    var_dump($sanitized);
}

Output:

---
7 >>> int(7)
---
54 >>> int(50)
---
3 >>> int(5)
---
foo >>> int(5)
\$\endgroup\$
  • \$\begingroup\$ Oh I wanted to include more details about where the values come from, just forgot. The $limit comes from an input field, with a number type, so only number can be entered. There is a minimum value of 5, and maximum one of 50. But since I can still actually type a number other than 5-50, I check if the number is between 5 and 50 with php, like: if($_POST['limit']<5{ $_POST['limit']=5; }.If it's less than 5, I set it to be 5. Same if it's more than 50. \$\endgroup\$ – K. P. Oct 4 at 5:47
  • \$\begingroup\$ The form is not the only way to send data to your receiving file ...even if that is the only way that you've built it. Naughty people will circumvent the constraints of your form. \$\endgroup\$ – mickmackusa Oct 4 at 6:06
  • \$\begingroup\$ I added a limit sanitizing demo \$\endgroup\$ – mickmackusa Oct 4 at 8:45

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.