1

I have a php application where the function "makeSessionVar" creates a session variable from application defined data, then concatenates it into the SQL string.

function makeSessionVar($A,$B){ // (<id>, <sql column name>); 
    ...
    $_SESSION['col'.$A] = $B;
    ...
}

makeSessionVar("1", "dateModified"); 
makeSessionVar("2", "title");
makeSessionVar("3", "notes");

$editSQL = "UPDATE myTable SET " . $_SESSION['col1'] . " = ?,  " . $_SESSION['col2'] . " = ?,  " . $_SESSION['col3']. " = ? WHERE ID = ?";

My understanding is that concatenation is dangerous for user defined data, but is there a way for an end user to poison these session variables to inject arbitrary code, or is this safe?

Yes, I know that the right way is to do a fully prepared statement it is like this below, but I'm more interested in knowing if it actually matters from a security standpoint.

$editSQL = "UPDATE myTable SET ? = ?,  ? = ?,  ? = ? WHERE ID = ?"
5

Presuming that prepared queries are easy to use and don't cause performance issues here, then you should just use prepared queries. All else being equal, I prefer to do things the secure way by default. In other words, instead of asking "Should I do this the secure way?" (which is what you are asking) try approaching from the opposite perspective: "Is there any reason not to do this the secure way?". Unless you have a good reason to do it without prepared queries, just always use them.

For session variables I think it is especially important to always use prepared queries, because their nature makes it easy to lose track of where they came from. It is true that given your current use case there is no immediate risk of an SQL injection vulnerability (but note that session data and cookie data can be easily confused, and the latter are not secure). However business needs often change and this can generate a large risk when the creation of data is not easily "visible" from where it is used. What happens if on the next iteration of the application one of these pieces of data now comes from user input? You'll probably remember to convert your query into a prepared query when you first store the user input. But will you remember to follow that same data through your entire application and make sure you aren't using it without prepared queries in a later place? If you end up inserting the data into the database securely, but later fetch that data from the database and use it in another insecure query, then you can still end up with an SQL injection vulnerability.

So don't ask "Should I use prepared queries?". Instead start from the other end: "Is there a reason why I shouldn't use prepared queries?". If you don't have a compelling reason to skip the prepared queries, then use them. If you think you have a good reason to do it insecurely, then at least you are at a good starting point for a thorough cost/benefit analysis.

Prepared queries with variable column names

Please note that this syntax is not actually allowed for prepared queries:

$editSQL = "UPDATE myTable SET ? = ?, ? = ?, ? = ? WHERE ID = ?"

Column names cannot be provided with place holders. As a result you actually can't build prepared queries like this. Instead you have to provide the column names as variables into the query building, which means that it can be vulnerable to SQLi vulnerabilities. Therefore, the way you protect against SQLi in cases like this is by whitelisting the variable contents to make sure that it is exactly something that it is supposed to be.

|improve this answer|||||
  • 4
    Yeah, I think the short answer here is "don't slip in to bad practices just because you can." – h4ckNinja Oct 12 '18 at 19:30
  • @h4ckNinja 100% yes. – Conor Mancone Oct 12 '18 at 19:34
  • Ah, hadn't really tested that far ahead. In this case, the "why not" is development time. The goal here is that "makeSessionVar()" should be able to construct my entire CRUD such that all I need to do is fill out a few parameters for each data point, and dump them into a template that can use that to build out the SQL, JS, HTML, etc. In this case, I do not foresee there ever being a reason to accept user input for those fields, but if it were ever added at any point, it would inheirantly be a system admin level permission anyway. – Nosajimiki Oct 12 '18 at 21:22
  • 1
    The other thing that I'm thinking here is that if you are using a framework like Symfony, Zend, Laravel, etc., they make it difficult to not use prepared statements. There's little reason to not use a framework these days. They cut dev time, and give you XSS, SQLi, CSRF, etc. protections for free. – h4ckNinja Oct 13 '18 at 20:03
  • 1
    @Nosajimiki Considering that the very last version of PHP will officially hit EOL and stop receiving security updates in barely 2 months, you have your answer. If you are seriously concerned about security then a rebuild is absolutely critical. Of course most people in your circumstances are not on PHP 5.6, so your version of PHP probably hit EOL a while back... – Conor Mancone Oct 17 '18 at 0:34

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.