7

I have the following code used to create some database from C# application

SqlConnection myConnection = new SqlConnection(ConnectionString);
string myQuery = "CREATE DATABASE " + tbxDatabase.Text; //read from textbox
myConnection.Open();
SqlCommand myCommand = new SqlCommand(myQuery, myConnection);
myCommand.ExecuteNonQuery();

Now I worry if it is safe, will C# accept hacker input like "A; DROP TABLE B" or something similar? How to make it safer?

j0k
  • 22,600
  • 28
  • 79
  • 90
RRM
  • 3,371
  • 7
  • 25
  • 40
  • use this post http://stackoverflow.com/questions/6547986/how-to-prevent-a-sql-injection-escaping-strings İt suggests using parameter which is the best way – Arif YILMAZ Feb 26 '13 at 15:12
  • well, that's for values -- not table name. – John Woo Feb 26 '13 at 15:12
  • what you are trying to is very dangerous. If I had to do the same thing like you, I wouldnt worry about a hacker comes and inserts "drop table B", but I would worry about letting the hacker to come to that page. It would be easier to secure the page – Arif YILMAZ Feb 26 '13 at 15:20

5 Answers5

5

Table Names and columns names cannot be parameterized but for the first line of defense, wrap the tablename with delimiter such as braces,

string myQuery = "CREATE DATABASE [" + tbxDatabase.Text + "]";

or create a user define function that checks for the value of the input, eg

private bool IsValid(string tableName)
{
    // your pseudocode
    // return somthing
}

then on your code,

if (IsValid(tbxDatabase.Text))
{
    SqlConnection myConnection = new SqlConnection(ConnectionString);
    string myQuery = "CREATE DATABASE [" + tbxDatabase.Text + "]";
    myConnection.Open();
    SqlCommand myCommand = new SqlCommand(myQuery, myConnection);
    myCommand.ExecuteNonQuery();
}
else
{
    // invalid name
}
John Woo
  • 258,903
  • 69
  • 498
  • 492
  • 2
    I agree with your validation recommendation, but how does adding brackets to the table name provide security? For example `"test] drop database xxx --"` – Matthew Feb 26 '13 at 15:14
  • if you are going to concatenate the user input into a query, you should at least verify the text first. In this case, you would probably reject the request if the submitted table name includes anything but alphabetic ascii (edit: "ascii" meaning no extended characters, only a-z A-Z) – David J Feb 26 '13 at 15:18
  • Sql Server provides the QUOTENAME() function to escape sql identifers. While you may want to enforce certain restrictions on the name (e.g. no spaces or special characters), that should be in addition to properly escaping the input (see my answer below). – StrayCatDBA Feb 26 '13 at 16:26
1

yes, your code is very insecure, one alternative is creating stored procedures that only accept the exact parameters to avoid building commands on the fly.

C# itself will not help ypu to avoid those things, it should be embedded on the application logic

Jorge Alvarado
  • 2,664
  • 22
  • 33
1

Use Stored Procedure and pass parameters

SP007
  • 1,871
  • 1
  • 22
  • 31
  • Everybody talks about stored procedures but they belong to some database and I'm just creating a new empty database. So to have stored procedures I'd need to have a base first which I don't have. Am I missing something??? – RRM Feb 27 '13 at 12:00
0

Put the actual creation command inside a stored procedure.

DO NOT just stick the text from a textbox into a string and run it.

EDIT: okay, so it turns out CREATE requires a constant expression, so you can't parameterize the command.

But, if you must do this in code, you should at least validate the input before concatenating it into a string. In this case, you should not allow any table names that contain anything other than a - z, A - Z.

David J
  • 659
  • 1
  • 9
  • 25
0

The secure way to accomplish this is to use the sql function QUOTENAME() to escape the input.

CREATE PROC example_create_db ( @db_name NVARCHAR(256) )
AS 
    BEGIN
        DECLARE @sql NVARCHAR(1000)
    /*
        QUOTENAME() adds the surrounding []'s
    */

        SET @sql = 'CREATE DATABASE ' + QUOTENAME(@db_name) 
        EXEC sp_executesql
    END
StrayCatDBA
  • 2,740
  • 18
  • 25