10

I have an SQL query like "select * from records where record like '%" + user_input + "%'" My goal here is to get all the records. So far everything I have tried involves using comments to bypass the whitespace filter, but with / and - disabled that did not work.

Does this mean my SQL query is safe? Is there any way someone can break my query and view all the records?

schroeder
  • 129,372
  • 55
  • 299
  • 340
  • 7
    What do you mean by "with / and - disabled"? – Bergi Jul 18 '21 at 14:20
  • 43
    Does it even matter if it's injectable or not? The proper solution is so easy and simple, I don't see why it's worth chancing it on the hope that you covered all your bases – Alexander Jul 18 '21 at 22:14
  • 64
    Why oh god why do people insist on trying to be clever. Use prepared statements. – Gregory Currie Jul 19 '21 at 04:59
  • 5
    The only reason NOT to use parameterized queries is your reluctance to learn how to do it. I guarantee you it will be worth it. – Andy Lester Jul 19 '21 at 19:55
  • I’m voting to close this question because it asks for the solution to an ongoing CTF challenge. –  Jul 21 '21 at 13:33
  • 1
    Seriously, why even do this :/. The LIT organizers (specifically Eyangch on this challenge) worked so hard to curate a nice and educational sqlite3 injection problem, and you just decide to spoil it for everyone by posting on a public forum? Like what do you even get from this? My disappointment is immeasurable and my day is ruined :< – CodeTiger Jul 20 '21 at 22:43
  • @Aplet123 I am very new to StackExchange so I sadly don't have enough reputation points to comment (minimum of 15 reputation points). Regarding the precedence of "homework" questions, while I see your point, I personally believe this is more serious than "homework question" as it was from an ongoing tournament which explicitly prohibited the usage of external help. Consequently, we are upset since it imposes great unfairness for other participants who worked hard to get this challenge. – CodeTiger Jul 21 '21 at 03:37
  • I know your pain because I have organized multiple CTFs in the past, and seen many problems posed in SO/SE questions. However, there's really not much to be done apart from leaving a comment asking people to refrain from answering until after the competition is over. The moderators have made their stance firm that their job isn't to moderate competition questions, and many people see these questions as very insightful learning experiences (after all, that is the point of writing a challenge, right?). – Aplet123 Jul 21 '21 at 11:11
  • In fact, it's arguably better this way for the future. While the challenge will probably get taken down very soon and its writeups buried under an obscure CTFTime page, you can already see from the reception of this question (10 upvotes, 54 upvotes on the answer, an appearance in the hot network questions), that this will last and help people for years to come. Trying to shame people to delete their question after the CTF finishes (like this person did) doesn't accomplish anything. – Aplet123 Jul 21 '21 at 11:18

3 Answers3

57

Does this mean my SQL query is safe? Is there any way someone can break my query and view all the records?

No, it is not safe. More than being able to view all the records of one table, you can pass in:

"'AND(EXISTS(SELECT(1)FROM"SECRET_TABLE"WHERE((username='Admin')AND(password_hash='0123456789ABCDEF'))))AND"RECORD"LIKE'"

If you get any output then you know that:

  • There is a table called SECRET_TABLE;
  • That table has the columns USERNAME and PASSWORD_HASH; and
  • There is a row where the username is Admin and the password hash is 0123456789ABCDEF.

And the passed in expression does not use the -*|%/ characters or any whitespace and results in a valid SQL expression.

db<>fiddle here

A determined attacker could then use this type of query to pull out data from any table the connected user has access to.

Don't use string concatenation to include user input into queries; use prepared statements with parameters. For example:

"SELECT * FROM   table_name WHERE  RECORD LIKE '%' || ? || '%'"

And pass your dynamic value in as a bind parameter.

MT0
  • 646
  • 4
  • 6
  • 1
    I suppose you can also use SUBSTRING(password_hash,1,1)=='0' – user253751 Jul 18 '21 at 20:31
  • 45
    "... use prepared statements." - I think you mean parameterized queries. People often confuse the two concepts, and proper prepared statements should be parameterized, but it's not the prepared part that makes a query safe. For example, using psycopg2, not prepared but parameterized and thus safe: cursor.execute('SELECT * FROM users WHERE name = %(name)s', {'name': user_input}). Prepared but not properly parameterized and thus unsafe: cursor.prepare('SELECT * FROM users WHERE name = "' + user_input + '"'); cursor.execute(). – marcelm Jul 19 '21 at 07:17
  • 3
    @marcelm parameterized queries are a specific type of prepared statement, most database drivers that allow parameterized statements without a prepare simple pass on the query as if no parameters have been given (aka: just substitute the keys with the supplied values). The prepared part means you send the query and the data in different packages to the database… making it impossible to interpret and data as instructions any more. So you are correct we mean them both. But even owasp mixes the two concepts. https://cheatsheetseries.owasp.org/cheatsheets/Query_Parameterization_Cheat_Sheet.html – LvB Jul 19 '21 at 08:29
  • 2
    @user253751 Tricks like that are don by automatic hacking tools. Instead of looping though everyc haracter, start in the middle of the range and check if it is greater, this allows you to get quicker to the value – Ferrybig Jul 19 '21 at 08:52
  • 5
    Obligatory XKCD comic strip: little "Bobby Tables". This brings tears to my eyes every single time a reread it! – LorenzoDonati4Ukraine-OnStrike Jul 19 '21 at 10:25
  • 1
    @LvD In theory the database wire protocol could allow a packet that consists of a string and separate parameters (not part of string) as a way to support parameterized queries independent of query preparation features. None seem to implement that directly. But SQL Server does support non-prepared parameterized queries via an RPC call packet to the sp_executesql procedure, which takes a parameterized SQL string (batch) as its first parameter, and all remaining parameters are treated as the parameters for executing that string with. Thus SQL Server supports non-prepared parameterized queries. – Kevin Cathcart Jul 19 '21 at 16:38
15

A query itself is not either injectable or not; there's a programming language involved around it, and you should really share how the user input is sanitized before putting it into the query. Moreover, best practice would be using prepared statements instead of sanitation hacks.

Some ideas on this case:

  • What if the user input is completely empty? That is different from any whitespace and results in %%.
  • There's no SQL injection required to get all the records as there's a related defect in the operation logic: it's easy enough to iterate through a..z, A..Z & 0..9 and then remove the duplicates.

Some enhancements:

  • Use prepared statements.
  • Instead of blacklisting characters whitelist the expected characters.
  • Reject empty user input or require a minimum length for it.
  • Limit the results; use SQL TOP, LIMIT, FETCH FIRST or ROWNUM Clause depending on the SQL variant you are using.
Esa Jokinen
  • 18,957
  • 6
  • 58
  • 61
  • 2
    Also, depending on SQL server and API used to access it, user_input could be something like ';select'haha. Your advice is good for security (although whitelisting one may turn out to be very hard, especially if one needs to support UTF-8). But OP needs in addition to implement rate limiting and blacklisting offenders separately, otherwise (as you noted) people can iterate and get all the results (just a little slower if minimum input length is enforced). But most likely this is simply a wrong way to do whatever OP needs to do - OP is better to ask what they need to accomplish. – Matija Nalis Jul 18 '21 at 16:48
  • 4
    Except for the first proposal (using prepared statements) none of the other enhancements will do very much against a determined attacker except if you had incredible resources available (whitelisting characters is tremendously difficult given the intricacies of sql). – Voo Jul 18 '21 at 21:31
  • 1
    And even prepared statements can be no better than concatenation if the concatenation is well tested using the DB client library's value escaping function while your routine to build a prepared statement with a dynamic number of parameters has a defect causing it to put the parameter values out of order with respect to the placeholders. One might use a dynamic parameter count to add AND/OR conditions or to work around lack of support an array-valued parameter for operator IN. – Damian Yerrick Jul 18 '21 at 21:38
  • 2
    @Voo, whitelisting can work if you restrict the allowed characters sufficiently. For example, I'm not aware of any opportunities for injection if the allowed characters are restricted to the ASCII digits 0-9. – Mark Jul 19 '21 at 00:55
2

Like other answers and comments, I agree that you should parameterized queries and prepared statements. A modification to your question (I'll get to why later) is:

No -*|%/ and no spaces, is this SQL injectable?

In this case, you could just replace all spaces with either tabs or newlines, as they function identically to spaces for this case in sqlite3.

Now to why I modified the question: Impressive idea! So impressive that we decided to use it for CTF contest that ended at 11:59 EDT 7/18. Congrats! . (LIT CTF @ https://lit.lhsmathcs.org/, challenge at https://ctftime.org/task/16542)

eyangch
  • 21
  • 2