Safe from SQL injection?

Is the following style of coding safe from sql injection or not?

        public int Insert(Category entity)
        {
            string query = string.Format
                (
                "insert into categories (sequence, title, description, enabled, visible) values ('{0}', '{1}', '{2}', '{3}', '{4}')",
                entity.Sequence,
                entity.Title,
                entity.Description,
                entity.Enabled,
                entity.Visible
                );
            int affected = 0;
            using (SqlConnection connection = new SqlConnection(connectionString))
            {
                connection.Open();
                using (SqlCommand command = new SqlCommand(query, connection))
                {
                    affected = command.ExecuteNonQuery();
                }
                connection.Close();
            }
            return affected;
        }

No

@dhtmlgod

I’m trying to refresh my ASP and .NET skills, and reading other’s threads is one way that helps me learn. Can you please expand your answer a bit. I’m not interested in a code hand-out, but a pointer in the right direction would be appreciated.

If you want it to be sql injection safe, you need to use parameters. Just inserting it like that will still allow it to be injected.

Say entity.Title is: “’ drop table Entity”

I hope that answers ur question Mittineague.

Yes, a big help, thanks. I’m familliar with parameters in PHP’s “improved” MySQL, so I should be able to figure it out easy enough.

… off to RTFM …

How does using an SqlParameter to set the value make it any more or less safe? It’s still just setting a string value isn’t it?

P.S. I read the above link, and still don’t understand.

With parameters you do not add add ’ marks around ur vars. And the sqlcommand filters it out and protects it from sql injection. There are ways to protect you doing it like that. Like doing this: Entity.Title.Replace(“'”,“‘’”);

But that can become cumbersum and is just better left to the compiler handling that

This post might help some more: http://weblogs.asp.net/scottgu/archive/2006/09/30/Tip_2F00_Trick_3A00_-Guard-Against-SQL-Injection-Attacks.aspx


void Main()
{
 
 using(var connection = new SqlConnection(@"..."))
 using(var command = new SqlCommand(string.Format(@"select * from Log where Id = {0}", "1; select * from Log"), connection))  // command with 2nd statement injected.
 {
  connection.Open();
  using(var reader = command.ExecuteReader())
  {
   reader.Dump(); // returns two result sets !!
  }
 }
 
 /*
  command sent to sql server  
  select * from Log where Id = 1; select * from Log
 */
 
 
 using(var connection = new SqlConnection(@"..."))
 using(var command = new SqlCommand(@"select * from Log where Id = @id", connection)) 
 {
  command.Parameters.Add("@id", SqlDbType.Int, 4).Value = "1; select * from Log"; // Typed parameter -- not int, will break with a .net exception
  connection.Open();
  using(var reader = command.ExecuteReader())
  {
   reader.Dump();
  }
 }
 
 using(var connection = new SqlConnection(@"..."))
 using(var command = new SqlCommand(@"select * from Log where Id = @id", connection)) 
 {
  command.Parameters.Add("@id", SqlDbType.NVarChar, 100).Value = "1; select * from Log"; // Untyped (string) parameter -- not int, will break with a sql excecption 
  connection.Open();
  using(var reader = command.ExecuteReader())
  {
   reader.Dump(); // 'Conversion failed when converting the nvarchar value '1; select * from Log' to data type int.'
  }
 }
 */
 /* 
  command sent to sql server --  
  exec sp_executesql N'select * from Log where Id = @id',N'@id nvarchar(100)',@id=N'1; select * from Log' 
 */
 
 using(var connection = new SqlConnection(@"..."))
 using(var command = new SqlCommand(@"select * from Log where Message like @message", connection)) 
 {
  command.Parameters.Add("@message", SqlDbType.NVarChar, 100).Value = "%No%; select * from Log"; // only "%No%" should return 3 records
  connection.Open();
  using(var reader = command.ExecuteReader())
  {
   reader.Dump(); // returns only one result set with no records!!
  }
 }
 
 
 /*
  command sent to sql server --  
  exec sp_executesql N'select * from Log where Message like @message',N'@message nvarchar(100)',@message=N'%No%; select * from Log'
 */
 
}
 
 

paramerterized queries make use of sp_executesql that does the escaping for you…

exactly as pufa and I said. Let sql do it for you. No need to do it all urself in ur code. It is just gona add unnecessary bulk to your code. Its kinda like removing the ValidateRequest from your page and then you stripping out all the html urself.

Parameterized queries help alot–they cover the typical script kiddie way of stuffing. They do not prevent every method of sql injection, especially if your parameterized queries are just parameterizing stuff fed to a stored proc which dynamically generates SQL.

I’d also contend that, for the most part, one should no longer be writing SQL these days.

Thanks, sql injection is a a lot clearer now. =)

Additionally, the following should then be safe (as can reasonably be expected anyway) since it uses params?

public int Delete(Category entity)
{
int affected;
string query = “delete from categories where id = @id;
using (SqlConnection connection = new SqlConnection[FONT=Consolas][SIZE=2][FONT=Consolas]SIZE=2)
{

connection.Open();

[/SIZE][/FONT][/SIZE][/FONT]using (SqlCommand command = new SqlCommand[FONT=Consolas][SIZE=2][FONT=Consolas][SIZE=2](query, connection))
{

command.Parameters.Add([/SIZE][/FONT][/SIZE][/FONT]new SqlParameter[FONT=Consolas]SIZE=2
{
ParameterName =
[/SIZE][/FONT]@id,
Direction =
ParameterDirection.Input,
DbType =
DbType[FONT=Consolas][SIZE=2][FONT=Consolas][SIZE=2].Int32,
Value = entity.Id
});

affected = command.ExecuteNonQuery();

}

connection.Close();
}
[/SIZE][/FONT][/SIZE][/FONT]return[FONT=Consolas][SIZE=2][FONT=Consolas][SIZE=2] affected;
}

[/SIZE][/FONT][/SIZE][/FONT]

Yes, that is fine and will be safe

Really? Because I find nothing wrong with LINQ2SP’s. I hate it when people assume “the latest and greatest” will solve your problems better then what is already tried and true.

I get it that new technology, Entity Framework, is going to solve all of your problems and make you a peanut butter and jelly sandwich all at the same time, but enough with the elitist attitude. The people who rag on SQL are the ones who suck at T-SQL.

What a weak statement to make. “You shouldn’t write SQL”. What happens if you need something far more complex then a simple/moderate query? What about DB maintenance? Triggers? Views? Functions? Their are far more things to T-SQL then SP.

To quote Nightstalker, from another thread:

LINQ gets converted to TSQL on the fly. So at the end it pretty much the same thing. Linq just makes it easier for you, and is very clever and usually generates TSQL better than most developers and can sometimes even perform better that a Sproc that is written badly.

…and so does NHibernate and Linq To Entities. They all just transform your code into t-sql. Here’s the question then: are we just getting lazy and justifying it by declaring that all databases should now be considered storage only?

Further, why does storage only have to be equivalent to table only? Stored Procedures, triggers, and other things can go a long way toward maintaining a db without injecting actual business logic. Most of it is just common sense things. Remember, there are people who like to tinker with things. Having safeguards within the db itself helps protect it, in addition to making certain queries easier.

Ponder…

No, we’re not getting lazy. By decoupling our domain from the storage medium, we stop thinking of our domain as simple anaemic data containers. I’ve found working without a database makes things easier to create and change and can really reduce the amount of time it takes to develop an application. That’s not lazy, it directly impacts a companies bottom line and that’s a good thing. And who wants to worry about stored procedures, T-SQL, etc. 99.9% of the time they are not the problem domain and are tools and code that need maintained that you can avoid.

That’s very interesting because I’ve found the opposite to be true. In all the projects I’ve started, with the intent of using DDD, I’ve found them extremely hard to visualize, and in one case, completely impossible. In every case so far, I’ve reverted back to the anemic data model where the domain (if you can call it that at this point) lives within the logic contained in the services.

I agree with wwb on this one. ORMs are the way of the future. When it comes to complex queries, the ORM can do that as well. Y not just embrace the future technology and learn it instead of sticking to old technologies. And saying that wwb’s statement is weak, is rather the opposite. Staying with tech you know and not evolving with the tech, is weak IMHO.

That is like saying people that code in .net 4.0 sucks at .net 1.1. And y do they move to new tech when there is tried and tested techs like 1.1. It is time to move on