- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Hopefully this isn't too late for you to see it, but the obvious query (to exactly preserve the content of current results) is shown below. There is no need for ISNULL or CASE, and they don't actually seem appropriate here, unless a change in logic is required.
This would almost certainly result in a table scan, but the current solution does that and then some. If you wanted to be able to make use of indexes for particular search criteria, you would need to generate the WHERE clause on the fly.
Admin
Before relying on that method you might want to check it.
DECLARE @sProjectID AS VARCHAR(32)
SELECT LEN(@sProjectID)
-----------
NULL
(1 row(s) affected)
Admin
"Clever irony" or just plain stupid? That's some scary lookin' SQL my friend! Then again, I guess the beauty of code like this is that you don't have to worry about bothersome database tasks like setting up indexes or relations or anything like that ...
Admin
Warning; if "param1" is coming straight from the outside world, you're ripe for an SQL injection attack. Stop it now.
Admin
rsynnott --
No, it is NOT. That is completely wrong, there is absolutely no danger at all of sql injection. It is scary how many people really have no idea what sql injection truly means.
The biggest issue with the code shown, as myself and others explained, is the performance.
Typically, for dynamic criteria on a varchar() column, I tend to prefer:
WHERE Column LIKE COALESCE(@Value,'%')
which in many cases performs better than using an OR.
Admin
Why is this SQL statement so "scary?" It doesn't seem all that complicated to me. The only thing that looks a little scary to me are some of the where conditions. But in this case, he was just copying those conditions from the original post.
Anyway, I'm not trying to be snide; I'd just like some clarification if you care to give it.
Admin
Um.
Yes it is.
Anytime you simply concatenate a value onto a SQL command string you subject yourself to an injection attack. That's the basis of an injection attack. That's it. No more, no less. The amount of damage that can be done varies based on the setup.
If param1 is a value taken from the outside world, stored in a local variable, and you simply concatenate it to a SQL string, you're setting yourself up for a problem.
http://www.somesystem.com/listproducts.asp?productid=regular
You take the query string, pull out the product id of regular, stuff it in param1, and use it to create a SQL statement.
Sql = "SELECT * FROM Products WHERE CategoryId = '" & param1 & "'"
Sql is now equal to "Sql = "SELECT * FROM Products WHERE CategoryId = 'regular'"
It works. You get a page of products.
Joe Blow comes along and modifies your URL.
http://www.somesystem.com/listproducts.asp?CategoryId=regular' or 1=1--
You take the query string, pull out the product id of regular, stuff it in param1, and use it to create a SQL statement.
Sql = "SELECT * FROM Products WHERE CategoryId = '" & param1 & "'"
Sql is now equal to "SELECT * FROM Products WHERE CategoryId = 'regular' or 1=1--'
Now the page is listing all products, regardless of category, because 1 always equals 1.
That's not SQL injection to you?
Sql.Command = "SELECT * FROM Products WHERE CategoryId = @Param1";
Sql.Parameters.Add(new SqlParameter("@Param1", Param1));
Allows you to use the parameter value obtained from the user without risk of injection, yes.
But concatenating a value to a dynamic SQL string exposes you to SQL injection.
Period.
If you want a "real" example...
http://www.somesystem.com/listproducts.asp?CategoryId=regular'; exec master..xp_cmdshell 'ping 1.2.3.4'--
Sql is now equal to Sql.Command = "SELECT * FROM Products WHERE CategoryId = regular'; exec master..xp_cmdshell 'ping 1.2.3.4'--
Since SQL Server runs by default as the local SYSTEM account you have access to xp_cmdshell which allows you to remotely execute any command you want. Using ; ends the first command and executes the second.
I don't know what you think SQL injection is, but I think you're the one who's either wrong, or not explaining himself very well.
Admin
I can't edit the last post.
A white paper on SQL Injection by Spidynamics: http://www.spidynamics.com/whitepapers/WhitepaperSQLInjection.pdf
Good read for those who want to learn more.
Admin
The problem is all the poster said is they do
LIKE '%' + @somevar + '%'
that's all they indicated; they did not indicate that they are building sql strings dynamically. You cannot assume that, and to see a snippet of code like that to assume it is a dynamically executed SQL statement is wrong. That is like saying anytime you concatenate any variables with constaints in any SQL statment it is open to injection; wrong! The key is the dynamic execution of a SQL string.
SELECT * FROM TABLE WHERE Col1 LIKE '%' + @SomeVar + '%'
is not open to sql injection unless it is dynamically executed.
Admin
Didn't we just go over this? :)
As far as I know you're certainly correct, but I think that Mr. Jeff S. is assuming that this is not a dynamically created SQL statement that gets executed all willy nilly.
Admin
Dang, insta-posted.
Admin
I guess you may be right; he is just copying the WHERE clause as verbatim as possible from the original poster's code. So, while it is still a horribly constructed conditional WHERE (it cannot use any indexes at all and it is and also wrong -- Len(null) = Null) I should not have assumed the poster actually might have thought it was good code.
Admin
Gotcha.
This is safe, not subject to SQL injection:
CREATE PROCEDURE [dbo].[TestProc] ( @TestParameter AS varchar(240)) AS SELECT * FROM SampleTable WHERE SampleColumn LIKE '%' + @TestParameter + '%'
GO
This is not safe, subject to SQL injection:
CREATE PROCEDURE [dbo].[TestProc] ( @TestParameter AS varchar(240)) AS EXEC('SELECT * FROM SampleTable WHERE SampleColumn LIKE ''%''' + @TestParameter + '''%''') GO
I would like to note that both methods are subject to SQL injection if the SQL statement is prepared on the client as a string, which is then sent to the server for processing. (This methodology is how most SQL injection attacks are created.)
My apologies Jeff.
Admin
If you read my original post, I was showing how you could replace the body of the stored procedure with a single SELECT statement which generates results identical to the current stored procedure. Your suggested "fix" would change existing logic. It is a major WTF to assume you know better without checking that against user expectations.
I also pointed out in that message that as written, it would not use any indexes, and what to do if you wanted that.
Please read the posts before you respond to them.
As far as how I tend to deal with such issues, I tend to write a framework which builds up the WHERE clause at run-time, so that it will use available indexes. I've made a career out of writing frameworks for doing that.
Admin
I already indicated that I was probably wrong about your post (in the very post you quoted); maybe YOU should read the posts a little more closely too.
Admin
because you need to store a row in the DB that indicates (a) to whom the mail was sent and (b) what the contents of the email were.
in other words, sending it using C#, or within the "middle tier" may mean you have to call back to your DB and INSERT a record into your SENT_EMAILS table... there's no need to bring that logic outside of the DB.
why store records of sent emails?
so you can later run queries on them and determine things like reply rates, and patterns in customer support, or how many people from Boise bought your product.
Admin
The way MS SQL does equals, the LTRIM and RTRIM are not necessary.
Admin
So many people have commented so thoroughly on the big WTFs here, but I'd like to mention a small one. The code assumes the project ID it is passed might be space-padded, and the value in the DB might be as well. Neither of these things is likely to be true, and both could be guaranteed to NOT be true, if the rest of the app took the effort to do so. And basing the WHERE condition on a function like this will keep the DB from using an index on the sProjectID column, forcing a full table scan and massively slowing down the query. (A poster remarked that the way SQL Server handles the equals operator makes this use of LTRIM and RTRIM unnecessary. Is that true? I don't know, I'm an Oracle developer. (So be nice to me.))
On the other issues that have been raised, I would say 1. Use bind variables; 2. Use bind variables; and 3. Even if you use dynamic SQL to include or exclude terms in the WHERE clause for optional parameters, use bind variables anyway for the values.
Admin
Trailing spaces do not matter in T-SQL, but leading spaces do.
The implied WTF with using LTRIM(TRIM()) is that your database is storing ProjectID values (which are presumably important and should have foreign key references) inconsistently and in a manner in which you cannot index them or use foreign key constraints.
Trimming the input it one thing, but needing to trim the values in the database for leading spaces on primary key columns???? *gasp*
Admin
Admin
It doesn't really have a race condition because each calls creates a separate, isolated temporary table for individual fsckwittery.
Admin
I wonder if this is a case of shifting understanding.
I once had to implement an 'intelligent' search that took between 1 and 50 different criteria, sorting the results by exact, close and possible matches. This was impossible as a WHERE clause; it used temporary tables, cursors and calculated 'match' vectors.
As more of the function was implemented (and the requirements expanded), the stored procedure became so complicated that we re-wrote it in a far simpler form in the middle layer. The SP kept shrinking until only the cursor was left and it looked quite like the offending one in this article. It stayed that way for several months before someone noticed and replaced it with 'SELECT * FROM .. WHERE ..'
It's a possibility...