Please Help Prevent SQL injection
Hi All,
Our old C# API dynamically generates SQL, and does the CRUD operations for several large software products.
A recent PEN test identified SQL injection is (easily) possible for one of the READ operations. It is called from hundreds of PHP files.
This part of the API was designed to allow developers to define the filters on a query. PHP developers have exposed this to authenticated users.
Can anyone suggest a way to validate the user input & ensure unsafe SQL injection can't occur?
- An example query received from the user might look like:
name = 'john' and title = 'MR' and (select top 1 group_id from groups where type = 1 and region = 'AUD') = 5
- I do have the list of valid columns that a user can access, so I can string match against it. Including valid dynamic columns as per the example above.
- I was thinking about splitting the string, then matching columns, however, I'm not confident with this. Perhaps there is a library that can convert SQL where clauses into a list of parameters ---- I can wish can't I?!?
Any help or suggestions would be greatly appreciated.
18 Replies
Unknown User•6mo ago
Message Not Public
Sign In & Join Server To View
The best way to prevent SQL injection is to use prepared statements instead of concatenating strings around
Unknown User•6mo ago
Message Not Public
Sign In & Join Server To View
Table names cannot be parametrized, alas, but you could at least validate that the name of the table is that of an actual table
Unknown User•6mo ago
Message Not Public
Sign In & Join Server To View
My condolences that you have to maintain this garbage, though
Thoughts and prayers
Unknown User•6mo ago
Message Not Public
Sign In & Join Server To View
Thank you.
Do you have any suggestions that would allow us to use the input string, in the format it is currently provided? -- If possible, this will save 100s of hours of reword on the PHP side.
Unknown User•6mo ago
Message Not Public
Sign In & Join Server To View
Ok.
This is an example of the filter we receive, as a string. Not individual parameters.
tbl_coll.filter = "name = 'john' and title = 'MR' and (select top 1 group_id from groups where type = 1 and region = 'AUD') = 5"
This only applies to the where clause. Everything else is parameterize as you have suggested.
The Columns, Table, joins etc cannot be changed by the developer.
tbl_coll.filter is effectively our where clause. PHP developers call this API to read and write from the database. -- C# was many times faster at this. Lots of code has been written by a lot of PHP devs. If we have to parameterize, we can do it, I think it will just require them to do a lot of rework. If possible, it would be great if this C# API could continue to accept the string we already receive and then validate the columns and ensure it is safe.
tbl_coll.filter is effectively our where clause. PHP developers call this API to read and write from the database. -- C# was many times faster at this. Lots of code has been written by a lot of PHP devs. If we have to parameterize, we can do it, I think it will just require them to do a lot of rework. If possible, it would be great if this C# API could continue to accept the string we already receive and then validate the columns and ensure it is safe.
This C# API gets a fragment of an SQL query as a string...?
:NotLikeThis:
Yes - that's how it currently works.
Well, if it can be any arbitrary SQL, then it can be any arbitrary SQL
You can't exactly distinguish between a where over select, and a bobby tables
Yep. 😩
Well, my personal advice would be to burn this trash heap to the ground and write it from scratch, but good this time
But I assume that's not an option
Hmmmm, not really an option. It would piss lots of people off!
My horrible plan at the moment, is to load the list of valid columns (including dynamic ones)
Try and find them individually in the string.
Split the string into these known columns and values.
then parameterize the value to the know clean column.
I'm hoping each value can then be analysed individually for SQL injection.
Probably the least bad option, yeah
Ok, thank you so much for the input. I'll give it a crack.