This thread is for discussions of To SP or not to SP in SQL Server.

When a stored procedure is called directly from the code on the web server, it DOES prevent SQL injection attacks.  When you call a command that uses an EXECUTE statement, you might as well be performing an ad-hoc query.  Stored procedures are not meant to be called this way from the code on your web server.  Call the stored procedure directly.

Here are some examples that show how SPs can prevent SQL injection attacks.

For the following examples, let's make a few assumptions:

1.  The findtitle SP contains the SQL statement SELECT * FROM books WHERE title = @title.
2.  A hacker entered 'DROP TABLE books into the textbox.

Your previous example:
Code:

strsql = "EXECUTE findtitle '" & textboxtitle.text & "'"
objCmd = New SqlCommand(strSQL, objConn)

In this example, the code would evaluate this way:
strsql = "EXECUTE findtitle ''DROP TABLE books"

Here, the code would execute the SP (which would find no titles equal to an empty string), and then would drop the table books.  NOT GOOD.

My suggested way of calling the stored procedure:
Code:

string title = String.Empty;

SqlConnection conn = new SqlConnection("Your connection string goes here");
//Calls the stored procedure
SqlCommand cmd = new SqlCommand("findtitle", conn);

//The second and third parameters within the SqlParameter constructor will be dependent on the parameter within the stored procedure, the values supplied below are for example purposes
SqlParameter param = new SqlParameter("findtitle", SqlDbType.VarChar, 50, ParameterDirection.Input, false, ((System.Byte)0), ((System.Byte)0), null, DataRowVersion.Current, textboxtitle.text);
cmd.Parameters.Add(param);

SqlDataReader dr = cmd.ExecuteReader(CommandBehavior.CloseConnection);

try
{
   while(dr.Read())
   {
       title = dr["title"].ToString();
   }
}
catch (Exception)
{
   //Perform error code here
}
finally
{
   //Close data reader.  Connection will be closed simultaneously because of execute reader parameter
   if(dr != null)
   {
       dr.Close();
   }
}

//Write the retrieved value to a label on the form.
lblTitle.Text = title;

In my example above, by calling the stored procedure directly and passing the value of the textbox as a SqlParameter object, the potential SQL injection attack is eliminated.  This is because the quote and following SQL statement are viewed as a parameter value instead of T-SQL.  The SP would be evaluated this way:

SELECT * FROM books WHERE title = "'DROP TABLE books"

Because there is no title with the value 'DROP TABLE books, you would receive an empty set.  The single quote in this example is included in the value to search for.  In this example, no hacker code is executed and all tables are intact.

In the previous example, the single quote is evaluated as code, instead of a value, and completes the SELECT SQL statement.  Once that statement is completed, the hacker can add any SQL code they want to be executed.  In the previous example, the books table is dropped.

So, as you can see from the examples above, SP can prevent SQL injection attacks if executed the way they were intended.

I accept that this is susceptible for the SQL injection attack.
But you have to check the text box string before passing to the stored procedure.
It means that, make sure that there is no illegal character in the text box.

You don't have to check the textbox for illegal characters if you use stored procedures correctly.  See post above.

When developing a data access method you generally write some sort of skeleton code that is just enough to get data in and out of your database, knowing that later on you will comeback and add error checking, logging and so on. If you do this in SP's then you don't have to recompile to see it work. You simply change your SP and you're off!

Now to your point about using SP's only if you are sticking with MSSQL, I think that's a misconception too. If you design your application with the proper separation, then your Data Access Layer should be an abstract pluggable provider. That way, if you want to use ORM on ORACLE instead of SP's along with the SqlClient class then you just write a different provider and your app adapts.

Of course there is overhead in creating the provider model for your DAL, so I would only suggest this there is a chance your app will run under something else.

And the more I think about it, the more I am convinced that changes can also be handled at the DAL level. I always write my DAL as a separate assembly. I always construct the methods to return void, a DataSet or a single native type. Therefore, as long as your signatures remain simple and consistent and you don't return custom entities or SqlClient based types, then you can write a different DAL that accesses your data however you want.

You've done two important things (or more)

1. You've proven that it takes a little more know-how to write code that prevents sql injection - as noted in the article

2. You've added a bunch of lines of code (which isn't optimised) to refute a simple example that the article was trying to convey.

Thanks for the tutorial, but that wasn't the point being made... It was simply that this is how a developer can really screw up... and all the while thinking "I'm safe, because I'm 'using' a stored procedure"

Orm.Net (and others like it) sort of takes the Ad-Hoc queries to the next level - basically they allow you to do a Neo-esque automated generation of the queries simply by reading the DB schema. So a developer runs the tool and gets code objects to work with - and the objects have all the necessary ad-hoc queries within them, so the developer doesn't have to touch sql strings (queries or stored proc names). BTW: ORM.Net also gives you access to stored procs if there are any in the DB...

Now that argument about maintainability is reduced down to size. It goes: "you can make a lot of changes that break a lot of things - which might be hard to find in the code... blah blah blah." - As noted in the article, you might have to do some code updates and a rebuild anyway - depending on how extensive the DB changes are - even when using straight stored procs.

All the developer has to do is re-run the tool (It connects to the updated DB and updates the objects based on the new schema). Then he does a quick rebuild.

Notes:
1. If changes were made to the DB that have nothing to do with the code, then this rebuild is not necessary.
2. Sometimes satelite DB changes require a code rebuild - usually because the developer grabbed everything from the DB to build the objects, but then didn't use all the objects that were created. The rebuild fixes this automatically, but it's better to tell your tool to ignore those entities (i.e. tables) that it doesn't need in the first place.
3. If stuff in the DB that's being used by the app change - and the objects now fail to build, then since you are using objects and not SQL strings, the build should fail exactly where it needs to be fixed. This kind of build failure speeds up the refactoring process - which would have to occur anyway for the code to take advantage of the "DB enhancements"

Very nice article - and honest! You might want to address the use of these kinds of tools though...

Quote:

I have a number of situations in which users can have access to only certain rows of a database. The rules covering which rows a user can access are often complex and best managed in procedural code, or at least not easily handled in straight SQL Set-based code. In these situations, placing this logic in an SP (and denying any access to the bare tables) is very helpful.

Actually there is a much easier way to implement row-based security using joins to a security table that has at least values for the SecurityPrinciple  and the KeyValue from the table.  A simple join to the table, encapsulated in a view and you can use normal access outside of here (generally this is better done on a domain value so it is not terribly tedious, but either way it is done naturally using SQL)  It is what one might do on the front end using programmed logic, but far easier to administrate (since security changes!)

Quote:

One wild card here is that in some organizations developers are not allowed to manipulate SPs or other objects on the SQL Server directly. In this case, for non-technical reasons, SPs are not a good option.

This is not a wild card, it is a weird case of poor management.  If a programmer can generate code "on the fly" using client code, it is ignorant to prevent them doing the same thing on the server in code.  Obviously there are some major differences in coding procedures versus functional coding but (and here is the great thing) if the proc takes the right parms, and it returns the right data, it can be optimized.  Obviously don't wait until your newbie programmer has written 500 procs using cursors, but it doesn't have to be an us vs them mentality.

Quote:

Trust me, even though this hapless programmer is using stored procedures, the application is susceptible to SQL injection attacks.

This is very true. Either method is no better or worse for injection attacks.  It just takes some intelligence to deal with it.  Check out http://www.sommarskog.se/dynamic_sql.html for more information

Quote:

The important thing to note about this is that there is no “compiling” of SPs in the traditional sense. In some environments (Oracle, I am told, and in a number of other databases as well), SPs really are converted to a different programming language (such as C or C++) and compiled to native code. This is not the case in SQL Server, for SPs or ad-hoc SQL. SPs have a slight advantage in that the execution plan can be cached and found a bit more efficiently.

The compiling issue is a non-starter.  The cost of compilation is so minimal as to not be a real concern unless you are executing thousands of times a second.  However, the cost of building a query plan is the real cost.  The problem with ad hoc access is that unless you are looking at a single table in your query, the plan will not be parameterized.  For example run:

set showplan_text on
go
select    customers.customerid, orders.freight
from    customers
    join orders
       on customers.customerid = orders.customerid
where customers.customerid = 'vinet'

 |--Nested Loops(Inner Join)
      |--Clustered Index Seek(OBJECT:([Northwind].[dbo].[Customers].[PK_Customers]), SEEK:([Customers].[CustomerID]='vinet') ORDERED FORWARD)
      |--Bookmark Lookup(BOOKMARK:([Bmk1002]), OBJECT:([Northwind].[dbo].[Orders]))
           |--Index Seek(OBJECT:([Northwind].[dbo].[Orders].[CustomerID]), SEEK:([Orders].[CustomerID]='vinet') ORDERED FORWARD)

In the plan you will see the literal value 'vinet' in there.  This means that this plan is only good if some other user happens to run the exact same query (including spacing and capitalization) with the same literal.  Far better to use a procedure because you control the parameter.

Quote:

As far as maintenance goes, regardless of whether you use ad-hoc SQL or SPs, an unskilled person will create a maintenance nightmare; a skilled practitioner will do a great job.

Fundamentally this is true.  When you need to make a change to a query and it requires a complete recompile of the client code and hence a heavy bit of testing to redeploy to 100 customers (or to 100 users in your organization) it becomes more of a nightmare than it needs to be.

Quote:

So, should you use SPs or ad-hoc SQL? The answer is “it depends.” I have placed myself firmly on the side of doing all database access through SPs. I do so knowing that I am not getting any unique security benefits using SPs, knowing that the performance benefits are not as clear cut as I once might have thought (but are still real in some cases), knowing how to leverage SPs to minimize the maintenance load, and understanding that I am more tied to SQL Server than I might be if I were to use ad-hoc SQL. What do you think?

I think you are pretty much right on, though you do get security benefits if you follow through on the entire plan for building the application right, there is a benefit. If you use a proxy account to do security, you are right, the security benefits are far less.  Performance is greatly enhanced, but not to a level that you will notice for most applications with smaller numbers of users.  As concurrency needs rise however, stored procedures are of great benefit (though SQL Server does a dandy job of assisting the architect who doesnt care.

You are more tied to SQL Server in some ways, but less in others.  I am not an expert in any other dialects, but the king of the use standard SQL or you are stupid clearly favors using stored procedures (newsgroup thread: (http://groups.google.com/group/microsoft.public.sqlserver.programming/browse_frm/thread/41b94165a9d7f1ca/840120292a4a1168?lnk=st&q=joe+celko+stored+procedures&rnum=3#840120292a4a1168))


For my original rebuttal to the Bouma article: (http://spaces.msn.com/members/drsql/Blog/cns!1pqLLlQgw9hZKLV1K4eLatKw!273.entry)