December 9, 2010 at 1:55 pm
Craig Farrell (12/9/2010)
Gail, sadly, I know. I didn't say it was pretty, just easy to maintain and is 'good enough' against smaller recordsets.
As long as they stay small. I've been cleaning this type of code out of a client's system for over 8 months now. The resultsets were small when the developers wrote the code, far from small now.
Anything "good" requires the headaches that dso is currently trying to deal with. 🙂
Yes and no. There's no need to deal with the conversions to varchar and concatenations into the string that is been done here if sp_executesql is used with parameters. Also removes SQL injection vulnerability
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
December 9, 2010 at 2:00 pm
If you insist on doing it this way, make sure that you check and re-check the varcahr params for malicious code.
See corrections within. You're missing quotes.
dso808 (12/9/2010)
ALTER PROCEDURE [dbo].[PTicketSearch](
@StartDate datetime,
@EndDate datetime,
@Submitter varchar(100),
@Assigned varchar(100),
@Status varchar(10)
)
AS
BEGIN
DECLARE @SQL VARCHAR(MAX)
SET @SQL = 'SELECT * FROM PROBLEMTICKET WHERE 1=1 '
IF (@STARTDATE IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETDATE >= ''' + CONVERT( VARCHAR(20), @StartDate, 101) + ''''
END
IF (@ENDDATE IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETDATE <= ''' + CONVERT( VARCHAR(20), @EndDate, 101) + ''''
END
IF (@SUBMITTER IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETSUBNAME = ''' + @Submitter + ''''
END
IF (@ASSIGNED IS NOT NULL )
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETASSIGNED = ''' + @Assigned + ''''
END
IF (@STATUS IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETSTATUS = ''' + @Status + ''''
END
PRINT @SQL -- So that you can see what it is while debugging.
EXECUTE(@SQL)
END
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
December 9, 2010 at 2:05 pm
dso808 (12/9/2010)
Thanks Craig! That's taken care of now. But when I execute the stored procedure to test like so (just trying to get ALL records from the table):-- exec PTicketSearch '','','','',''
That's not going to get you all of the records. Within the procedure you're checking if the params are NULL to see whether to include them or not. NULL and '' are very different things.
Doing that gets you a query that has all of the filter predicates in, with SQL looking for rows where they are all empty strings (other than the date that will convert implicitly to 1900/01/01 I believe)
To get all records, run this
exec PTicketSearch NULL, NULL, NULL, NULL, NULL
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
December 9, 2010 at 2:20 pm
Thanks Gail! OK so using NULL for the parameters returned all the records which is good. I then put in the double quotes. Now when I entered the date parameters like so:
-- exec PTicketSearch '1/1/10','1/1/12',NULL,NULL,NULL
I get the following error message:
Msg 207, Level 16, State 1, Line 1
Invalid column name '01/01/2010'.
Msg 207, Level 16, State 1, Line 1
Invalid column name '01/01/2012'.
This occurs with double quotes as well. But when I don't use quotes at all, I get the following message:
Msg 102, Level 15, State 1, Line 2
Incorrect syntax near '/'.
I'll keep chipping away and thanks so much for your help! I'm learning quite a bit here thanks to you guys.
December 9, 2010 at 2:24 pm
Gail as usual is more thorough than I am. 🙂
Can you do me a flavor, and the one where it says 'date' isn't a column name, just commment out the 'exec (@SQL)' and copy/paste the print @SQL you've got there? Trying to see if it's complaining at the proc call level or at the exec @SQL level.
Never stop learning, even if it hurts. Ego bruises are practically mandatory as you learn unless you've never risked enough to make a mistake.
For better assistance in answering your questions[/url] | Forum Netiquette
For index/tuning help, follow these directions.[/url] |Tally Tables[/url]
Twitter: @AnyWayDBA
December 9, 2010 at 2:41 pm
dso808 (12/9/2010)
I then put in the double quotes.
Look again, those were not double quotes. They're two single quotes. (two so that the quote is escaped and appears within the string)
Craig Farrell (12/9/2010)
Can you do me a flavor, and the one where it says 'date' isn't a column name, just commment out the 'exec (@SQL)' and copy/paste the print @SQL you've got there? Trying to see if it's complaining at the proc call level or at the exec @SQL level.
It's complaining because dso has used double quotes (") instead of two single quotes (') and quoted identifiers is likely on, meaning that "01/01/2010" is interpreted as a column name, whereas it's supposed to be '01/01/2010' which is interpreted as a string literal.
p.s. What flavour would you like Craig? Peppermint mocha perhaps? 😉
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
December 9, 2010 at 3:28 pm
GilaMonster (12/9/2010)
It's complaining because dso has used double quotes (") instead of two single quotes (') and quoted identifiers is likely on, meaning that "01/01/2010" is interpreted as a column name, whereas it's supposed to be '01/01/2010' which is interpreted as a string literal.p.s. What flavour would you like Craig? Peppermint mocha perhaps? 😉
I had figured, but that's why I wanted to see the @SQL output. 😛 Of course, I could have simply read his comment, but what confused me was the single apostrophe's around the column names in the error.
As to flavors, I'll go with anything that's NOT that Domino's bread bowl I just ate. Meh, won't repeat that error. Hm, I think I'll try some Earl Grey tea... that should get a good rinse going.
Never stop learning, even if it hurts. Ego bruises are practically mandatory as you learn unless you've never risked enough to make a mistake.
For better assistance in answering your questions[/url] | Forum Netiquette
For index/tuning help, follow these directions.[/url] |Tally Tables[/url]
Twitter: @AnyWayDBA
December 9, 2010 at 3:52 pm
Hi Craig aand thanks again. OK, so I commented out 'execute(@SQL)' and put in 'print(@SQL)'. I got the following message:
SELECT * FROM PROBLEMTICKET WHERE 1=1 AND PROBTICKETDATE >= "01/01/2010" AND PROBTICKETDATE <= "01/01/2011"
That's all that was returned. Interesting - what do you think?
December 9, 2010 at 3:54 pm
dso808 (12/9/2010)
That's all that was returned. Interesting - what do you think?
Read my last post.
I do not recommend this approach. Take a look at the method on my blog (link previously). No messy quotes, no concatenating variables into strings, no SQL Injection vulnerability.
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
December 9, 2010 at 3:55 pm
ARGGHH! Sorry about that Gail! I meant single quotes... Thanks again!
December 9, 2010 at 3:58 pm
GilaMonster (12/9/2010)
dso808 (12/9/2010)
That's all that was returned. Interesting - what do you think?Read my last post.
I do not recommend this approach. Take a look at the method on my blog (link previously). No messy quotes, no concatenating variables into strings, no SQL Injection vulnerability.
DSO, when you get a chance, Gail is right. If you're just starting and going to learn a new way to do this, you might as well go whole hog and learn the right way... and her blog is one of the best descriptions of how to do that. I tend to take shortcuts depending on the scenario, which is... naughty of me.
Try to convert this after you get your boss happy into that method for more stability, less problems, and to remove one massive security hole. If you hit a single more issue along the way, I would recommend just immediately switching gears to the parameterized method.
Never stop learning, even if it hurts. Ego bruises are practically mandatory as you learn unless you've never risked enough to make a mistake.
For better assistance in answering your questions[/url] | Forum Netiquette
For index/tuning help, follow these directions.[/url] |Tally Tables[/url]
Twitter: @AnyWayDBA
December 9, 2010 at 3:58 pm
That command you just posted had double quotes around the string (I checked, it's the character "). That's not what I put into the procedure I fixed for you earlier. I put two ' characters, not one ". SQL interprets "abc" as meaning a column/table name, 'abc' as a string.
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
December 9, 2010 at 4:21 pm
Gail - I am SO sorry. Found the problem in the procedure. Like you mentioned, I MESSED UP the quotes big time. I used a combination of single/doouble which was flatout WRONG. I corrected it by just using all singles. Here's the final product:
ALTER PROCEDURE [dbo].[PTicketSearch]
(
@StartDate datetime,
@EndDate datetime,
@Submitter varchar(100),
@Assigned varchar(100),
@Status varchar(10)
)
AS
BEGIN
DECLARE @SQL VARCHAR(MAX)
SET @SQL = 'SELECT * FROM PROBLEMTICKET WHERE 1=1 '
IF (@STARTDATE IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETDATE >= ''' + CONVERT( VARCHAR(20), @StartDate, 101) + ''''
END
IF (@ENDDATE IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETDATE <= ''' + CONVERT( VARCHAR(20), @EndDate, 101) + ''''
END
IF (@SUBMITTER IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETSUBNAME = ''' + @Submitter + ''''
END
IF (@ASSIGNED IS NOT NULL )
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETASSIGNED = ''' + @Assigned + ''''
END
IF (@STATUS IS NOT NULL)
BEGIN
SET @SQL = @SQL + ' AND PROBTICKETSTATUS = ''' + @Status + ''''
END
EXECUTE(@SQL)
END
Then, I'd execute the procedure like so:
-- exec PTicketSearch '1/1/10','1/1/11',NULL,NULL,NULL
Records are returned, regardless if I use single or double quotes in the parameters (thats where I got confused with your input - I thought you meant in the execute statement, not the procedure itself - sorry again :-)). Been playing around with this procedure and it seems to be doing what we need it to do...
But I'm going to go over your blog post with a fine-toothed comb and figure out if this is really the best way for us to go. But this is working now and I've learned quite a bit thanks to you and Craig. Thanks again guys for all your help. Have a great weekend!
December 9, 2010 at 8:12 pm
Hi Gila,
Yes and no. There's no need to deal with the conversions to varchar and concatenations into the string that is been done here if sp_executesql is used with parameters. Also removes SQL injection vulnerability
In this method is there an indirect way of SQL injection? this is just to make myself clear if my understading is wrong.
Thanks & Regards,
MC
December 10, 2010 at 12:00 am
In which method? The one on my blog or the one shown here?
The one on my blog, no. http://sqlinthewild.co.za/index.php/2009/04/03/dynamic-sql-and-sql-injection/
The one shown here, absolutely yes.
exec PTicketSearch NULL,NULL, '''; DROP TABLE ProblemTicket; -- ' ,NULL,NULL
will produce, as the SQL to be executed
SELECT * FROM PROBLEMTICKET WHERE 1=1 AND PROBTICKETSUBNAME = ''; DROP TABLE ProblemTicket; -- ' ,NULL,NULL
I leave it as an exercise to the reader to see what will happen if that runs (and permissions are loose enough)
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
Viewing 15 posts - 16 through 30 (of 57 total)
You must be logged in to reply to this topic. Login to reply