Is escaping SQL queries like this safe?

Question

I am currently working on a NodeJS backend script that parses incoming HTTP requests to write to and read from a MySQL database for work. I tried to protect it against SQL injections by using a kind of two-layer protection.

To write to the database the user needs to provide valid JSON. This is how the JSON's keys need to be for the script to write to the DB. If they aren't, the request will be cancelled.

{
    "uuid": "1234-5678-abcd-efgh",
    "product_id": 6,
    "product_extras": "color=red",
    "product_count": 2,
    "buyer_name": "X Y",
    "shipping_address": "XY Street 5"
}

The first layer that checks this input is a very basic blacklist. Here the USER_INPUT variable is the JSON above that has already gone through the validation process.

let BLACKLIST = ["DROP ", "DELETE ", "INSERT ", "UPDATE ", "SELECT ", "WHERE ", "ALTER "];
let dont_execute = false;

for(var i = 0; i < BLACKLIST.length; i++) {   
    if(JSON.stringify(USER_INPUT).toUpperCase().includes(BLACKLIST[i].toUpperCase())) {
        console.log("\x1b[31mreceived blacklisted command - aborting");
        dont_execute = true;
        return false;
    }
}

After that validation, if dont_execute is still false, the second layer will be called and so the query will be escaped and sent just like this:

// setting.sqlconnection.table_name is equal to "orders" in this case. Also this variable can't be changed or specified by the user. It's pulled from a settings.json file

sql.sendQuery("INSERT INTO " + setting.sqlconnection.table_name + " (uuid, productid, orderid, productextras, productcount, buyername, shippingaddress) VALUES (" + sqlconnection.escape(uuid) + ", " + sqlconnection.escape(parseddata.product_id) + ", " + null + ", " + sqlconnection.escape(parseddata.product_extras) + ", " + sqlconnection.escape(parseddata.product_count) + ", " + sqlconnection.escape(parseddata.buyer_name) + ", " + sqlconnection.escape(parseddata.shipping_address) + ")");

I have tried many different injections like this one:

{
    "uuid": "1234-5678-abcd-efgh",
    "product_id": 6,
    "product_extras": "color=red",
    "product_count": 2,
    "buyer_name": "X Y",
    "shipping_address": "');DROP orders;--"
}

And just like I expected, it didn't work because first of all it was cought in the first blacklist layer. But after disabling it just to test things out the whole SQL query was just interpreted like a string as the apostrophe was escaped (' became \'), which is just what I wanted. The query that will be sent with the SQLI-infected JSON will then as far as I know look like this in the end:

INSERT INTO orders (guid, productid, orderid, productextras, productcount, buyername, shippingaddress) VALUES ("1234-5678-abcd-efgh", 6, null, "color=red", 2, "X Y", "\');DROP orders--")

But the problem is, after showing this to my boss he said it wouldn't be safe and it's still vulnerable to SQLI. So my question is if he is right and if so, what I could do to improve it.

Additional information:
I am using the npm package mysql for the database connection
I am using XAMPP with MySQL to host the database locally

I'm sorry if I couldn't provide more information but I'm pretty sure that I am not allowed to post more stuff than this.

If you pass in the variables using a prepared statement with parameters then you don't need to do anything the parameter escaping will be handled for you. — SPlatten, Nov 12 '18 at 10:11
No, it's not safe. Use placeholders/parameters in a query and bind values to them. (Except for table names; those have to be directly in the statement). — Shawn, Nov 12 '18 at 10:12
Okay. I will take a look at parameters. But is importing the table name from a file safe? — Sv443, Nov 12 '18 at 10:13
blacklisting or whiteliting to prevent SQL injections is never safe.. SQL injection have more complex injection vectors which can be used the bypass the filter. Besides like @SPlatten said you need to go prepared statements for this to be 100% sure it's safe against SQL injections. — Raymond Nijland, Nov 12 '18 at 10:32
@Sv443, its up to you what you make a parameter, when you construct your statement put in ? and these are replaced with an expected equal number of parameters in the order they are passed. — SPlatten, Nov 12 '18 at 10:35
@RaymondNijland thanks for the answer. The blacklist is only meant to be a first layer, like a very coarse filter. I am currently looking into parametrized / prepared queries. — Sv443, Nov 12 '18 at 10:37
you should stop working with your filter.. @Sv443 only filter what would be "safe" if you had a SQL parser/lexer that would analyze the generated SQL with a predefined SQL structure to check if something was injected.. — Raymond Nijland, Nov 12 '18 at 10:47

Bill Karwin · Accepted Answer · 2020-11-09T22:41:31.440

The denylist approach is bound to miss some cases. You'd need to study a lot more about how queries are formed, and you should write thorough unit tests for your code so anyone who reviews your code can see which cases you've tested.

The denylist approach is also going to get false positives. It appears that you cannot insert any data that includes the word "DROP" for example. That's going to block some legitimate data values.

Both of these problems are solved by using parameterized queries, as the comments above have suggested. You said you'll "take a look" at using parameters, but you should think of it as the primary solution for protecting against SQL injection, not any sort of optional or advanced usage.

However, parameters are only useful in place of scalar values (strings, numbers, dates). You can't use parameters for table names or column names or other identifiers, or SQL expressions, or SQL keywords. Those cases may be less common, but you've got at least one example in your dynamic table name.

Harmful input doesn't come only from users. It can come from files, from web services, from JSON documents. It can even come from your own database! Any content that has a chance of containing weird characters can cause SQL injection.

SQL injection isn't necessarily malicious. It can just be a mistake that is more likely to cause your SQL query to be invalid, instead of causing a data breach.

The solution to supplement parameterized queries is often allowlisting. For example, if you wonder if the table name from your config file is legitimate, check it against a list of known table names. Some apps keep this as in a constant array. Some apps query the INFORMATION_SCHEMA to get the up-to-date list of tables.

Ever see a table in an e-commerce database named ORDER? That would cause SQL to become confused, because ORDER is a reserved word. You should delimit table names in back-ticks just in case they are SQL reserved words or contain punctuation or whitespace.

Here's an example of adding back-ticks around the table name:

sql.sendQuery("INSERT INTO `" + setting.sqlconnection.table_name + "` (uuid, ...

Thanks for your answer! I have now implemented parameter binding which should be the safest option of all. Also I have reworked the whole blacklist function, because just as you said, it will probably cause unwanted behavior. It is now simply a function that logs a *possible* SQL injection but passes the query to the parameter binding function nonetheless. Also the table is not named "order" but "orders" and the mysql library automatically encases it in backticks. — Sv443, Nov 16 '18 at 13:54

Is escaping SQL queries like this safe?

1 Answers1