September 6, 2012 at 7:53 am
derek.colley (9/6/2012)
Surely there's an argument here that string validation (to take one example) should be handled app-side anyway, so that your table only ever receives clean data?If not, and validation must be done database-side, If you're using a stored procedure to insert the data, and the app is calling the SP, the stored procedure COULD use functions like LTRIM and RTRIM to tidy up the data before entry. I would have thought this would be a function alongside existing validation criteria (like, e.g., removing restricted characters and checking the length of the input string).
The idea of using a trigger to clean data on INSERT to the database seems OK but a bit cumbersome tbh. Surely vulnerable to injection too, what if the string was
" abnormal string ';DISABLE TRIGGER ALL ON DATABASE; --" ?
Well you certainly can't rely on the front end to do your validation.
There is no risk of sql injection based on the contents of the string. It is what you do with that string that could cause injection risk. You would have to execute that string before injection even comes into play. The trigger in this case would be simply to ltrim(rtrim(InsertedData)). In no way would this cause any risk of injection. Now if you executed the inserted data that would be a whole different story.
--edit spelling
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
September 6, 2012 at 7:54 am
derek.colley (9/6/2012)
Surely there's an argument here that string validation (to take one example) should be handled app-side anyway, so that your table only ever receives clean data?If not, and validation must be done database-side, If you're using a stored procedure to insert the data, and the app is calling the SP, the stored procedure COULD use functions like LTRIM and RTRIM to tidy up the data before entry. I would have thought this would be a function alongside existing validation criteria (like, e.g., removing restricted characters and checking the length of the input string).
The idea of using a trigger to clean data on INSERT to the database seems OK but a bit cumbersome tbh. Surely vulnerable to injection too, what if the string was
" abnormal string ';DISABLE TRIGGER ALL ON DATABASE; --" ?
Try it. You'll find that it's not susceptible to injection because it's not being used in dynamic SQL.
--Jeff Moden
September 6, 2012 at 8:00 am
Jeff Moden (9/6/2012)
Try it. You'll find that it's not susceptible to injection because it's not being used in dynamic SQL.
Good point. You'd merely get the string inserted into the table without an execution.
Maybe it's the vast amounts of red-coloured Courier New that I wade through daily (not mine btw) that's biased my view.
I understand the need for dynamic SQL, but still think that in some cases it's the lazy way out.
EDIT: Comments retracted. Try this Jeff:
CREATE TABLE test.injectionTestTable ( foo VARCHAR(100) )
GO
CREATE PROCEDURE sp_InsertSomeData ( @Foo VARCHAR(100) )
AS BEGIN
INSERT INTO test.injectionTestTable(foo)
SELECT @Foo
END
EXEC sp_InsertSomeData @Foo='Hello World'
EXEC sp_InsertSomeData @Foo='Hello World ';SELECT 'Injection success!' --'
EDIT (again)!: Facepalm moment. Just realised what's wrong with trying to test this from SSMS.
When I replaced the outer ' with ", I see the data is inserted.
I'm not much of a dev guru so can't test this from an external application call, but I reckon it would be possible to fool a call to the SP by using the extra injection code, though.
---
Note to developers:Want to get the best help? Click here https://www.sqlservercentral.com/articles/forum-etiquette-how-to-post-datacode-on-a-forum-to-get-the-best-help (Jeff Moden)
My blog: http://uksqldba.blogspot.com
Visit http://www.DerekColley.co.uk to find out more about me.
September 6, 2012 at 8:04 am
I guess the trigger method for cleaning up new data would be ok but that places an insert tax on the system. I'd prefer not to do that if possible. And, as stated, it does nothing to clean up the old data.
On the other hand, I'm a stickler for code that works correctly and will force the hand of developers. This type of cleanup should really be done on the front end (IMHO) and one way to force that, if it's really a problem on given fields in the app, is to add a constraint that does a check for leading and trailing spaces. Of course, that's also an unnecessary tax on the system.
This is one of the reasons I prefer CRUD rather than using ORMs. You can do this type of cleanup and either provide intelligent error messages or do the cleanup without the tax of doing an insert following by an upate in an after trigger.
You could also use a instead-of trigger but that brings a whole other world of hurt into play.
The other thing to consider is that other triggers (an audit trigger) may someday be added to the table and this type of cleanup done as a trigger may interfere with the values that actually get stored in the audit table unless the audit trigger also does a cleanup.
--Jeff Moden
September 6, 2012 at 8:05 am
Jeff/Sean - see edited post above.
---
Note to developers:Want to get the best help? Click here https://www.sqlservercentral.com/articles/forum-etiquette-how-to-post-datacode-on-a-forum-to-get-the-best-help (Jeff Moden)
My blog: http://uksqldba.blogspot.com
Visit http://www.DerekColley.co.uk to find out more about me.
September 6, 2012 at 8:09 am
derek.colley (9/6/2012)
Jeff Moden (9/6/2012)
Try it. You'll find that it's not susceptible to injection because it's not being used in dynamic SQL.
Good point. You'd merely get the string inserted into the table without an execution.Maybe it's the vast amounts of red-coloured Courier New that I wade through daily (not mine btw) that's biased my view.
I understand the need for dynamic SQL, but still think that in some cases it's the lazy way out.
EDIT: Comments retracted. Try this Jeff:
CREATE TABLE test.injectionTestTable ( foo VARCHAR(100) )
GO
CREATE PROCEDURE sp_InsertSomeData ( @Foo VARCHAR(100) )
AS BEGIN
INSERT INTO test.injectionTestTable(foo)
SELECT @Foo
END
EXEC sp_InsertSomeData @Foo='Hello World'
EXEC sp_InsertSomeData @Foo='Hello World ';SELECT 'Injection success!' --'
This doesn't do any sql injection. Try running this and then select * from test.injectionTestTable. You will have two rows.
Your second exec statement is not how sql injection works. You have created two sql statements.
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
September 6, 2012 at 8:14 am
Yes, sorry Jeff. Edited post above to reflect this a few moments ago. When I replace the outer ' with " then the 'inject' merely inserts a row. Got a bit over-excited when I got SSMS to do my bidding, I suppose!
[wanders off for more coffee]...
---
Note to developers:Want to get the best help? Click here https://www.sqlservercentral.com/articles/forum-etiquette-how-to-post-datacode-on-a-forum-to-get-the-best-help (Jeff Moden)
My blog: http://uksqldba.blogspot.com
Visit http://www.DerekColley.co.uk to find out more about me.
September 6, 2012 at 8:26 am
I'm not much of a dev guru so can't test this from an external application call, but I reckon it would be possible to fool a call to the SP by using the extra injection code, though.
Not really sure what you mean. You are calling your proc with a parameter. Your proc never executes the parameter. There is absolutely zero chance of sql injection based on what you posted.
I modified your proc to demonstrate how sql injection could come into play in a situation like this.
alter PROCEDURE sp_InsertSomeData ( @Foo VARCHAR(100) )
AS BEGIN
--INSERT INTO injectionTestTable(foo)
-- SELECT @Foo
declare @sql varchar(max)
set @sql = 'insert into injectionTestTable(foo) values (''' + @Foo + ''')'
select @sql
exec(@sql)
END
Notice that now your parameter is getting executed as dynamic sql. If you now called your proc like below you will see how injection can work.
EXEC sp_InsertSomeData @Foo='Hello World'');insert injectionTestTable(foo) values (''This injection worked '
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
September 6, 2012 at 8:52 am
Jeff Moden (9/6/2012)
I guess the trigger method for cleaning up new data would be ok but that places an insert tax on the system. I'd prefer not to do that if possible. And, as stated, it does nothing to clean up the old data.On the other hand, I'm a stickler for code that works correctly and will force the hand of developers. This type of cleanup should really be done on the front end (IMHO) and one way to force that, if it's really a problem on given fields in the app, is to add a constraint that does a check for leading and trailing spaces. Of course, that's also an unnecessary tax on the system.
This is one of the reasons I prefer CRUD rather than using ORMs. You can do this type of cleanup and either provide intelligent error messages or do the cleanup without the tax of doing an insert following by an upate in an after trigger.
You could also use a instead-of trigger but that brings a whole other world of hurt into play.
The other thing to consider is that other triggers (an audit trigger) may someday be added to the table and this type of cleanup done as a trigger may interfere with the values that actually get stored in the audit table unless the audit trigger also does a cleanup.
Obviously the existing data needs cleaned up separately. But that is truly valuable only if you can insure that future data is clean also. Otherwise you'd still have to LTRIM/RTRIM every time anyway.
Certainly the trigger should be written as efficiently as poossible, as always for all triggers.
Checking the data in the code is great theoretically, except that any place the data can be inserted you have to repeat the code. You could force all inserts thru one stored proc, again theoretically, but there's usually a crisis "one-time" where someone changes data live.
I agree about the INSTEAD OF trigger ... I would definitely avoid that here if possible.
I don't think audit triggers are relevant any more, given the better options built into SQL itself now. However, I would always tend to set this type of trigger to run first anyway to be consistent -- no other code ever has to deal with LTRIM/RTRIM/etc..
SQL DBA,SQL Server MVP(07, 08, 09) "Money can't buy you happiness." Maybe so, but it can make your unhappiness a LOT more comfortable!
September 6, 2012 at 9:35 am
ScottPletcher (9/6/2012)
I don't think audit triggers are relevant any more, given the better options built into SQL itself now.
I've not found those options to do so well.
--Jeff Moden
September 6, 2012 at 9:45 am
Jeff Moden (9/6/2012)
ScottPletcher (9/6/2012)
I don't think audit triggers are relevant any more, given the better options built into SQL itself now.I've not found those options to do so well.
Interesting. I've had no issues with CDC, and it's way less overhead than a traditional auditing trigger.
Any auditing trigger will be vastly more overhead than the simple LRTRIM(RTRIM( trigger we're talking about for this table.
SQL DBA,SQL Server MVP(07, 08, 09) "Money can't buy you happiness." Maybe so, but it can make your unhappiness a LOT more comfortable!
September 6, 2012 at 9:55 am
ScottPletcher (9/6/2012)
Jeff Moden (9/6/2012)
ScottPletcher (9/6/2012)
I don't think audit triggers are relevant any more, given the better options built into SQL itself now.I've not found those options to do so well.
Interesting. I've had no issues with CDC, and it's way less overhead than a traditional auditing trigger.
Any auditing trigger will be vastly more overhead than the simple LRTRIM(RTRIM( trigger we're talking about for this table.
And CDC probably works just great, if you have Enterprise Edition (Developer Edition and Evaluation Edition as well). If you don't, it isn't available.
September 6, 2012 at 9:55 am
ScottPletcher (9/6/2012)
Jeff Moden (9/6/2012)
ScottPletcher (9/6/2012)
I don't think audit triggers are relevant any more, given the better options built into SQL itself now.I've not found those options to do so well.
Interesting. I've had no issues with CDC, and it's way less overhead than a traditional auditing trigger.
Any auditing trigger will be vastly more overhead than the simple LRTRIM(RTRIM( trigger we're talking about for this table.
We have been having a discussion about a static look up table. I say the whole discussion about a trigger for a states table has gone from somewhat silly to completely ridiculous. The states in the US have not changed since 1959 and there is not much chance it is going to change anytime soon. Insert the data for all the states (properly formatted) and move on. If this table was even remotely dynamic I can see where the trigger and such would come into play but for a static list it just isn't needed.
I realize that the concepts can easily extend well beyond this example but the OP has not posted back in several pages of discussion at this point.
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
September 6, 2012 at 11:25 am
Did you guys (and gals) notice the original question and poorly written table structure? I think this is a state reference table with 50 entries. I'd just manually redo it... 🙂
Jared
CE - Microsoft
September 6, 2012 at 11:33 am
SQLKnowItAll (9/6/2012)
Did you guys (and gals) notice the original question and poorly written table structure? I think this is a state reference table with 50 entries. I'd just manually redo it... 🙂
Probably right. Looks like the table may be really messed up now since an UPDATE "deleted" data instead of cleaning it up.
Viewing 15 posts - 31 through 45 (of 45 total)
You must be logged in to reply to this topic. Login to reply