June 28, 2016 at 6:03 am
Hello,
I am trying to dynamically update a table (as much as possible) based on a parameter. However, because I'm trying to avoid SQL injection I'm hard coding the table names, but still need to be able to select which one based on another parameter (@State).
@ID int,
@State varChar,
@ColumnName varchar,
@NewValue datetime
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
-- Insert statements for procedure here
SELECT CASE @State
Case 'AZ' THEN
UPDATE AZ_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
Case 'CA' THEN
UPDATE CA_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
Case 'HI' THEN
UPDATE HI_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
What is the best way to go about this?
David92595
June 28, 2016 at 6:20 am
IF @State = 'AZ'
UPDATE AZ_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
IF @State = 'CA'
UPDATE CA_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
...
Alternately, use dynamic SQL and check that the table name being passed/constructed matches a list of existing table names/state codes. A value like 'CA''; DROP TABLE Users;--' won't match anything in the 2-character list of state abbreviations, neither would the constructed table name 'CA''; DROP TABLE Users;--_Fees' match anything in sys.tables and hence you could throw any such attempts out
Why separate tables for each state?
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
June 28, 2016 at 6:29 am
That is Originally what I wanted to do, but after searching online, everyone seems paranoid of SQL injection. So I thought I'd do it this way...
How would I write it then, all of my attempts have failed.
UPDATE @State + '_Fees'
SET @ColumnName = @NewValue
WHERE ID = @ID
OR
UPDATE @State + '_Fees WHERE ID = @ID'
SET @ColumnName = @NewValue
June 28, 2016 at 6:44 am
You need Dynamic SQL, you can't parameterise a table name. Build up a string, execute it.
And yes, people are paranoid for good reason, which is why you will check that the value passed is a valid state (by comparing against a known list) or that the resulting table name is a valid table name (by comparing against the system tables)
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
June 28, 2016 at 7:02 am
David92595 (6/28/2016)
Hello,I am trying to dynamically update a table (as much as possible) based on a parameter. However, because I'm trying to avoid SQL injection I'm hard coding the table names, but still need to be able to select which one based on another parameter (@State).
@ID int,
@State varChar,
@ColumnName varchar,
@NewValue datetime
AS
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
-- interfering with SELECT statements.
SET NOCOUNT ON;
-- Insert statements for procedure here
SELECT CASE @State
Case 'AZ' THEN
UPDATE AZ_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
Case 'CA' THEN
UPDATE CA_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
Case 'HI' THEN
UPDATE HI_Fees
SET @ColumnName = @NewValue
WHERE ID = @ID
What is the best way to go about this?
David92595
My question would be... why are these fees stored in a separate table for each state?
--Jeff Moden
Change is inevitable... Change for the better is not.
June 28, 2016 at 9:28 am
As a quickie "solution", for now, something like this:
SET ANSI_NULLS ON;
SET QUOTED_IDENTIFIER ON;
GO
ALTER PROCEDURE <proc_name>
@ID int,
@State varchar(30),
@ColumnName varchar(100),
@NewValue datetime
AS
SET NOCOUNT ON;
IF @State LIKE '%[;]%' OR @ColumnName LIKE '%[;]%' OR @NewValue LIKE '%[;]%'
RETURN -100 /*sql injection attempt!*/
DECLARE @sql varchar(8000)
SET @sql = 'UPDATE [' + @State + '_Fees] SET [' + PARSENAME(@ColumnName, 1) + '] = ''' + @NewValue + ''''
EXEC(@sql)
GO
SQL DBA,SQL Server MVP(07, 08, 09) A socialist is someone who will give you the shirt off *someone else's* back.
June 28, 2016 at 9:36 am
Because I inherited this database and the person that made it thought in spreadsheets not relational tables.
Thank you! Are their any other good ways to stop a sql injection? My database, until recently, has been small and is still only internal so exposure to injections has been minimal. I know i need to get ahead of the curve on this though.
David
June 28, 2016 at 10:39 am
David92595 (6/28/2016)
@JeffBecause I inherited this database and the person that made it thought in spreadsheets not relational tables.
Thank you! Are their any other good ways to stop a sql injection? My database, until recently, has been small and is still only internal so exposure to injections has been minimal. I know i need to get ahead of the curve on this though.
David
Yes, for sql injection, there are definitely other measures that need taken. My code above is just quick way to do a reasonable check for injection prior to executing.
Another potential issue with dynamic SQL is that permissions must be granted explicitly granted to the user running the code, and not come thru a role. If the code is being run under a "power user"/"power app" id, that's not an issue, but if you run code as each user, it can be a permissions issue with dynamic SQL.
SQL DBA,SQL Server MVP(07, 08, 09) A socialist is someone who will give you the shirt off *someone else's* back.
June 28, 2016 at 11:28 am
David92595 (6/28/2016)
Are their any other good ways to stop a sql injection?
Yes. Don't concatenate input into strings to be executed unless completely unavoidable (eg table names and column names), if you have to then whitelist the input to ensure it's acceptable (blacklist, like Scott shows, is generally less safe as a smart attacker might be able to get around it, for example by passing a binary string and relying on SQL's implicit conversion).
From earlier:
use dynamic SQL and check that the table name being passed/constructed matches a list of existing table names/state codes. A value like 'CA''; DROP TABLE Users;--' won't match anything in the 2-character list of state abbreviations, neither would the constructed table name 'CA''; DROP TABLE Users;--_Fees' match anything in sys.tables and hence you could throw any such attempts out
Parameterise everywhere you can, it reduces the work needed in checking the strings. So:
ALTER PROCEDURE <proc_name>
@ID INT,
@State CHAR(2),
@ColumnName VARCHAR(256),
@NewValue DATETIME
AS
SET NOCOUNT ON;
DECLARE @TableName sysname;
DECLARE @sql NVARCHAR(4000);
SET @TableName = @State + '_Fees';
IF NOT EXISTS ( SELECT 1 FROM sys.tables WHERE name = @TableName )
BEGIN
RAISERROR('Invalid State' , 16, 1);
RETURN;
END;
IF NOT EXISTS ( SELECT 1 FROM sys.columns WHERE name = @ColumnName )
BEGIN
RAISERROR('Invalid Column' , 16, 1);
RETURN;
END;
SET @sql = 'UPDATE [' + @TableName + '] SET [' + @ColumnName + '] = @_NewValue WHERE ID = @_ID';
EXEC sp_executesql @sql, '@_NewValue DATETIME, @_ID INT', @_NewValue = @NewValue, @_ID = @ID;
GO
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
June 28, 2016 at 12:13 pm
David92595 (6/28/2016)
@JeffBecause I inherited this database and the person that made it thought in spreadsheets not relational tables.
Thank you! Are their any other good ways to stop a sql injection? My database, until recently, has been small and is still only internal so exposure to injections has been minimal. I know i need to get ahead of the curve on this though.
David
There's a possibility of making this almost brain-dead simple and totally SQL Injection proof and not require ANY dynamic SQL...
If you ad a constraint to the state column of each table to force a limit on what can be in the table, you could create and updateable "Partitioned View" that would treat all of the related tables almost as if it were a single table. The view would need to have a hardcoded column for "state" for each table. That way you would pass the ID and the state and both would simply be included in the WHERE clause. No dynamic SQL needed at all.
--Jeff Moden
Change is inevitable... Change for the better is not.
June 28, 2016 at 1:25 pm
But if you add more states (so more tables), you would only have to fix one thing, and everything would continue to work.
June 28, 2016 at 3:34 pm
Jeff Moden (6/28/2016)
David92595 (6/28/2016)
@JeffBecause I inherited this database and the person that made it thought in spreadsheets not relational tables.
Thank you! Are their any other good ways to stop a sql injection? My database, until recently, has been small and is still only internal so exposure to injections has been minimal. I know i need to get ahead of the curve on this though.
David
There's a possibility of making this almost brain-dead simple and totally SQL Injection proof and not require ANY dynamic SQL...
If you ad a constraint to the state column of each table to force a limit on what can be in the table, you could create and updateable "Partitioned View" that would treat all of the related tables almost as if it were a single table. The view would need to have a hardcoded column for "state" for each table. That way you would pass the ID and the state and both would simply be included in the WHERE clause. No dynamic SQL needed at all.
Hmm ... since the column name to be updated is passed in as well, you'd at least need to regenerate the view whenever the table schema changed.
SQL DBA,SQL Server MVP(07, 08, 09) A socialist is someone who will give you the shirt off *someone else's* back.
June 28, 2016 at 4:09 pm
David92595 (6/28/2016)
@JeffIf im going to hard code it wouldn't it just be easier (i.e. not having to create a view) to go with the IF @State = 'AZ'...
Its hard coded to the extent that only my 3 tables listed will be able yo be altered.
Stop and think about how the tables are named. It would be easy to write a bit of code that took advantage of that fact and you could have a stored procedure that would auto-magically find the tables and create the view from their names. I've done such a thing many times, even across databases. Getting ready to do so, again, to replace the mistake known as a "Partitioned Table" with a "Partitioned View".
--Jeff Moden
Change is inevitable... Change for the better is not.
June 28, 2016 at 4:13 pm
ScottPletcher (6/28/2016)
Jeff Moden (6/28/2016)
David92595 (6/28/2016)
@JeffBecause I inherited this database and the person that made it thought in spreadsheets not relational tables.
Thank you! Are their any other good ways to stop a sql injection? My database, until recently, has been small and is still only internal so exposure to injections has been minimal. I know i need to get ahead of the curve on this though.
David
There's a possibility of making this almost brain-dead simple and totally SQL Injection proof and not require ANY dynamic SQL...
If you ad a constraint to the state column of each table to force a limit on what can be in the table, you could create and updateable "Partitioned View" that would treat all of the related tables almost as if it were a single table. The view would need to have a hardcoded column for "state" for each table. That way you would pass the ID and the state and both would simply be included in the WHERE clause. No dynamic SQL needed at all.
Hmm ... since the column name to be updated is passed in as well, you'd at least need to regenerate the view whenever the table schema changed.
Ah, crud (no pun intended)... agreed. Forgot about the column name. Ya just gotta love "catch all" queries.
--Jeff Moden
Change is inevitable... Change for the better is not.
Viewing 15 posts - 1 through 15 (of 15 total)
You must be logged in to reply to this topic. Login to reply