how to prevent SQL Injection

Question

I am using stored procedures. In order to save time, I made some generic procedures that uses dynamic sqlin order to update. Such generic procedure is:

CREATE PROCEDURE [dbo].[SetField]
 @company_id uniqueidentifier,
 @id bigint,
 @field_code nvarchar(50),
 @value nvarchar(50)
AS
BEGIN
 DECLARE @field_name nvarchar(50)
 SET @field_name = NULL
 SELECT @field_name=field_name
 FROM dbo.FIELD_DEFINITION
 WHERE field_code=@field_code

 IF @field_name IS NOT NULL
 BEGIN

  IF @value IS NULL OR @value=''
  BEGIN
   SET @value='NULL'
  END
  ELSE
  BEGIN
   IF @field_code='START_DATE' OR @field_code='END_DATE'
   BEGIN
    SET @value = CONVERT(datetime, @value ,103)
   END
   SET @value=''''+@value+''''
  END

  DECLARE @sql nvarchar(1000)
  SET @sql = 'UPDATE dbo.TABLE '+
     'SET '+@field_name+'='+@value+' '+
     'WHERE company_id=''' + CAST(@company_id as nvarchar(36)) + ''' AND '+
     'id='+CAST(@id as nvarchar)
  EXEC(@sql)
 END
END

How can I prevent sql injection with this code?

Answer 1

You said:

In order to save time, I made some generic procedures that uses dynamic sql in order to update

If you'd asked first, we could have saved time and suggested this...

UPDATE
    dbo.TABLE
SET
    Field1 = CASE WHEN @field_name = 'Field1' THEN @value ELSE Field1 END,
    Field2 = CASE WHEN @field_name = 'Field2' THEN @value ELSE Field2 END,
    Field3 = CASE WHEN @field_name = 'Field3' THEN @value ELSE Field3 END,
    ...
    Fieldn = CASE WHEN @field_name = 'Fieldn' THEN @value ELSE Fieldn END
WHERE
    company_id = @company_id AND id = @id

Answer 2

The important aspect to remember about SQL injection is that means that, if at all possible, you should never embed user-supplied values directly into your SQL. This doesn't mean that you can't use dynamic sql (though it definitely makes things easier if you don't), but it does become more dangerous at times.

In your specific example, you can keep the parameterization of everything except @field_name. This, unfortunately, must be embedded directly into the SQL; everything else can be passed as a parameter again, so there's no need to worry about their content.

The safest thing that you can do in this specific example is the following:

if(exists (select 1 from INFORMATION_SCHEMA.Columns where TABLE_NAME = 'Table' and TABLE_SCHEMA = 'dbo' and COLUMN_NAME = @fieldName))
begin
    DECLARE @sql nvarchar(1000)
    SET @sql = 'UPDATE dbo.TABLE '+
       'SET ' + QUOTENAME(@field_name) + '=@value ' + 
       'WHERE company_id=@company_id AND '+
       'id=@id'

    exec sp_executesql @sql,N'@id bigint, @company_id uniqueidentifier, @value nvarchar(50)',@id,@company_id,@value
end

This does two things:

1. It verifies that there is actually a column with that name in the table. If the user were to embed any other SQL statements into the field, then this check would fail and the statement would not be executed. You could also call raiseerror to report the error, but I'll leave that exercise up to you.
2. It encloses the field name in square braces so that field names that contain spaces or reserved words will not break the statement. This may not be an issue for you, but it's always good practice if you're generating the SQL yourself.

Answer 3

I would start looking at ways to prevent sql injection attacks before you ever call the SP. Be careful with dynamic SQL strings pieced together from querystring or form data. Use a SqlCommand object.

Edit: In response to the comment, here is a nice explanation of how parameterized queries (SqlCommand queries) help prevent SQL injection.

From http://forums.asp.net/t/1568268.aspx:

...The placeholder - @Id - has become part of the hardcoded SQL. At runtime, the value provided by the querystring is passed to the database along with the hardcoded SQL, and the database will check the ProductID field as it attempts to bind the parameter value to it. This ensures a level of strong typing. If the parameter value is not the right type for the database field (a string, or numeric that's out of range for the field type), the database will be unable to convert it to the right type and will reject it. If the target field datatype is a string (char, nvarchar etc), the parameter value will be "stringified" automatically, which includes escaping single quotes. It will not form part of the SQL statement to be executed.