4 ms·
Is This Code SQL Injectable with Any Real Impact?
I am currently discussing with a colleague whether the following (pseudo) code is vulnerable to SQL injection (SQL Server):
database.BeginTransaction();
String userId = dto.UserId;
String firstQuery = "select * from users where userid='" + userId + "'";
ResultSet rs = database.executeQuery(firstQuery);
// Use the first entry in the result set as a user with 10 columns
rs.next();
User user = new();
user.Username = rs.getString(2);
...
// Character-wise comparison of user's password with dto.Password, if incorrect, unauthorized is returned here
// Now the second query, which doesn’t need a return but must run successfully
String secondQuery = "select * from settings where userid='" + userId + "'";
ResultSet rs = database.executeQuery(secondQuery);
// Use the first entry in the result set as settings with 40 columns
rs.next();
Settings settings = new();
settings.DarkMode = rs.getBoolean(2);
...
database.CommitTransaction();
// catch block here
This code would look pretty bad in real life. I wanted to show my colleague that it's vulnerable to SQL injection, and I successfully exploited it using `UNION ALL` in the first query to retrieve a user with a self-specified password. However, it fails to compile for the second query due to a column count mismatch, and an error is thrown when the `UNION ALL` is executed.
I also tried to insert a user into the table by concatenating `SELECT` with `INSERT`, but since `CommitTransaction` is never called, it doesn't have any actual effect. So, while the code is SQL injectable, nothing serious can happen because the transaction is wrapped, right? This seems a bit absurd.
- elmerfud 2y agoI would say it kind of depends. It certainly written in bad form as you should almost never do direct variable insertion into your SQL query string. You should should be doing the variable substitution and a prepare execute type method. Either with your database doing it directly or with your libraries that are handling the queries to do it for you. If there is no possible way for an untrusted user to modify your userID, then there is no SQL injection possibility because that is a known set value from a piece of trusted code. If it's untrusted data there should also be additional sanitization around it before it even makes it this far to get concatenated into your query. Still all of that is a multi-layered approach to ensuring your data is properly sanitized. The final bit is proper query substitution. If I saw a piece of code like this it would immediately fail a PR review and anyone who argued that it's fine we would be engaging a wider engineering audience because that person needs a retrained. Additionally the select * is bad form as well. You should be returning the data that you need which is rarely an undefined all. Also for your union approach where it fails due to column mismatches you can address that by simply concatenating your columns together so you return the same number of expected columns in your injected query.
- dharman 2y agoThe userId is coming from the client. I tried this which worked: ' UNION SELECT TOP 1 UserID, Username, 'MyCustomPassword' AS Passwort, ... FROM Users --. I got an user with the defaults of the first entry and my set password. The problem is that the userId from the client is also used in the second query. Here the union wont work anymore because of the column count mismatch. I dont even care about the results of the second query - it just must not throw an error but it seems to be impossible to write a query placed in the userId from the client doing what I did in the first query without affecting the second one so that it throws an error or?
- turtleyacht 2y agoEven if there is no practical impact, there is a maintenance impact. Think of yourself a few years on or a new team member. Some innocent refactoring occurs, and the service is deployed as public-facing. Every time your code is scanned, someone has to consciously ignore it. That can lead to alert fatigue. What if it gets squelched? Now the code has dangerous debt. Mentally, there is toil: this place in the code is fine, but what about others? A conscientious dev will start looking for similar issues. That takes up more time. Suppose someone searches the codebase for SQL examples and finds this one. They think it's an acceptable pattern, even though the search also turns up parameterized queries. Now the bad precedent repeats in the shiny new service, and debt proliferates.