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?

Improve this question
9
  • 7
    What do you mean by "with / and - disabled"?
    – Bergi
    Jul 18, 2021 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, 2021 at 22:14
  • 64
    Why oh god why do people insist on trying to be clever. Use prepared statements.
    – Gregory Currie
    Jul 19, 2021 at 4: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, 2021 at 19:55
  • 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, 2021 at 22:43

3 Answers 3

Reset to default
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.

Improve this answer
6
  • 1
    I suppose you can also use SUBSTRING(password_hash,1,1)=='0'
    – user253751
    Jul 18, 2021 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, 2021 at 7: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. cheatsheetseries.owasp.org/cheatsheets/…
    – LvB
    Jul 19, 2021 at 8: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, 2021 at 8: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, 2021 at 10:25
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.
Improve this answer
4
  • 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, 2021 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, 2021 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, 2021 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, 2021 at 0: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)

Improve this answer

You must log in to answer this question.