August 14, 2018 at 4:55 am
Thom A - Tuesday, August 14, 2018 4:19 AMpeter.row - Tuesday, August 14, 2018 4:06 AMWhat I'd really like is a QUOTENAME function that only quoted if the name required it because the name being used had spaces, represents a keyword etc...
Why? Because I have some scaffolding scripts that generate SQL scripts based on a table schema etc... The resulting script is then saved and used and is under version control.I really hate reading through SQL full of [tablename].[column] formatting it is totally distracting to me. I only quote stuff when needed for SQL to understand it or for SSMS/SOS not to highlight it as a keyword.
I use sp_executesql @theSQLCommand, @ParamsAsString, @Param1Value, @Parm2Value... ParmXValue to avoid SQL injection when using dynamic SQL.
I don't agree with that mentality on the function, nor am I sure that could even work. Key and reserved words can (and do) change. If you were using a linked server you might end up not quoting an object name that in another version needs to be quoted (as the objects name is now a reserved word, or perhaps no longer is (although I doubt the latter would happen)). I admit, I'm not a massive fan of [Server].[Database].[Schema].[Object] either, however, I'd much rather have all of them quoted and it work 100% of the time, than only have the object quoted when SQL Server deemed it "needed". You could then have a scenario in the future that due to a new keyword a breaking change is introduced, and QUOTENAME won't handle it.
Far better do do something 100% of the time, and thus ensure 100% functionality, than do it only when you think "it is needed", and then miss a time when it is (and thus cause a failure).
Generally I'd probably agree but when you are a developer working on your product that you know inside out and you only support versions X/Y/Z of SQL Server then you can apply some human knowledge into it to know whether or not the choice is fine. I'm not talking about SQL that is used dynamically with no knowledge of what is going on I'm talking about specific use cases where you are generating, for example, stored procedures for a new dictionary table that fits into your framework for dealing with dictionaries in frontend/backend.
August 14, 2018 at 5:05 am
peter.row - Tuesday, August 14, 2018 4:55 AMGenerally I'd probably agree but when you are a developer working on your product that you know inside out and you only support versions X/Y/Z of SQL Server then you can apply some human knowledge into it to know whether or not the choice is fine. I'm not talking about SQL that is used dynamically with no knowledge of what is going on I'm talking about specific use cases where you are generating, for example, stored procedures for a new dictionary table that fits into your framework for dealing with dictionaries in frontend/backend.
Not sure I really understand your point here though. if it's SQL generated dynamic SQL, why does it matter if all the object names are quoted or not anyway? The only time, generally, you look at dynamic SQL generated from SQL (like in the article) is to debug it; you're not looking at it otherwise as it happening behind the scenes. If it's behind the scenes, why does it matter (to you), if the object name is written as TableName or [TableName]?
Thom~
Excuse my typos and sometimes awful grammar. My fingers work faster than my brain does.
Larnu.uk
August 14, 2018 at 6:03 am
Thom A - Tuesday, August 14, 2018 5:05 AMpeter.row - Tuesday, August 14, 2018 4:55 AMGenerally I'd probably agree but when you are a developer working on your product that you know inside out and you only support versions X/Y/Z of SQL Server then you can apply some human knowledge into it to know whether or not the choice is fine. I'm not talking about SQL that is used dynamically with no knowledge of what is going on I'm talking about specific use cases where you are generating, for example, stored procedures for a new dictionary table that fits into your framework for dealing with dictionaries in frontend/backend.
Not sure I really understand your point here though. if it's SQL generated dynamic SQL, why does it matter if all the object names are quoted or not anyway? The only time, generally, you look at dynamic SQL generated from SQL (like in the article) is to debug it; you're not looking at it otherwise as it happening behind the scenes. If it's behind the scenes, why does it matter (to you), if the object name is written as TableName or [TableName]?
Because it's dynamically generating SQL that you otherwise would have to write by hand. Once generated you save it to a file and it becomes part of your source control. Before doing that you might need to tweak what was generated because the generating script can't take into account all possible use cases.
August 14, 2018 at 6:16 am
RonKyle - Monday, August 13, 2018 11:19 AMThom A - Monday, August 13, 2018 9:08 AMLuis Cazares - Monday, August 13, 2018 7:34 AMRonKyle - Monday, August 13, 2018 6:22 AMWhy did you use a sysname data type in your example? I've used this approach before, but the variables where char or varchar with not too long a length, e.g. @TableName varchar(10). Nothing against the use of the function. It looks useful. But your example seems to be something of a straw man.I'm guessing that's because objects names in SQL Server use that data type which is basically a synonym for nvarchar(128).
Since that's the maximum length accepted for QUOTENAME, it seems logical to use it.Luis is exactly right here. Although I would never suggest having an object (especially a table/view/function) with a name consisting of 128 characters, SQL Server does permit it. I would very much not be surprised if there are people out there that do use very long object names. Using sysname seems the logical choice here, therefore, when dealing with a dynamic object, as (like Luis said) it is basically a synonym for nvarchar(128).
I can see the point. But if I know my table names don't exceed 20 characters, using that instead of the sysname data type goes a long way towards hindering SQL injection as the query simply can't be long enough. I'm still trying to think this through, however, and may change my opinion.
If it's a query that's created using concatenation rather than parameterization, then you may need QUOTENAME to avoid SQL Injection even if there are "only" 20 characters. One of the frequent hacks is to add something very short that will cause an error that will give the hacker a clue as to what the next hack attempt should be.
Of course, it's far better to use parameterized queries even if you're using it for "Catch All" queries, which don't require the use of QUOTENAME if you do it right. Gail Shaw has what I consider to be the definitive article on the subject at the following URL:
https://sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries/
--Jeff Moden
August 14, 2018 at 6:24 am
Thom A - Tuesday, August 14, 2018 4:19 AMpeter.row - Tuesday, August 14, 2018 4:06 AMWhat I'd really like is a QUOTENAME function that only quoted if the name required it because the name being used had spaces, represents a keyword etc...
Why? Because I have some scaffolding scripts that generate SQL scripts based on a table schema etc... The resulting script is then saved and used and is under version control.I really hate reading through SQL full of [tablename].[column] formatting it is totally distracting to me. I only quote stuff when needed for SQL to understand it or for SSMS/SOS not to highlight it as a keyword.
I use sp_executesql @theSQLCommand, @ParamsAsString, @Param1Value, @Parm2Value... ParmXValue to avoid SQL injection when using dynamic SQL.
I don't agree with that mentality on the function, nor am I sure that could even work. Key and reserved words can (and do) change. If you were using a linked server you might end up not quoting an object name that in another version needs to be quoted (as the objects name is now a reserved word, or perhaps no longer is (although I doubt the latter would happen)). I admit, I'm not a massive fan of [Server].[Database].[Schema].[Object] either, however, I'd much rather have all of them quoted and it work 100% of the time, than only have the object quoted when SQL Server deemed it "needed". You could then have a scenario in the future that due to a new keyword a breaking change is introduced, and QUOTENAME won't handle it.
Far better do do something 100% of the time, and thus ensure 100% functionality, than do it only when you think "it is needed", and then miss a time when it is (and thus cause a failure).
Heh... I have to admit that I absolutely hate brackets in code most of the time (there are certain exceptions, as with anything else). I hate the fact that when MS generates code for you, that it's riddled with brackets, which is totally unnecessary (IMHO) eye clutter. It think that it's far better to give the person doing the programming the option at to what they'd like to see or the standards that they're being required to follow.
While I also agree with "Bullet Proofing Code", trying to anticipate that a future version of SQL might require something like putting Linked Names in brackets is a little steep on the "it could happen" list for me to actually change decades of coding style.
And, no... none of that is meant as an argument. Like you, I'm just expressing personal preferences here.
--Jeff Moden
August 14, 2018 at 6:28 am
peter.row - Tuesday, August 14, 2018 4:06 AMWhat I'd really like is a QUOTENAME function that only quoted if the name required it because the name being used had spaces, represents a keyword etc...
Why? Because I have some scaffolding scripts that generate SQL scripts based on a table schema etc... The resulting script is then saved and used and is under version control.I really hate reading through SQL full of [tablename].[column] formatting it is totally distracting to me. I only quote stuff when needed for SQL to understand it or for SSMS/SOS not to highlight it as a keyword.
I use sp_executesql @theSQLCommand, @ParamsAsString, @Param1Value, @Parm2Value... ParmXValue to avoid SQL injection when using dynamic SQL.
I'd love that option but it would have to be an option so that I can support shops that require the damned brackets, which I also hate. 😀
--Jeff Moden
August 14, 2018 at 7:18 am
Jeff Moden - Tuesday, August 14, 2018 6:28 AMpeter.row - Tuesday, August 14, 2018 4:06 AMWhat I'd really like is a QUOTENAME function that only quoted if the name required it because the name being used had spaces, represents a keyword etc...
Why? Because I have some scaffolding scripts that generate SQL scripts based on a table schema etc... The resulting script is then saved and used and is under version control.I really hate reading through SQL full of [tablename].[column] formatting it is totally distracting to me. I only quote stuff when needed for SQL to understand it or for SSMS/SOS not to highlight it as a keyword.
I use sp_executesql @theSQLCommand, @ParamsAsString, @Param1Value, @Parm2Value... ParmXValue to avoid SQL injection when using dynamic SQL.
I'd love that option but it would have to be an option so that I can support shops that require the damned brackets, which I also hate. 😀
Well they can use QUOTENAME and we can use QUOTENAMEIF, 🙂 - we can dream, :Wow:
August 15, 2018 at 12:11 am
Good article, important to remember a simple check of permissions catches sql injection. Why give an account create table in the first place
-----------------------------------------------------------------------------------------------------------
"Ya can't make an omelette without breaking just a few eggs" 😉
August 15, 2018 at 7:27 am
Perry Whittle - Wednesday, August 15, 2018 12:11 AMGood article, important to remember a simple check of permissions catches sql injection. Why give an account create table in the first place
How would you do that?
August 15, 2018 at 7:35 am
RonKyle - Wednesday, August 15, 2018 7:27 AMPerry Whittle - Wednesday, August 15, 2018 12:11 AMGood article, important to remember a simple check of permissions catches sql injection. Why give an account create table in the first placeHow would you do that?
One way is to grant db_ddladmin privs to a user of the database. You can grant more specifically but no users should have beyond db_datareader and, maybe, db_datawriter, IMHO. Actually, my personal belief is that even those privs are too high especially for application logins. Those users should only have PUBLIC privs and the privs to execute certain stored procedures that do all the work. Such stored procedures can have the ability to create tables and other things while the user has basically no privs and cannot deviate from what the proc allows.
Yeah... I know.... that sets a lot of peoples' hair on fire but, in the two places where I've seen it done, security was incredible and it wasn't as big a problem as everyone thought it was going to be. It didn't take as long to do as people were expecting, either.
--Jeff Moden
August 15, 2018 at 7:42 am
RonKyle - Wednesday, August 15, 2018 7:27 AMPerry Whittle - Wednesday, August 15, 2018 12:11 AMGood article, important to remember a simple check of permissions catches sql injection. Why give an account create table in the first placeHow would you do that?
If it's something like a website accounnt, then like Perry said, only give it the permissions it needs. That might be as limited as only granted it the EXEC command on the schema web, if you only permit your website to interact with your SQL Server via Stored Procedures.Unfortunately, that doesn't always work in the real world, due to things like Entity Framework, which takes the ownership of creating database objects from the DBA/SQL Developer and giving them to the application developer; which isn't something I'm really a fan of. if that is the case, then it's really important to ensure that the login doesn't have any further permissions it shouldn't have outside of it's "little" bubble.
Thom~
Excuse my typos and sometimes awful grammar. My fingers work faster than my brain does.
Larnu.uk
August 15, 2018 at 7:52 am
Thom A - Wednesday, August 15, 2018 7:42 AMRonKyle - Wednesday, August 15, 2018 7:27 AMPerry Whittle - Wednesday, August 15, 2018 12:11 AMGood article, important to remember a simple check of permissions catches sql injection. Why give an account create table in the first placeHow would you do that?
If it's something like a website accounnt, then like Perry said, only give it the permissions it needs. That might be as limited as only granted it the EXEC command on the schema web, if you only permit your website to interact with your SQL Server via Stored Procedures.Unfortunately, that doesn't always work in the real world, due to things like Entity Framework, which takes the ownership of creating database objects from the DBA/SQL Developer and giving them to the application developer; which isn't something I'm really a fan of. if that is the case, then it's really important to ensure that the login doesn't have any further permissions it shouldn't have outside of it's "little" bubble.
This also applies to Jeff's comments. The original comment said a simple check of permissions. How is this "simple check" done. My view is by not using sysname as a data type, you can go a long way towards stopping the sql injection. I'm just trying to understand better the comments that I'm reading.
August 15, 2018 at 11:27 am
RonKyle - Wednesday, August 15, 2018 7:52 AMThom A - Wednesday, August 15, 2018 7:42 AMRonKyle - Wednesday, August 15, 2018 7:27 AMPerry Whittle - Wednesday, August 15, 2018 12:11 AMGood article, important to remember a simple check of permissions catches sql injection. Why give an account create table in the first placeHow would you do that?
If it's something like a website accounnt, then like Perry said, only give it the permissions it needs. That might be as limited as only granted it the EXEC command on the schema web, if you only permit your website to interact with your SQL Server via Stored Procedures.Unfortunately, that doesn't always work in the real world, due to things like Entity Framework, which takes the ownership of creating database objects from the DBA/SQL Developer and giving them to the application developer; which isn't something I'm really a fan of. if that is the case, then it's really important to ensure that the login doesn't have any further permissions it shouldn't have outside of it's "little" bubble.
This also applies to Jeff's comments. The original comment said a simple check of permissions. How is this "simple check" done. My view is by not using sysname as a data type, you can go a long way towards stopping the sql injection. I'm just trying to understand better the comments that I'm reading.
It's not sysname as a datatype that's the problem for SQL Injection. Concatenating things like column names, table names, and even criteria in a WHERE clause is the problem when it comes to SQL Injection. QUOTENAME is a useful tool in overriding a lot of possible attacks simply because it not only encapsulates the data in brackets, but will also encapsulate embedded brackets and a few other things to render them harmless if concatenation is absolutely necessary (for a table name, for example). Still, I'll read the variable and play it against the list of table names to ensure that it also actually contains a valid table name.
The reason why it's based on sysname is because that was the intended purpose... to work with meta-data names, which are all basically sysname aliased datatypes.
--Jeff Moden
August 15, 2018 at 1:16 pm
It's not sysname as a datatype that's the problem for SQL Injection. Concatenating things like column names, table names, and even criteria in a WHERE clause is the problem when it comes to SQL Injection. QUOTENAME is a useful tool in overriding a lot of possible attacks simply because it not only encapsulates the data in brackets, but will also encapsulate embedded brackets and a few other things to render them harmless if concatenation is absolutely necessary (for a table name, for example). Still, I'll read the variable and play it against the list of table names to ensure that it also actually contains a valid table name.
The reason why it's based on sysname is because that was the intended purpose... to work with meta-data names, which are all basically sysname aliased datatypes.
[/quot
Jeff hits the nail on the head here. Yes, a varchar(20) makes it harder to inject with, not not impossible. On the other hand, a properly quoted sysname/nvarchar(128) will make it far far harder. The choice of datatype is irrelevant if the code is open to Injection, and is also irrelevant if it isn't.
But, this article isn't only aimed at environments where an object has a name of 20 characters or less. I've seen plenty of examples, in questions online and in the workplace, where an object's name is more than 20 characters. The SP's in the article are (simple) examples, but (at least for objects, the filename has a caveat discussed in the comments earlier) will work with any given name, regardless of how long, short, or characters it may contain. If you, in your environment, want to use a varchar(20), as you can guarantee it'll always then, then that's great, but the real world is not so kind. 🙂
Thom~
Excuse my typos and sometimes awful grammar. My fingers work faster than my brain does.
Larnu.uk
August 16, 2018 at 6:07 am
Thom, thanks again for taking the time to write this article. It's not only a great "Back to Basics" article but it turned out to be an "I Learned Something New" article for a lot of good folks. It's also made for quite the interesting discussion that I think a whole lot of people appreciate because it has its roots in one of the most important aspects of security there is, SQL Injection. My hat is off to you and everyone, regardless of the stance they've taken, because the discussion poses questions that a whole lot of people simply could never anticipate when writing an article. This is why these types of articles can be a thousand times more helpful than Books Online and one of the reasons why I love the SQLServerCentral community and the SQL Server community in general.
--Jeff Moden
Viewing 15 posts - 16 through 30 (of 40 total)
You must be logged in to reply to this topic. Login to reply