views:

402

answers:

11
public static bool TruncateTable(string dbAlias, string tableName)
{
    string sqlStatement = string.Format("TRUNCATE TABLE {0}", tableName);
    return ExecuteNonQuery(dbAlias, sqlStatement) > 0;
}
+1  A: 

Have a look at this link

Does this code prevent SQL injection?

Remove the unwanted from the tableName string.

I do not think you can use param query for a table name.

astander
+2  A: 

Use parameterized queries.

Crowe T. Robot
From a table name? Do you have some links to support this?
astander
A strangely correct but poor answer. A small example based on the OP code would be way better.
AnthonyWJones
True, but people really should understand the dangers of letting ANY string be passed into a query without using parameterized queries.I didn't have the time to write up a code example, but he should know about them.
Crowe T. Robot
+2  A: 

Use a stored procedure. Any decent db library (MS Enterprise Library is what I use) will handle escaping string parameters correctly.

Also, re:parameterized queries: I prefer to NOT have to redeploy my app to fix a db issue. Storing queries as literal strings in your source increases maintenance complexity.

David Lively
If you don't want to redeploy, then do it correctly in the first place and test it.
Joe Philllips
Why didn't I think of that. While I'm at it, I'll just stop putting in all of those stupid bugs that I've been coding all these years. Also, genius, *deploy* can mean a lot of things - like going from local box to dev server. Interrupting other developers by rebuild/redploy to dev can be a pretty big disruption.
David Lively
It can also be disruptive to deploy a new revision of a stored procedure.
Bill Karwin
"Use a stored procedure" does not address the SQL injection risk. It's just as easy to execute an unsafe dynamic SQL query in a stored procedure as it is in application code.
Bill Karwin
Whatever. It addresses it as much as using parameterized queries does.
David Lively
That much is true.
Bill Karwin
A: 

You could use SQLParameter to pass in tableName value. As far as I know and tested, SQLParameter takes care of all parameter checking and thus disables possibility of injection.

Vladimir Kocjancic
A: 

I don't recall the exact syntax but use parametrized queties

public static bool TruncateTable(string dbAlias, string tableName)
{
    SqlCommand command = new SqlCommand("TRUNCATE TABLE @tableName");
    command.Parameters.AddWithValue("@tableName", tableName);            
    return command.ExecuteNonQuery();
}
Chad
-1 AFAIK, This doesn't work. You can't use a parameter to specify a table name in Oracle or Sql Server.
John Buchanan
A: 

If you can't use parameterized queries (and you should) ... a simple replace of all instances of ' with '' should work.

string sqlStatement = string.Format("TRUNCATE TABLE {0}", tableName.Replace("'", "''"));
wgpubs
hardly, what about '--'
Chad
you're right. there are actually a number of things that need to be replaced (e.g. ' ; --, /* */ xp_, etc...) and thus my recommendation to use a parameterized query (or any other valid option, e.g. stored proc, ORM, etc...). still, along with constraining the length of the string "tableName" ... this solution would probably work 100% of the time.
wgpubs
+6  A: 

As far as I know, you can't use parameterized queries to perform DDL statements/ specify table names, at least not in Oracle or Sql Server. What I would do, if I had to have a crazy TruncateTable function, that had to be safe from sql injection would be to make a stored procedure that checks that the input is a table that is safe to truncate.


-- Sql Server specific!
CREATE TABLE TruncableTables (TableName varchar(50))
Insert into TruncableTables values ('MyTable')

go

CREATE PROCEDURE MyTrunc @tableName varchar(50)
AS
BEGIN

declare @IsValidTable int
declare @SqlString nvarchar(50)
select @IsValidTable = Count(*) from TruncableTables where TableName = @tableName

if @IsValidTable > 0
begin
 select @SqlString = 'truncate table ' + @tableName
 EXECUTE sp_executesql @SqlString
end
END
John Buchanan
+1  A: 

There are some other posts which will help with the SQL injection, so I'll upvote those, but another thing to consider is how you will be handling permissions for this. If you're granting users db+owner or db_ddladmin roles so that they can truncate tables then simply avoiding standard SQL injection attacks isn't sufficient. A hacker can send in other table names which might be valid, but which you wouldn't want truncated.

If you're giving ALTER TABLE permissions to the users on the specific tables that you will allow to be truncated then you're in a bit better shape, but it's still more than I like to allow in a normal environment.

Usually TRUNCATE TABLE isn't used in normal day-to-day application use. It's used for ETL scenarios or during database maintenance. The only situation where I might imagine it would be used in a front-facing application would be if you allowed users to load a table which is specific for that user for loading purposes, but even then I would probably use a different solution.

Of course, without knowing the specifics around why you're using it, I can't categorically say that you should redesign, but if I got a request for this as a DBA I'd be asking the developer a lot of questions.

Tom H.
+3  A: 

If you're allowing user-defined input to creep into this function via the tablename variable, I don't think SQL Injection is your only problem.

A better option would be to run this command via its own secure connection and give it no SELECT rights at all. All TRUNCATE needs to run is the ALTER TABLE permission. If you're on SQL 2005 upwards, you could also try using a stored procedure with EXECUTE AS inside.

CodeByMoonlight
+19  A: 

The most common recommendation to fight SQL injection is to use an SQL query parameter (several people on this thread have suggested it).

This is the wrong answer in this case. You can't use an SQL query parameter for a table name in a DDL statement.

SQL query parameters can be used only in place of a literal value in an SQL expression. This is standard in every implementation of SQL.

My recommendation for protecting against SQL injection when you have a table name is to validate the input string against a list of known table names.

You can get a list of valid table names from the INFORMATION_SCHEMA:

SELECT table_name 
FROM INFORMATION_SCHEMA.Tables 
WHERE table_type = 'BASE TABLE'
  AND table_name = @tableName

Now you can pass your input variable to this query as an SQL parameter. If the query returns no rows, you know that the input is not valid to use as a table. If the query returns a row, it matched, so you have more assurance you can use it safely.

You could also validate the table name against a list of specific tables you define as okay for your app to truncate, as @John Buchanan suggests.

Even after validating that tableName exists as a table name in your RDBMS, I would also suggest delimiting the table name, just in case you use table names with spaces or special characters. In Microsoft SQL Server, the default identifier delimiters are square brackets:

string sqlStatement = string.Format("TRUNCATE TABLE [{0}]", tableName);

Now you're only at risk for SQL injection if tableName matches a real table, and you actually use square brackets in the names of your tables!

Bill Karwin
Validate against know possible inputs. +1
Greg
I think a lot of people forgot that you can't use parameterized queries with this type of query. Good observation.
Joe Philllips
The OP should also consider the possibility of two or more tables having the same name but belonging to different owners/schemas.
CodeByMoonlight
A: 

In this concrete example you need protection from SQL injection only if table name comes from external source.

Why would you ever allow this to happen? If you are allowing some external entity (end user, other system, what?) to name a table to be dropped, why won't you just give them admin rights.

If you are creating and removing tables to provide some functionality for end user, don't let them provide names for database objects directly. Apart from SQL injection, you'll have problems with name clashes etc. Instead generate real table names yourself (e.g DYNTABLE_00001, DYNTABLE_00002, ...) and keep a table that connects them to the names provided by user.


Some notes on generating dynamic SQL for DDL operations:

  • In most RDBMS-s you'll have to use dynamic SQL and insert table names as text. Be extra careful.

  • Use quoted identifiers ([] in MS SQL Server, "" in all ANSI compliant RDBMS). This will make avoiding errors caused by invalid names easier.

  • Do it in stored procedures and check if all referenced objects are valid.

  • Do not do anything irreversible. E.g. don't drop tables automatically. You can flag them to be dropped and e-mail your DBA. She'll drop them after the backup.

  • Avoid it if you can. If you can't, do what you can to minimize rights to other (non-dynamic) tables that normal users will have.

Tomek Szpakowicz