January 17, 2017 at 11:13 am
Last week I did a code review for a stored proc, intended for a search screen, which used dynamic SQL. After working with the developer to fix a few issues I approved the code and passed it along to be pushed into production… And that’s when things got interesting…
The DBA responsible for doing the move to production, asked me why I would approve the use of dynamic SQL w/o the use of sys.sp_executesql. The conversation went a little like this…
Him: Why did you approve this code? It’s dynamic SQL and it’s using EXEC(@sql) rather than EXEC sys.sp_executesql.
Me: Because sys.sp_executesql a pita to use and it’s not the only valid method of preventing SQL injection when working with dynamic sql.
Him: But using sys.sp_executesql is “best practices”.
Me: No… Taking steps to prevent SQL injection is best practices. The use of sys.sp_executesql is certainly a valid method of doing that but so is the use of a “sanitizing function” that scrubs any CHAR or VARCHAR parameters of certain characters and replaces single quotes with double single quotes.
Him: Aside for the added syntax, what is your objection using it? What makes it a pain in your backside?
Me: I have 3 requirements for dynamic sql…
1) It must have a “@DeBug BIT” parameter.
2) The @Debug parameter, when set to 1, will cause the proc to generate the query text generated by the dynamic sql.
3) The last section of the comments must contain a sample execution of the procedure, which includes valid parameters values.
The idea is that I want an easy way to see what code is being dynamically generated so that I can evaluate its performance.
Adjusting the code to use sys.sp_executesql makes that painful due to the fact that the DeBug code has variables in place of actual values… Meaning that I have to manually create a DECLARE statement and manually copy/paste the values from the sample in the comments… Or… Require the developer to code it as part of the debugging section (Even I have limits to my cruelty)…
Okay… So I’m sure some of you, if you’ve read this far are, losing you minds… sys.sp_executesql is an absolute must when using dynamic sql… Right? There are more than a few big names, people who I respect and admire who hold this position and I’m open to being convinced that I’m wrong.
So, in that vein, I decided to see if there was a simple way to get what I wanted (easily generated sql code output that can be copy/pasted into its own SSMS tab for evaluation) and use sys.sp_executesql.
I immediately ruled out the use of a function. Functions don’t play well with varying numbers of parameters. A stored proc allows to create a large number of defaulted parameters and then ignore them if you’re happy with the default values…
My first thought was to simply go to sys.parameters and build a DECLARE section from that. It would have worked easily enough but I was concerned that it would be too easy to get the parameter values in the wrong order.
My second thought was to try to mimic sys.sp_executesql itself. After all, if I’m going to the trouble of coding the execution of sys.sp_executesql it would be a simple matter of copy/pasting that to another section and then change the name of the stored proc… Yup… I liked that idea better!
Question: Is it possible?
Answer: Almost…
sys.sp_executesql allows a syntax that can’t be replicated with user defined stored procedures. For example:
EXEC sys.sp_executesql @sql, N’@_P1 INT, @_P2 VARCHAR(50)’, @_P1 = @P1, @_P2 = @P2
<@_P1 = @P1, @_P2 = @P2 is valid syntax for sys.sp_executesql but you can’t create a USP that accepts that kind of input.>
That said, you can use an alternate syntax like this:
EXEC sys.sp_executesql @sql, N’@_P1 INT, @_P2 VARCHAR(50)’, @P1, @P2
Of course, this does open the door to miss-ordered parameter values but I’m thinking that you’d have to actively want to screw that order up after you’ve already defined the parameters in the 2nd section.
Anyway, here’s what I came up with…
ALTER PROCEDURE dbo.PrintSQL
/* ==============================================================================
1/16/2017 JL, Created for Designed to provide dynamic output with parameters similar to sp_executesql
============================================================================== */
@sql VARCHAR(MAX),
@ParameterString VARCHAR(MAX),
@pv1 VARCHAR(MAX) = NULL, @pv2 VARCHAR(MAX) = NULL, @pv3 VARCHAR(MAX) = NULL, @pv4 VARCHAR(MAX) = NULL, @pv5 VARCHAR(MAX) = NULL,
@pv6 VARCHAR(MAX) = NULL, @pv7 VARCHAR(MAX) = NULL, @pv8 VARCHAR(MAX) = NULL, @pv9 VARCHAR(MAX) = NULL, @pv10 VARCHAR(MAX) = NULL,
@pv11 VARCHAR(MAX) = NULL, @pv12 VARCHAR(MAX) = NULL, @pv13 VARCHAR(MAX) = NULL, @pv14 VARCHAR(MAX) = NULL, @pv15 VARCHAR(MAX) = NULL,
@pv16 VARCHAR(MAX) = NULL, @pv17 VARCHAR(MAX) = NULL, @pv18 VARCHAR(MAX) = NULL, @pv19 VARCHAR(MAX) = NULL, @pv20 VARCHAR(MAX) = NULL,
@pv21 VARCHAR(MAX) = NULL, @pv22 VARCHAR(MAX) = NULL, @pv23 VARCHAR(MAX) = NULL, @pv24 VARCHAR(MAX) = NULL, @pv25 VARCHAR(MAX) = NULL,
@pv26 VARCHAR(MAX) = NULL, @pv27 VARCHAR(MAX) = NULL, @pv28 VARCHAR(MAX) = NULL, @pv29 VARCHAR(MAX) = NULL, @pv30 VARCHAR(MAX) = NULL,
@pv31 VARCHAR(MAX) = NULL, @pv32 VARCHAR(MAX) = NULL, @pv33 VARCHAR(MAX) = NULL, @pv34 VARCHAR(MAX) = NULL, @pv35 VARCHAR(MAX) = NULL,
@pv36 VARCHAR(MAX) = NULL, @pv37 VARCHAR(MAX) = NULL, @pv38 VARCHAR(MAX) = NULL, @pv39 VARCHAR(MAX) = NULL, @pv40 VARCHAR(MAX) = NULL,
@pv41 VARCHAR(MAX) = NULL, @pv42 VARCHAR(MAX) = NULL, @pv43 VARCHAR(MAX) = NULL, @pv44 VARCHAR(MAX) = NULL, @pv45 VARCHAR(MAX) = NULL,
@pv46 VARCHAR(MAX) = NULL, @pv47 VARCHAR(MAX) = NULL, @pv48 VARCHAR(MAX) = NULL, @pv49 VARCHAR(MAX) = NULL, @pv50 VARCHAR(MAX) = NULL,
@pv51 VARCHAR(MAX) = NULL, @pv52 VARCHAR(MAX) = NULL, @pv53 VARCHAR(MAX) = NULL, @pv54 VARCHAR(MAX) = NULL, @pv55 VARCHAR(MAX) = NULL,
@pv56 VARCHAR(MAX) = NULL, @pv57 VARCHAR(MAX) = NULL, @pv58 VARCHAR(MAX) = NULL, @pv59 VARCHAR(MAX) = NULL, @pv60 VARCHAR(MAX) = NULL,
@pv61 VARCHAR(MAX) = NULL, @pv62 VARCHAR(MAX) = NULL, @pv63 VARCHAR(MAX) = NULL, @pv64 VARCHAR(MAX) = NULL, @pv65 VARCHAR(MAX) = NULL,
@pv66 VARCHAR(MAX) = NULL, @pv67 VARCHAR(MAX) = NULL, @pv68 VARCHAR(MAX) = NULL, @pv69 VARCHAR(MAX) = NULL, @pv70 VARCHAR(MAX) = NULL,
@pv71 VARCHAR(MAX) = NULL, @pv72 VARCHAR(MAX) = NULL, @pv73 VARCHAR(MAX) = NULL, @pv74 VARCHAR(MAX) = NULL, @pv75 VARCHAR(MAX) = NULL,
@pv76 VARCHAR(MAX) = NULL, @pv77 VARCHAR(MAX) = NULL, @pv78 VARCHAR(MAX) = NULL, @pv79 VARCHAR(MAX) = NULL, @pv80 VARCHAR(MAX) = NULL,
@pv81 VARCHAR(MAX) = NULL, @pv82 VARCHAR(MAX) = NULL, @pv83 VARCHAR(MAX) = NULL, @pv84 VARCHAR(MAX) = NULL, @pv85 VARCHAR(MAX) = NULL,
@pv86 VARCHAR(MAX) = NULL, @pv87 VARCHAR(MAX) = NULL, @pv88 VARCHAR(MAX) = NULL, @pv89 VARCHAR(MAX) = NULL, @pv90 VARCHAR(MAX) = NULL,
@pv91 VARCHAR(MAX) = NULL, @pv92 VARCHAR(MAX) = NULL, @pv93 VARCHAR(MAX) = NULL, @pv94 VARCHAR(MAX) = NULL, @pv95 VARCHAR(MAX) = NULL,
@pv96 VARCHAR(MAX) = NULL, @pv97 VARCHAR(MAX) = NULL, @pv98 VARCHAR(MAX) = NULL, @pv99 VARCHAR(MAX) = NULL, @pv100 VARCHAR(MAX) = NULL
AS
BEGIN
SET NOCOUNT ON;
DECLARE @Declare VARCHAR(MAX) = '';
WITH
cte_ParameterValues AS (
SELECT
x.ItemNumber,
Value = REPLACE(x.Value, '''', '''''')
FROM ( VALUES
(1, @pv1), (2, @pv2), (3, @pv3), (4, @pv4), (5, @pv5), (6, @pv6), (7, @pv7), (8, @pv8), (9, @pv9), (10, @pv10),
(11, @pv11), (12, @pv12), (13, @pv13), (14, @pv14), (15, @pv15), (16, @pv16), (17, @pv17), (18, @pv18), (19, @pv19), (20, @pv20),
(21, @pv21), (22, @pv22), (23, @pv23), (24, @pv24), (25, @pv25), (26, @pv26), (27, @pv27), (28, @pv28), (29, @pv29), (30, @pv30),
(31, @pv31), (32, @pv32), (33, @pv33), (34, @pv34), (35, @pv35), (36, @pv36), (37, @pv37), (38, @pv38), (39, @pv39), (40, @pv40),
(41, @pv41), (42, @pv42), (43, @pv43), (44, @pv44), (45, @pv45), (46, @pv46), (47, @pv47), (48, @pv48), (49, @pv49), (50, @pv50),
(51, @pv51), (52, @pv52), (53, @pv53), (54, @pv54), (55, @pv55), (56, @pv56), (57, @pv57), (58, @pv58), (59, @pv59), (60, @pv60),
(61, @pv61), (62, @pv62), (63, @pv63), (64, @pv64), (65, @pv65), (66, @pv66), (67, @pv67), (68, @pv68), (69, @pv69), (70, @pv70),
(71, @pv71), (72, @pv72), (73, @pv73), (74, @pv74), (75, @pv75), (76, @pv76), (77, @pv77), (78, @pv78), (79, @pv79), (80, @pv80),
(81, @pv81), (82, @pv82), (83, @pv83), (84, @pv84), (85, @pv85), (86, @pv86), (87, @pv87), (88, @pv88), (89, @pv89), (90, @pv90),
(91, @pv91), (92, @pv92), (93, @pv93), (94, @pv94), (95, @pv95), (96, @pv96), (97, @pv97), (98, @pv98), (99, @pv99), (100, @pv100)
) x (ItemNumber, Value)
)
SELECT
@Declare = CONCAT(@Declare, ',', CHAR(13), r.r, ' = ',
CASE
WHEN t.system_type_id IN (35,40,41,42,61,99,167,175,189,231) AND pv.Value IS NOT NULL
THEN CONCAT('''', pv.Value, '''')
ELSE ISNULL(pv.Value, 'NULL')
END
)
FROM
dbo.SplitCSVToTable8K(@ParameterString, ',') sc
LEFT JOIN cte_ParameterValues pv
ON sc.ItemNumber = pv.ItemNumber
CROSS APPLY ( SELECT r = REPLACE(REPLACE(REPLACE(sc.Item, CHAR(9), ' '), CHAR(10), ''), CHAR(13), '') ) r
CROSS APPLY ( SELECT s1 = CHARINDEX('@', r.r, 1) ) s1
CROSS APPLY ( SELECT s2 = CHARINDEX(' ', r.r, s1.s1) + 1 ) s2
CROSS APPLY ( SELECT dt = SUBSTRING(r.r, s2.s2, ISNULL(NULLIF(CHARINDEX('(', r.r, s2.s2), 0), 8000) - s2.s2 ) ) dt
LEFT JOIN sys.Types t
ON dt.dt = t.name;
SET @Declare = CONCAT('DECLARE', CHAR(13), CHAR(9), STUFF(@Declare, 1, CHARINDEX('@', @Declare, 1) -1, ''), ';', CHAR(13), @sql)
EXEC dbo.LongPrint @Declare; -- This is an in-house stored procedure that gets around the 8000 char limit of PRINT (PRINT can be used for testing w/o the dbo.LongPrint proc)
END;
Here is a trivial example of how it works...
DECLARE
@NumOfRows INT = 555,
@DeBug BIT = 1;
DECLARE @sql NVARCHAR(4000) = '
WITH
cte_n1 (n) AS (SELECT 1 FROM (VALUES (1),(1),(1),(1),(1),(1),(1),(1),(1),(1)) n (n)),
cte_n2 (n) AS (SELECT 1 FROM cte_n1 a CROSS JOIN cte_n1 b),
cte_n3 (n) AS (SELECT 1 FROM cte_n2 a CROSS JOIN cte_n2 b),
cte_Tally (n) AS (
SELECT TOP (@_NumOfRows)
ROW_NUMBER() OVER (ORDER BY (SELECT NULL))
FROM
cte_n3 a CROSS JOIN cte_n3 b
)
SELECT
t.n
FROM
cte_Tally t;'
IF @DeBug = 1
BEGIN
EXEC dbo.PrintSQL @sql, N'@_NumOfRows INT', @NumOfRows;
END
ELSE
BEGIN
EXEC sys.sp_executesql @sql, N'@_NumOfRows INT', @NumOfRows;
END
The Debug outpit...
DECLARE
@_NumOfRows INT = 555;
WITH
cte_n1 (n) AS (SELECT 1 FROM (VALUES (1),(1),(1),(1),(1),(1),(1),(1),(1),(1)) n (n)),
cte_n2 (n) AS (SELECT 1 FROM cte_n1 a CROSS JOIN cte_n1 b),
cte_n3 (n) AS (SELECT 1 FROM cte_n2 a CROSS JOIN cte_n2 b),
cte_Tally (n) AS (
SELECT TOP (@_NumOfRows)
ROW_NUMBER() OVER (ORDER BY (SELECT NULL))
FROM
cte_n3 a CROSS JOIN cte_n3 b
)
SELECT
t.n
FROM
cte_Tally t;
January 17, 2017 at 11:14 am
Seriously? The new forum format is going to replace ending parens with smirky faces???
January 17, 2017 at 12:16 pm
Jason A. Long - Tuesday, January 17, 2017 11:14 AMSeriously? The new forum format is going to replace ending parens with smirky faces???
They forgot to disable bit 67 of DBCC TIMEWARP and it went back to when SSC was first created.
On the subject of your original post, my recommendation is to lookup "Catch All Quries" by Gail Shaw on "SQL in the wild" best way to do this, all other supposed "best practices" on the subject be damned. And yes, dynamic SQL is definitely the way to go for something like this but do study Gail's great post (should be required reading in all universities, tutorials, and forums).
--Jeff Moden
Change is inevitable... Change for the better is not.
January 17, 2017 at 1:16 pm
Jeff Moden - Tuesday, January 17, 2017 12:16 PMJason A. Long - Tuesday, January 17, 2017 11:14 AMSeriously? The new forum format is going to replace ending parens with smirky faces???They forgot to disable bit 67 of DBCC TIMEWARP and it went back to when SSC was first created.
On the subject of your original post, my recommendation is to lookup "Catch All Quries" by Gail Shaw on "SQL in the wild" best way to do this, all other supposed "best practices" on the subject be damned. And yes, dynamic SQL is definitely the way to go for something like this but do study Gail's great post (should be required reading in all universities, tutorials, and forums).
Iv'e read it a couple times (it's been a little while ago though). It's good stuff and very informative but I don't see where she's addressed the debugging of dynamic sql, at least not in this particular article... http://sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries/
Erland Sommarskog makes mention of debugging dynamic sql here (The Curse and Blessings of Dynamic SQL) but doesn't touch on the fact that you still need to get values for the parameters if you want to analyze the execution plan for the resulting debug code.
The purpose of the post was to share some code that others may find useful and to get some other opinions regarding the necessity of using sp_executesql vs the use of a sanitizing function (or whatever else is being used). Seemed like a good conversation topic.
January 17, 2017 at 4:30 pm
Ah, understood. Thanks, Jason.
Hmmm... maybe you should submit an article on this.
--Jeff Moden
Change is inevitable... Change for the better is not.
January 18, 2017 at 4:44 am
If you're after the query plan, why not pull it out of cache?
I'm a DBA.
I'm not paid to solve problems. I'm paid to prevent them.
January 18, 2017 at 4:56 am
Jeff Moden - Tuesday, January 17, 2017 12:16 PMAnd yes, dynamic SQL is definitely the way to go for something like this but do study Gail's great post (should be required reading in all universities, tutorials, and forums).
Funny, because I've just finished writing a follow up to that blog post which, among other things, says that I don't recommend dynamic SQL as the first choice of solution any longer.
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
January 18, 2017 at 5:07 am
Jason A. Long - Tuesday, January 17, 2017 11:13 AMThe use of sys.sp_executesql is certainly a valid method of doing that but so is the use of a “sanitizing function†that scrubs any CHAR or VARCHAR parameters of certain characters and replaces single quotes with double single quotes.
And that I disagree with.
The only method of completely, 100% ensuring that there is absolutely no chance whatsoever of SQL injection, is to never concatenate user input (scrubbed or otherwise) into a string and execute it. sp_executeSQL allows that, since it allows parameters. If you have a function that blacklists certain characters, there's a chance of something getting through. I've seen SQL injection in the last few years that has no quotes, no comment markers, no semicolons, none of the usual things that people blacklist.
The only way I'll use dynamic SQL these days is fully parametrised or with a whitelist function where I can check against lists of things allowed, for example table names and/or column names, not lists of things disallowed.
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
January 18, 2017 at 7:02 am
andrew gothard - Wednesday, January 18, 2017 4:44 AMIf you're after the query plan, why not pull it out of cache?
Because it isn't in the cache... I want to see what the estimated plan is going to look like in production before I release it into production.
Even then, the execution plan alone isn't overly helpful in debugging dynamic sql.
January 18, 2017 at 7:05 am
GilaMonster - Wednesday, January 18, 2017 4:56 AMJeff Moden - Tuesday, January 17, 2017 12:16 PMAnd yes, dynamic SQL is definitely the way to go for something like this but do study Gail's great post (should be required reading in all universities, tutorials, and forums).Funny, because I've just finished writing a follow up to that blog post which, among other things, says that I don't recommend dynamic SQL as the first choice of solution any longer.
Have you published it yet? If so, any chance you'd share the link?
January 18, 2017 at 8:08 am
GilaMonster - Wednesday, January 18, 2017 5:07 AMJason A. Long - Tuesday, January 17, 2017 11:13 AMThe use of sys.sp_executesql is certainly a valid method of doing that but so is the use of a “sanitizing function†that scrubs any CHAR or VARCHAR parameters of certain characters and replaces single quotes with double single quotes.And that I disagree with.
The only method of completely, 100% ensuring that there is absolutely no chance whatsoever of SQL injection, is to never concatenate user input (scrubbed or otherwise) into a string and execute it. sp_executeSQL allows that, since it allows parameters. If you have a function that blacklists certain characters, there's a chance of something getting through. I've seen SQL injection in the last few years that has no quotes, no comment markers, no semicolons, none of the usual things that people blacklist.The only way I'll use dynamic SQL these days is fully parametrised or with a whitelist function where I can check against lists of things allowed, for example table names and/or column names, not lists of things disallowed.
Thank you for the input Gail. As I said, I'm open to being persuaded... Out of curiosity, what method are you using to debug your dynamic sql?
Also, if you have any examples (or links) of sql injection that would thwart a black list function, it would be helpful in convincing developers to make the switch or at the very least provide a solid rationale for switching gears on them.
January 18, 2017 at 8:22 am
Jason A. Long - Wednesday, January 18, 2017 7:05 AMHave you published it yet? If so, any chance you'd share the link?
Not yet, it'll be on my blog when it does get published.
Summary: Option(Recompile) unless compile time or execution frequency dictate otherwise.
Jason A. Long - Wednesday, January 18, 2017 8:08 AMThank you for the input Gail. As I said, I'm open to being persuaded... Out of curiosity, what method are you using to debug your dynamic sql?
Also, if you have any examples (or links) of sql injection that would thwart a black list function, it would be helpful in convincing developers to make the switch or at the very least provide a solid rationale for switching gears on them.
tbh, I avoid dynamic SQL as much as possible these days, and when I do have to use it, I just print the Dynamic SQL out, with the parameter definitions (hardcoded, not generated like your example) and values subed in by hand.
I don't have a link to the attack I saw (I saw it in a server's log), but at the code it uses a hex string (0x010A4A...) and relies on SQL's willingness to convert things
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
January 18, 2017 at 8:59 am
GilaMonster - Wednesday, January 18, 2017 8:22 AMJason A. Long - Wednesday, January 18, 2017 7:05 AMHave you published it yet? If so, any chance you'd share the link?Not yet, it'll be on my blog when it does get published.
Summary: Option(Recompile) unless compile time or execution frequency dictate otherwise.Jason A. Long - Wednesday, January 18, 2017 8:08 AMThank you for the input Gail. As I said, I'm open to being persuaded... Out of curiosity, what method are you using to debug your dynamic sql?
Also, if you have any examples (or links) of sql injection that would thwart a black list function, it would be helpful in convincing developers to make the switch or at the very least provide a solid rationale for switching gears on them.tbh, I avoid dynamic SQL as much as possible these days, and when I do have to use it, I just print the Dynamic SQL out, with the parameter definitions (hardcoded, not generated like your example) and values subed in by hand.
I don't have a link to the attack I saw (I saw it in a server's log), but at the code it uses a hex string (0x010A4A...) and relies on SQL's willingness to convert things
Thanks Gail. I'll Keep an eye out for the new blog post and I already agree with your summary.
I'll do some Google searching to see if there are any articles related to sql injection via hex string or binaries.
Viewing 13 posts - 1 through 12 (of 12 total)
You must be logged in to reply to this topic. Login to reply