5
\$\begingroup\$

I am working on a program which use OLEDB to connect to a MS Access 2007 file. My program has a possibility to add and delete records from file by using SQL statements which select deleted item by ID.

Now, I was warned that my code might cause SQL injection which would delete all entries from the file. How can I prevent this? Where lies the problem? One person mentioned that I could avoid it by having good field verification, but what should I verify for?

This is how I delete items:

// SQL query which will delete entry by using entry ID.
string SQL = "DELETE FROM PersonalData WHERE DataID = " + txtEntryID.Text;

private void DeleteData(string SQL)
{
    // Creating an object allowing me connecting to the database.
    // Using parameters in command will avoid attempts of SQL injection.
    OleDbConnection objOleDbConnection = new OleDbConnection();
    // Creating command object.
    objOleDbConnection.ConnectionString =
        "Provider=Microsoft.ACE.OLEDB.12.0;" +
        "Data Source=" + filePath + ";" +
        "Persist Security Info=False;" +
        "Jet OLEDB:Database Password=" + pass + ";";
    OleDbCommand objOleDbCommand = new OleDbCommand();

    objOleDbCommand.CommandText = SQL;

    // Assigning a connection string to the command.
    objOleDbCommand.Connection = objOleDbConnection;

    try
    {
        // Open database connection.
        objOleDbConnection.Open();
        objOleDbCommand.ExecuteNonQuery();
    }
    catch (Exception ex)
    {
        // Displaying any errors that 
        // might have occured.
        MessageBox.Show("Error: " + ex.Message);
    }
    finally
    {
        // Close the database connection.
        objOleDbConnection.Close();
    }

    // Refreshing state of main window.
    mainWindow.DisplayFileContent(filePath);

    lblMessage.Text = "Data was successfully deleted.";

    // Clearing text box field.
    txtEntryID.Clear();
}
\$\endgroup\$
  • \$\begingroup\$ Its getting better, but I'd still try to use OleDbParameter in your query generation as a good habit. (Because when you do what you do this with string instead of a number, you're back to square one...) \$\endgroup\$ – Arjan Einbu Dec 1 '11 at 19:40
  • 1
    \$\begingroup\$ Also, int.TryParse is designed the way it is, so you can use it directly in the if. Like this: if(int.TryParse(str, out i)) \$\endgroup\$ – Arjan Einbu Dec 1 '11 at 19:42
8
\$\begingroup\$

Unless I misunderstand, you are using unfiltered user input from txtEntryID.
Never trust user input.

What if the user fills txtEntryID with 123 OR 1=1?
The query will delete everything:

DELETE FROM PersonalData WHERE DataID = 123 OR 1=1

What if, then, the user inputs 123; UPDATE Account SET credit = 1000 WHERE userId = me:

DELETE FROM PersonalData WHERE DataID = 123; UPDATE Account SET credit = 1000 WHERE userId = me
\$\endgroup\$
  • \$\begingroup\$ I do have field check for empty string and a number input only. Should that be enough? \$\endgroup\$ – HelpNeeder Dec 1 '11 at 17:38
  • \$\begingroup\$ ... for field validation? \$\endgroup\$ – HelpNeeder Dec 1 '11 at 18:13
  • 1
    \$\begingroup\$ @HelpNeeder In this exact case, the checks for empty and number only would probably be good enough. Using them won't teach you better habbits, as command parameters will. \$\endgroup\$ – Arjan Einbu Dec 1 '11 at 20:11
  • \$\begingroup\$ @Arjan Einbu: Ok, I will use them also. Thanks, You are very helpful. \$\endgroup\$ – HelpNeeder Dec 1 '11 at 23:27
  • 1
    \$\begingroup\$ I completely agree with @ArjanEinbu . \$\endgroup\$ – ANeves Dec 2 '11 at 11:22
6
\$\begingroup\$

Look at ANeves description of SQL injection.

I'll just show a solution to the problem:

First change the query to include a parameter placeholder, like this:

string query = "DELETE FROM PersonalData WHERE DataID = ?";

Later when you're building your query, you add the parameter to the OleDbCommand object:

var cmd = new OleDbCommand(query);
cmd.Parameters.Add(new OleDbParameter{Value = txtEntryID.Text};

... and then you're ready to execute the query as before.

The parameters mechanism of ADO.NET takes care of all known escapes that are needed to prevent a SQL-injection attack if you create you're queries right.

\$\endgroup\$
4
\$\begingroup\$

Your code can easily be exploited, as ANeves showed.

In your case a small fix can make it safe. By simply parsing the id as an integer, it's totally safe. An integer can not contain any harmful code:

int id = Int32.Parse(txtEntryID.Text);
string SQL = "DELETE FROM PersonalData WHERE DataID = " + id.ToString();

Concatenating the value into the SQL code is however generally considered bad practice. The general solution to handling user input in queries in a safe way is to use parameterised queries, like Arjan Einbu described.

You should still parse the input, though.


I made some change suggestions to your database method, making it reusable:

// Take a parameter that is a delegate for code that sets parameters
private static void ExecuteSql(string sql, Action<OleDbCommand> setParameters) {

  // Use 'using' to make sure that the objects are disposed correctly
  // No 'obj' prefix on variables, hungarian notation is not needed for type
  // No 'OleDb' in the variable name, that information is not needed in the name
  using (OleDbConnection connection = new OleDbConnection()) {

    objOleDbConnection.ConnectionString =
      "Provider=Microsoft.ACE.OLEDB.12.0;" +
      "Data Source=" + filePath + ";" +
      "Persist Security Info=False;" +
      "Jet OLEDB:Database Password=" + pass + ";";

    // Set query and connection in the command constructor
    using (OleDbCommand command = new OleDbCommand(sql, connection)) {

      // Call code that sets the parameters
      if (setParameters != null) {
        setParameters(objOleDbCommand);
      }

      try {
        connection.Open();
        command.ExecuteNonQuery();
      } catch (Exception ex) {
        MessageBox.Show("Error: " + ex.Message);
      }
      // No 'finally' block, as the 'using' block closes the connection

    }
  }
}

// Overload for no parameters
private static void ExecuteSql(string sql) {
  ExecuteSql(sql, null);
}

Usage:

// Parse the input
int id = Int32.Parse(txtEntryID.Text);
// Call the method with a lambda expression for setting parameters
// Set the parameter with type and value
ExecuteSql(
  "DELETE FROM PersonalData WHERE DataID = ?",
  command => {
    command.Parameters.Add("?", OleDbType.Integer).Value = id;
  }
);
// Handle UI stuff
mainWindow.DisplayFileContent(filePath);
lblMessage.Text = "Data was successfully deleted.";
txtEntryID.Clear();
\$\endgroup\$
  • \$\begingroup\$ I see you are using some C++ there? command => .. could you translate to c#? Or that's how it should be? \$\endgroup\$ – HelpNeeder Dec 1 '11 at 18:21
  • \$\begingroup\$ It isn't C++. Its C#, and => is a lambda expression. The second parameter to ExecuteSql is a method (more precice: a method that takes in an OleDbCommand as a param), so instead of creating a standalone method to send in, @Guffa sends in an anonymous method/lambda expression. \$\endgroup\$ – Arjan Einbu Dec 1 '11 at 19:33
  • \$\begingroup\$ @Guffa: I would see if I could do this without the lambda. Passing the parameter directly instead of the need to work with the command object. Perhaps use params object[] to keep your ExecuteSql method as general as it is... \$\endgroup\$ – Arjan Einbu Dec 1 '11 at 19:46
  • 1
    \$\begingroup\$ @ArjanEinbu: It's easier to create parameters when you have the command object. You could pass in params OleDbParameter[], but then you have to create the parameter objects and pass them in. I would not recommend passing in params object[] as you can't specify the data types for the parameters. \$\endgroup\$ – Guffa Dec 1 '11 at 20:50
2
\$\begingroup\$

Looking more at your code, I would also recommend using the using keyword instead of try/finally to close the connection.

using(var cn = new OleDbConnection(connectionString))
{
    // open your connection here if needed
    // run your querie(s) here
}  //cn.Dispose() is call here automatically, and that will also close the connection

This will improve the readability and understandability of your code by placing relevant code together, and also cut down on lines of code.

BTW since you're also using exception handling here, I would just put a try/catch around the using block.

\$\endgroup\$
2
\$\begingroup\$

I think readability of your code would increase if you used the constructor parameters more often.

Like:

var cn = new OleDbConnection(connectionString);

// instead of:
var cn = new OleDbConnection();
cn.ConnectionString = connectionString;

and

var cmd = new OleDbCommand(query);

// instead of:
var cmd = new OleDbCommand();
cn.CommandText = query;
\$\endgroup\$
2
\$\begingroup\$

Another solution would be to properly filter your input. It is easier, and it is also more dangerous, because it is easy to miss something.

For the example at hand, you need to parse the user input into a number, and then convert that number back to string in order to append it to your SQL string, as follows:

int dataid = int.Parse( txtEntryID.Text ); //will throw exception if not valid number
string SQL = "DELETE FROM PersonalData WHERE DataID = " + dataid; //implies dataid.ToString();

In the case of string parameters it gets more complicated, because you need to ensure that the string does not contain any characters below \u0020, that any single quotes within the string are properly escaped by performing s = s.Replace("'", "''"), and that any other characters that may have a special meaning under the RDBMS you are using are stripped away.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.