Anti SQL Injection Function

Question

Anti SQL Injection Function

JohnG69

Hall of Fame

Points: 3114
More actions
December 14, 2015 at 7:11 am

#99018

I have a Stored procedure that has many parameters (all varchar), some parameters are used in where clauses and others are used as table names. I have read many articles suggesting to use quotename for table names, escape single quotes, % and _ and white listing or black listing words or characters.
In my case, the quotename won't work as adding the [] characters, breaks my procedure, since I use sys.tables where name = @tablename to validate and get the columns from sys.columns.
So I have decided to create a generic function to validate the parameters to avoid SQL injections... this simple function takes an @input parameter and returns a modified version of the @input variable (if unwanted characters are found). this function uses the replace command when the following characters (or words) are found:
' -- /* */ % ; + [ UNION DROP CREATE INSERT SELECT UPDATE UNION
I can't get rid of the dynamic SQL... My question is can someone find a way to get through this function and still cause damage by injecting unwanted SQL?
here are some examples of code using the parameters:
SET @SQLStr = ' SELECT c.name, CASE WHEN y.name IS NOT NULL THEN 1 ELSE 0 END AS Set_Columns
INTO TMP_Col
FROM sys.tables t
JOIN sys.columns c on c.object_id = t.object_id
JOIN (SELECT c.name
FROM sys.tables t
JOIN sys.columns c ON c.object_id = t.object_id
WHERE t.name = ''' + @tar_local + ''') x ON x.name = c.name
LEFT JOIN (SELECT c.name
FROM sys.tables t
JOIN sys.columns c ON c.object_id = t.object_id
WHERE t.name = ''' + @tar_local + ''') y ON y.name = c.name and y.name in (''' + CASE WHEN @set_local IS NULL THEN 'No Set Parameter' ELSE REPLACE(@set_local,';',''',''') END + ''')
WHERE t.name = ''TMP_9999999'''
EXEC (@SQLStr)
;
SET @SQLStr = 'INSERT INTO TMP_PK_TAB_9999999 (TABLE_QUALIFIER, TABLE_OWNER, TABLE_NAME, COLUMN_NAME,KEY_SEQ, PK_NAME)
EXEC sp_pkeys ''' + @tar_local + ''''
EXEC (@SQLStr)
; and more complicated -> params are @tar_local, @src_local, @key_local... the other variables are built strings within the SP which I'm not worried about.
SET @SQLStr = 'INSERT INTO ' + @tar_local + '(' + @ColumnsStr + ') SELECT S.' + REPLACE(@ColumnsStr,',',',S.') + ' FROM (' + @src_local + ') S LEFT JOIN ' + @tar_local + ' T ON T.' + REPLACE(REPLACE(REPLACE(@key_local,' ',''),'=:','= S.'),'AND',' AND T.') +
' WHERE T.' + SUBSTRING(@ColumnsStr,PATINDEX('%F%',@ColumnsStr), CASE WHEN PATINDEX('%,%',@ColumnsStr) = 0 THEN LEN(@ColumnsStr)-PATINDEX('%F%',@ColumnsStr) ELSE PATINDEX('%,%',@ColumnsStr) - PATINDEX('%F%',@ColumnsStr) END) + ' IS NULL'
EXEC(@SQLStr)
I know this isn't the cleanest way, if you have any other ideas please feel free to share.
thanks in advance.

Viewing 15 posts - 1 through 15 (of 18 total)

You must be logged in to reply to this topic. Login to reply

Jeff Moden SSC Guru Points: 1004323 More actions · Answer 1

I see nothing in your first two scripts that require any dynamic SQL whatsoever.

As for the 3rd script, I don't know why such a bit of "Catch All" C.R.U.D. is necessary. That, not withstanding, what is the source of the information for the content of the variables?

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 2

jghali (12/14/2015)
My question is can someone find a way to get through this function and still cause damage by injecting unwanted SQL?

Yes. Pretty easily. Send the injected string as hex.

Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability

We walk in the dark places no others will enter
We stand on the bridge and no one may pass

JohnG69 Hall of Fame Points: 3114 More actions · Answer 3

As Hex? hmmm. So How would I protect against that in a Function that parses and whitelists the parameters?

JohnG69 Hall of Fame Points: 3114 More actions · Answer 4

Hey Jeff,

The parameters are tablenames and columns. Right now, a Delphi software (procedure) has the same parameters and does it all withing the program. They want to move that procedure from Delphi to Stored Procedure with minimal changes. In otherwords, using the same call with the same parameters.

My hands are tied... I don't see how I can avoid dynamic SQL in this case.

When you say, you don't see why dynamic SQL, How do you see it without dynamic SQL?

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 5

Whitelist means you reject anything that you don't explicitly allow. What you were talking about earlier

So I have decided to create a generic function to validate the parameters to avoid SQL injections... this simple function takes an @input parameter and returns a modified version of the @input variable (if unwanted characters are found). this function uses the replace command when the following characters (or words) are found:
' -- /* */ % ; + [ UNION DROP CREATE INSERT SELECT UPDATE UNION

is a blacklist (forbidding certain values).

If you're blacklisting, the hex will get through because there's no keywords that appear. If you're whitelisting (allowing only certain things), then the hex will be rejected every time, because it's not in the list of things you're matching.

Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability

We walk in the dark places no others will enter
We stand on the bridge and no one may pass

JohnG69 Hall of Fame Points: 3114 More actions · Answer 6

understood. But whitelisting is practically impossible as I would have to list all the table names and columns possible and disregarding the fact that there can be unique temporary tables which I don't know what the names can be.

Do you have any suggestion?

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 7

No you don't, you check against the system tables for permanent tables for table and column names. I'd just prohibit temp tables, as those are harder to validate against.

Basically, you either whitelist or you allow for the chance that sooner or later someone will pull off a SQL Injection attack.

Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability

We walk in the dark places no others will enter
We stand on the bridge and no one may pass

JohnG69 Hall of Fame Points: 3114 More actions · Answer 8

You're right. I have to find a way to validate every parameter thoroughly before letting it pass.

I will have to do different functions depending on the type of parameter and use system tables to see if tables and columns exist.

I have one parameter that is really causing me headaches... That parameter makes me cringe?!?!?

That parameter which I didn't want to bring up because I'm ashamed of... is a whole query 🙁

I've tried to have them move away from that but they tell me that it's too big of a change.

I will have to go through every word of the query and whitelist SELECTs, FROMs and make sure that the table is not a system table.

-- Really ugly

Jeff Moden SSC Guru Points: 1004323 More actions · Answer 9

JohnG69 (12/15/2015)
Hey Jeff,
The parameters are tablenames and columns. Right now, a Delphi software (procedure) has the same parameters and does it all withing the program. They want to move that procedure from Delphi to Stored Procedure with minimal changes. In otherwords, using the same call with the same parameters.
My hands are tied... I don't see how I can avoid dynamic SQL in this case.
When you say, you don't see why dynamic SQL, How do you see it without dynamic SQL?

The first two queries use the values of @tar_local as a string to begin with. Just use normal SQL there. Try it and see.

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

JohnG69 Hall of Fame Points: 3114 More actions · Answer 10

oh yes... you're right...

I removed unnecessary complications from the code the 2 first queries insert into unique temporary tables.

for which the unique number is generated by NEWID().

To avoid processes from stepping on eachother.

I started using #tables but don't remember why I had to change it to use unique tables with new id. I'm looking at the code now.

JohnG69 Hall of Fame Points: 3114 More actions · Answer 11

I think it was due to the simple fact that

OBJECT_ID('#table') doesn't find the #table.

I just found that I can use OBJECT_ID('tempdb.dbo.#table') and that I can safely drop the table within the session without touching other sessions.

...

I started re-writing the procedure and quickly ran into the following problem:

declare @SQLStr varchar(8000)

set @SQLStr = 'Select * into #tmp_table from(' + @Src + ') X '

Exec (@SQLStr)

Select * from #tmp_table

error: invalid object name '#tmp_table'

That's why I had decided to use a generated newID() a part of local table.

since I don't know for sure what the table structure is... otherwise I would have created the table and just used

declare @SQLStr varchar(8000)

Create Table #tmp_table (...)

set @SQLStr = 'Select * from(' + @Src + ') X '

insert into #tmp_table

Exec (@SQLStr)

Lynn Pettis SSC Guru Points: 442467 More actions · Answer 12

Change from a local temp table to a global temp table. I did that in a sandbox database and it worked, just can't post the code from work.

May cause issues so you may want to quickly insert the data into a local temp table and drop the global one.

JohnG69 Hall of Fame Points: 3114 More actions · Answer 13

The only problem with that is that I can have 4 processes doing exactly the same thing for different customers.

The global temp table will be hijacked by another process i.e. while one in inserting data the other one can drop the table or insert his own stuff...

I was trying to avoid that type of problem.

Thanks

John

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 14

JohnG69 (12/15/2015)
That parameter which I didn't want to bring up because I'm ashamed of... is a whole query 🙁

You can't whitelist that without writing an entire SQL parser. Consider aliases. If they want that functionality, then they need to accept the risk that they might find their entire DB up on pastebin, or find it gone because someone injected commands.

Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability

We walk in the dark places no others will enter
We stand on the bridge and no one may pass