SQL Injection in stored procedure
-
But beware the lurking exception:
If you feed it '1,2,abc,5', the code will fail. Much better than injection, but make sure the code will not cause damage if you feed it bad gumbo.
'Course this would also happen in the original code... (I just finished slogging through a bunch of functions that exploded when you fed it a -1 for a date...)
-
I am trapping any error generated and not worried on that. The only worry is if there is no error and sql injection affect data.
I have the requirement for dynamic query writing. this is just a smaller sample part of code the actual code is more than 10000 lines. I am trying to see the risk associated.
I know dynamic quries are bad. See the better link.
http://msdn.microsoft.com/msdnmag/issues/04/09/SQLInjection/default.aspx
You have to do sometime which is suggested bad but if you can proctect it that what I want.
Thanks
-
Strong willed, you are. Over 600 years, Yoda has re-written much cursor and dynamic SQL code! Much has Yoda seen, and rarely is dynamic SQL the answer. A 1,000 line SELECT statement? A jedi craves not this. Use views, you will. Break it up in parts. Understand boolean logic, you must.
Too flexible, perhaps, your SELECT has become.
Two articles from a paricular weblog, Yoda suggests you also review:
http://weblogs.sqlteam.com/jeffs/archive/2004/10/04/2167.aspx
http://weblogs.sqlteam.com/jeffs/archive/2003/11/14/513.aspx
open your mind. learn the path of the SQL jedi.
-
The logic used is:
getting parameters in csv and converting into table as you said in link. (this is basics now), then maching records based on csv generted table with data tables, apply busineess logic . Put the result set in temp table. dynamically apply filter on temp table for example top xx records , order by somecol, percentchange < or > or = etc (about 10-15 filters). I Worked decades in SQL from SQL's inception.
For reporting either make your code repetative and hard to maintain or use dynamic query, it depend upon the use and intelligence.
No offence Read SQL http://msdn.microsoft.com/library/default.asp?url=/library/en-us/acdata/ac_8_qd_14_2kc3.asp
http://www.managedtime.com/freesqlbook.php3
-
hmm. Books on SQL for Yoda! Clearly demonstrated, you have, to know much more about SQL than Yoda. Lucky I must have been, then, to even understand your question!
Since SQL's inception, just now you have considered injection, hm?
Filtering by first copying ALL rows from a table, and then filtering the results of that... Proud of yourself for this invention you must be, hm? Not the way of the jedi. Too late it may be, to turn you from the dark side!
-
Its clear Yoda don't know and don't want to learn.
God may not be able to help Yada. You need to learn SQL.
-
Dude it's the other way around.
The only time you'll ever be safe from sql injection using dynamic sql is when you do dba tasks (like for each databases in each servers do xyz) on the server that NO ONE other than yourself or other admins will run... but even then you can't be sure you're safe (developpers could use them to get admin access to all servers in that scenario or just delete all databases).
-
Its the requirement of the complex nature of procedure.
If you see problem let me know how. I submitted my proc already. I have read almost every article on internet for sql injection and don't find a place where it is mentioned as harmful.
I know dynamic query is bad and can be attacked but I perticularly want to know if this can be hacked or at risk. I don't buy that change code from dynamic query for the heck of changing even if it is not risky.
Thanks to all anyway.
-
SET @sql = 'SELECT JOB_id, job_desc
FROM JOBS WITH (NOLOCK)
WHERE JOB_id IN (' + @JobIDs + ')' +
'ORDER BY JOB_id'
Execute (@SQL)
consider this
Set @JobIDS = '''; Drop DATABASE master --'
or
Set @JobIDS = '''; exec sp_MSForEachDB 'DROP DATABASE ?' --'
the statement will execute and the db will be lost or all of 'em for that matter.
If you can live with the fact that ANYONE can destroy the server, any database or worse gain access to the data without your consent then please go on with dynamic SQL.
We all wish you the best of luck finding a new job when that happens.
P.S. I don't remember reading anywhere in these posts about speed. But performance will be much worse since the SP will have to be recompiled everytime it is used (can take up to a few seconds if the sp is very complex). Instead of being recompiled only when the ddl or indexed data has changed in the db schema.
-
Thanks, You missed out char(10). see below
create PROCEDURE dbo.usp_GetContactValues
(
@JobIDs nvarchar(4000)
)
AS
SET NOCOUNT ON
DECLARE @sql nvarchar(4000)
SET @sql = 'SELECT JOB_id, job_desc
FROM JOBS WITH (NOLOCK)
WHERE JOB_id IN (' + @JobIDs + ')' + char(10) +
'ORDER BY JOB_id'
print @sql
Execute (@SQL)
GO
Char(10) acutally puts next line. now it gives error near order by. FYI: Errors are already trapped.
-
I already know without char(10) or next line or everything in one line get attacked. Is there way to break when char(10) is there?
The problem in not using dynamic SQL is Complexity and app is already running. I don't want ot mess arround if there is no risk.
-
Got it
Thanks for your help.
-
Ajay, you are very stubborn. Listen to Yoda, he gave you a good solution: the UDF that takes a CSV string and returns a table of integers.
The only solution to avoid SQL Injection is to stop using dynamic SQL or properly validate the input string.
If you use tricks to get an error instead of validating the input, there will always be some way to make an injection. Here is a parameter that will avoid your CHAR(10) trick:
exec dbo.usp_GetContactValues '1) or (1=1) drop table jobs select 1 as job_id where (1=1'
The executed SQL would be:
SELECT JOB_id, job_desc FROM JOBS WITH (NOLOCK) WHERE JOB_id IN (1) or (1=1)
drop table jobs
select 1 as job_id where (1=1) ORDER BY JOB_id
Conclusion: stop trying to find tricks and use the right solution.
Razvan
-
(sadly) For this one, hopeless it is. Not for advice, come here has. To learn something new -- not possible! For him, all is already known.
To Boast of his indestructable sql. Breached by injection, it cannot be, he claims! Most powerful force in the universe, it must be! (he he)
To display his intelligence, his goal was.
Succeeded, he did, hm?
And to not understand, even with his vaunted dynamic SQL, that Yodas advice he can still use, shows his true nature!
If stubborn and lazy you are, if use it still you feel you must, Dynamic SQL can still call a UDF, hm? Best of both worlds, hm?
Too much time insulting Yoda, not enough time using Google, for this one!
-
Thanks for the advice . The app is trapping errors due to multiple result sets. But it can be hacked if proper trapping is missing. I would like to consider solution from Yoda. Thanks Yoda. I have the following scanrio with app running.
There are about 20 lookup tables. Select, insert and update procs are building dynamic query based on table name passed. Dynamic queries are bad and should be avoided.
Now I have these options.
1. Remove dynamic queries and write select, insert and update seperate procs for all the lookup tables (about 20).
2. Keep dynamic query and validate input properly.
3. change structure to merge all lookup tables and add parent table keeping info of table or lookup table type. Whole app will be affected and don't want to do this.
any other option?
What do you suggest?
I would like to thanks all who responded.
Viewing 15 posts - 16 through 30 (of 31 total)
You must be logged in to reply to this topic. Login to reply