November 11, 2008 at 5:28 am
The best thing to do, if it can be done, is to use variables like the original code. Of course, the OR still needs to be fixed as I suggested.
--Jeff Moden
Change is inevitable... Change for the better is not.
November 11, 2008 at 5:50 am
hi jeff,
what do you think: if you have serveral parameters in the where clause, which can be choosen in every possible way (e.g. the interface allows you to pick: state, country, rating, zipcode between and, lastname,...) from the user interface and you can pick 2, 3, none whatever - is it better to
send all parameters to the sql server anyway and implement a lot of if's to check if the parameter was selected and then generate the sql string dynamically in the procedure?
i always generate the where clause in the UI and call a procedure, mentioned before (the user has only permisson to procedures. for sql injection he has to hack my admin account.)
whats your opinion?
Susanne
November 11, 2008 at 6:04 am
It seems you have huge data update, insert and delete in the table. Please try to re-organise the table index and the query will execution will be faster.
November 11, 2008 at 6:05 am
I probably wouldn't do a lot of "IF's" although those can be quite effective provided that it's not building dynamic SQL. A well formed WHERE clause with CASE statements does the trick in most cases.
--Jeff Moden
Change is inevitable... Change for the better is not.
November 11, 2008 at 6:13 am
Thanks Jeff, I tried the restructured WHERE clause, and there is no noticable difference in the time. Removing the DISTINCT keyword still causes it to return duplicates.
It seems that the dynamicSQL is the way to go, the question is how little. What would be the best way to validate the incoming values to make sure that thay are safe? If the input parameters are defined as int, how much damage can an attacker cause? Can SQL injection be accomplished if I validate the incoming values as int?
Patrick
November 11, 2008 at 9:21 am
Patrick Russell (11/11/2008)
It seems that the dynamicSQL is the way to go, the question is how little. What would be the best way to validate the incoming values to make sure that thay are safe? If the input parameters are defined as int, how much damage can an attacker cause? Can SQL injection be accomplished if I validate the incoming values as int?
If you parameterise the dynamic SQL and use sp_executesql, like I'm suggesting, there's no risk of SQL injection. The risk comes in when you concatenate pieces of SQL string together, like in Sue's example
-- assume 3 parameters to a proc that contains this code, @locSITEID, @locSHOWID and @locBUCKETTOISSUEID
DECLARE @sSQL nvarchar(2000), @WhereClause mVARCHAR(2000)
SET @sSQL = 'SELECT col1, col2, col3 FROM SomeTable '
IF @locSITEID IS NOT NULL
SET @WhereClause = 'SiteID = @SiteID '
IF LEN(@WhereClause) !=0
SET @WhereClause = @WhereClause + ' AND '
IF @locSHOWID IS NOT NULL
SET @WhereClause = 'ShowID = @ShowID'
IF LEN(@WhereClause) !=0
SET @WhereClause = @WhereClause + ' AND '
If @locBUCKETTOISSUEID IS NOT NULL
SET @WhereClause = 'BucketID = @BucketID'
IF LEN(@WhereClause) !=0
SET @sSQL = @sSQL + ' WHERE ' + @WhereClause
exec sp_executesql @sSQL, '@SiteID int, @ShowID int, @BucketID int', @SiteID = @locSITEID, @ShowID = @locSHOWID , @BucketID = @locBUCKETTOISSUEID
If you look at that, you'll notice that I'm not ever concatenating the value of a parameter into a string that will be executed. What I'm doing is building up a parameterised query based on the values of certain variables/parameters and then executing that and passing the parameters in. It requires the user to have rights on the base tables, but there's no chance of injection from that.
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
November 11, 2008 at 9:39 am
Sue (11/11/2008)
thanks for your hint, Jeff.but isn't this better than to give users permissions on tables and generate the whole select statement dynamically as mentioned before?
But that's pretty much what you're doing. Any form of dynamic SQL breaks the ownership chain and requires that the users have permissions on the base tables and having the entire where clause concatenated in is far more risky than building up a parameterised query piece by piece and executing that. What if your where clause read '1=1; delete from tblxy'
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
November 11, 2008 at 12:01 pm
thanks Gail. I see what you mean and will do it your way from now on. 🙂
sql injection: but first of all he has to know the name of the table and then he has no delete permissions to the table. but anyway, it is dangerous for sure.
Susanne
November 11, 2008 at 9:03 pm
Patrick Russell (11/11/2008)
Thanks Jeff, I tried the restructured WHERE clause, and there is no noticable difference in the time. Removing the DISTINCT keyword still causes it to return duplicates.It seems that the dynamicSQL is the way to go, the question is how little. What would be the best way to validate the incoming values to make sure that thay are safe? If the input parameters are defined as int, how much damage can an attacker cause? Can SQL injection be accomplished if I validate the incoming values as int?
Patrick
Gail pretty much summed it up.
Also, can you post your restructured WHERE clause, please?
--Jeff Moden
Change is inevitable... Change for the better is not.
November 11, 2008 at 10:02 pm
Sue (11/11/2008)
sql injection: but first of all he has to know the name of the table and then he has no delete permissions to the table.
Right, but counting on that is known as "Security through Obscurity" and it is generally understood to be a security fallacy (as in "Not secure"), because you are relying on the user's ignorance of things that you are not actually in a position to keep secure: like the names of your company's tables. You have effectively moved your security perimeter from things that you can control (like what SQL code gets executed on your server) to things you cannot control (like how many people know what your database schema is).
[font="Times New Roman"]-- RBarryYoung[/font], [font="Times New Roman"] (302)375-0451[/font] blog: MovingSQL.com, Twitter: @RBarryYoung[font="Arial Black"]
Proactive Performance Solutions, Inc. [/font][font="Verdana"] "Performance is our middle name."[/font]
November 12, 2008 at 12:37 am
rbarryyoung (11/11/2008)
Right, but counting on that is known as "Security through Obscurity" and it is generally understood to be a security fallacy (as in "Not secure"), because you are relying on the user's ignorance of things that you are not actually in a position to keep secure: like the names of your company's tables.
And it's not hard to find out the names of the other tables, especially if the web site/app has descriptive errors turned on. Find a place where any number of records are displayed and injection is possible and tag a "union all select name from sysobjects where xtype='U'" on the end. Tweak columns and positions until no error is returned.
With patience, it's even possible to work out most of the schema without using the system tables, just by paying attention to the errors returned. I have done that to a system (with permission). It's time consuming, and the automated attacks won't do it, but it certainly is possible.
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
November 12, 2008 at 6:32 am
Jeff Moden (11/11/2008)
Also, can you post your restructured WHERE clause, please?
Below is the code using the restructured WHERE clause you posted. In testing I saw no noticable difference; 8-10 seconds. And when I removed the DISTINCT keyword, It still returns duplicates.
SELECT DISTINCT ET.iId, ET.cName, C.cCategoryName
FROM tbl_ReportIssues RI
FULL OUTER JOIN tbl_EquipmentTypes ET
INNER JOIN tbl_Categories C
ON ET.iCategoryId = C.iCategoryId
INNER JOIN tbl_Issues I
ON ET.iCategoryId = I.iCategoryId
INNER JOIN tbl_EquipTypeToShow ETS
ON ET.iId = ETS.iEquipTypeId
INNER JOIN tbl_BucketsToIssues BI
ON I.iIssueId = BI.iIssueId
LEFT OUTER JOIN tbl_ReportIssueDetails RID
ON ETS.iEquipTypeId = RID.iEquipTypeId
ON RI.iReportIssueId = RID.iReportIssueId
AND RI.iBucketToIssueId = BI.iBucketToIssueId
WHERE
(
(ET.iSiteId = @locSITEID)
AND (ETS.iShowId = @locSHOWID OR @locSHOWID IS NULL)
AND (BI.bDeleted = 0)
AND (ETS.bDeleted = 0)
AND (BI.iBucketToIssueId = @locBUCKETTOISSUEID OR @locBUCKETTOISSUEID IS NULL)
)
OR
(
(ET.iSiteId = @locSITEID)
AND (ETS.iShowId = @locSHOWID OR @locSHOWID IS NULL)
AND (BI.iBucketToIssueId = @locBUCKETTOISSUEID OR @locBUCKETTOISSUEID IS NULL)
AND (RI.iReportId = @locREPORTID OR @locREPORTID IS NULL)
)
Patrick
November 12, 2008 at 6:17 pm
Patrick Russell (11/12/2008)
Jeff Moden (11/11/2008)
Also, can you post your restructured WHERE clause, please?
Below is the code using the restructured WHERE clause you posted. In testing I saw no noticable difference; 8-10 seconds. And when I removed the DISTINCT keyword, It still returns duplicates.
That because I apparently didn't have enough coffee when I made the suggestion... there is no difference in the code and I just didn't see it that way. Thanks and sorry about the bum answer.
--Jeff Moden
Change is inevitable... Change for the better is not.
Viewing 13 posts - 16 through 27 (of 27 total)
You must be logged in to reply to this topic. Login to reply