Can This Code Be Exploited for SQL Injection with Significant Impact?

Question

I’m currently debating 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(); 
user.Username = rs.getString(2); 
... 
// Character-wise comparison of users 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(); 
settings.DarkMode = rs.getBoolean(2); 
...
database.CommitTransaction();
// catch block here

In reality, this code looks quite bad. I wanted to prove to my colleague that it’s vulnerable to SQL injection, and I managed to exploit it using UNION ALL in the first query to retrieve a user with a custom password like this ' UNION SELECT TOP 1 UserID, Username, 'MyCustomPassword' AS Passwort, ... FROM Users --.. However, the second query fails due to a column count mismatch (10 vs. 40 columns), throwing an error when UNION ALL is executed. The result of the second query is not important but the query should not throw. Is there any sql string for userid that would satisfy both queries syntax? Is this code really "safe"

I also tried to insert a user into the table by concatenating a SELECT with an INSERT statement, but since CommitTransaction is never called, nothing actually happens. So while this code is indeed vulnerable to SQL injection, the transaction wrapping prevents any serious impact. This feels somewhat absurd—am I missing something?"

EDIT: The userId is coming from the client and the transaction is rollbacked in the catch block therefore my initial approach with insert did not seem to work since the outer transaction is rollbacked when the second query is run (throws error) and therefore the transaction within the injected sql, or am I missing something?

• Martin Smith: If you can run arbitrary inserts there is nothing stopping you appending arbitrary COMMIT to commit the transaction too
• Jonathan Fite: Yes, it can. But are users allowed to supply the value of UserID or is it always from a separate result set. What if you added "; commit transaction" to the end of your injection? Also if you have linked servers, etc. etc. So the injection is technically possible, but only if it uses actual user text to build the string. Still bad practice, use parameters.
• Lennart - Slava Ukraini: userId = "apa' or ''='" => select * from users where userid='apa' or ''='' The second query will also be a valid sql query
• D.Dave: @MartinSmith I edited my question
• D.Dave: @Lennart-SlavaUkraini thanks for the comment! but I would need to know the user id which I initially do not know know except I look in the database but I know the existing columns but not more. I also have to use a custom password set by me so that the password comparision works in other for me to get to the next query

Answer 1

The code in your example precisely matches the definition of SQL injection:

insertion or "injection" of a SQL query via the input data from the client to the application

so it's clearly vulnerable and there's nothing more to "prove".

As for "Significant Impact", it depends entirely on how you define "significant". Generally, since it's impossible to say how exactly the vulnerability will be exploited, you can't estimate the significance ahead of time. Simple user data exfiltration can be the end of your business.

If it's a real life application, fix it before it's too late.

If it's a school project, it's probably an "F".

• D.Dave: Yes, it is a university project and i think also its very bad but even though I can debug through the code there is for me no way to actually do something. So I probably lost the bet
• vonPryz: @D.Dave Another exploit: denial of service. Insert junk data to a table in a while 1=1 loop. This will fill up the transaction log, and running out of disk space is going to effectively stop all actual business transactions in the DB.

Answer 2

A short example to demonstrate:

#!/usr/bin/python
userId = "apa' or ''='"
s1 = "select * from users where userid='" + userId + "'"
print(s1)
s2 = "select * from settings where userid='" + userId + "'"
print(s2)
-- ddl
create table users (userid int);
insert into users (userid) values (1),(2);
create table settings (userid int, whatever int);
insert into settings (userid, whatever) values (1,3),(2,0);

The python program will print:

select * from users where userid='apa' or ''=''
select * from settings where userid='apa' or ''=''

If I execute these strings:

db2 "select * from users where userid='apa' or ''=''"
USERID 
-----------
 1
 2
 2 record(s) selected.
db2 "select * from settings where userid='apa' or ''=''"
USERID WHATEVER 
----------- -----------
 1 3
 2 0
 2 record(s) selected.

• D.Dave: Hey, thanks. I run this query to my actual database it works but I need also to set the password the reason is as explained in the comment section. I want to also send the password I set for the user so that I dont get an unauthorized to get to the second query. So this worked great and I could set the password: ' UNION SELECT TOP 1 UserID, Username, 'MyCustomPassword' AS Passwort, ... FROM Users -- but the problem was it failed the second query. So i guess there is no practical impact
• vonPryz: @D.Dave There already is a very practical impact: data leakage. Attacker can get a list of all valid users from the database. Then they can look for accounts that are listed in, say, have been pwnd. Also, injection is #3 in OWASP-10, so one really should not allow it, even if you think there's no pracitcal use for it here.