CODEWITHSUNDEEP
c#sqlsecurity
Vlad
Vlad

Reputation: 217

Security-check for custom SQL SELECT query in C#

I need to verify that a run-time user-supplied SQL query is only used to SELECT data - and can in no way execute other operations (delete, update, insert, ..) or alter the database (alter, create, drop, truncate, ...)

I am not looking for a restricted-user solution (may be implemented later), but for a C# query "white-listing".

Currently, this is the code I am using :

    private bool ValidateDatasourceQuery(String datasourceQuery)
    {
        bool result = false;

        try
        {
            bool isValid = true;

            String query = datasourceQuery.Trim().ToLower();

            if (query.Substring(0, 6) != "select") { isValid = false; }

            if (query.Contains("delete ") || query.Contains(" delete")) { isValid = false; }
            if (query.Contains("exec ") || query.Contains(" exec")) { isValid = false; }
            if (query.Contains("insert ") || query.Contains(" insert")) { isValid = false; }
            if (query.Contains("update ") || query.Contains(" update")) { isValid = false; }

            if (query.Contains("alter ") || query.Contains(" alter")) { isValid = false; }
            if (query.Contains("create ") || query.Contains(" create")) { isValid = false; }
            if (query.Contains("drop ") || query.Contains(" drop")) { isValid = false; }
            if (query.Contains("truncate table ") || query.Contains(" truncate table")) { isValid = false; }

            result = isValid;
        }
        catch (Exception exception) { GUC_Utilities.TraceError(exception); }

        return result;
    }

Any thoughts and ideas? Are there ways to pass through this check and execute a dangerous operation like DELETE? How would you improve this code?

Also another question, is ExecuteReader method only able to run SELECT statements, or could also run other CRUD operations? Like in the following code :

                //execute command
                SqlCommand sqlCommand = new SqlCommand(sql, sqlConnection);
                SqlDataReader sqlDataReader = sqlCommand.ExecuteReader();
                dataTable.Load(sqlDataReader);

Thanks for your time!

PS I am only interested in improving & validating the given code - no GUI, specific roles & other suggestions are currently an option

EDIT (2014-01-16) : After further research and tests, I can confirm that there IS NO RELIABLE WAY to prevent hackers injecting destructive statements inside your SQL query (semicolons, character injections, built-in functions, etc.). The only way to maintain data integrity is TO CREATE A SPECIFIC USER ROLE WITH LIMITED SET OF PRIVILEGES. Everything other must be considered as potentially unsafe. As well, note that EXECUTEREADER can indeed run DELETE, UPDATE and INSERT statements.

Upvotes: 4

Views: 5098

Answers (7)

Sonja
Sonja

Reputation: 595

You can add the comment that is used in most of the cases for SQL Injection:

if (query.Contains("--")) { isValid = false; }

Upvotes: 0

Yakir Manor
Yakir Manor

Reputation: 4829

this might work better, this allows the keyword to appear if its a part of a bigger alphanumeric string:

public static bool ValidateQuery(string query)
{
    return !ValidateRegex("delete", query) && !ValidateRegex("exec", query) && !ValidateRegex("insert", query) && !ValidateRegex("alter", query) &&
           !ValidateRegex("create", query) && !ValidateRegex("drop", query) && !ValidateRegex("truncate", query);
}
public static bool ValidateRegex(string term, string query)
{
    // this regex finds all keywords {0} that are not leading or trailing by alphanumeric 
    return new Regex(string.Format("([^0-9a-z]{0}[^0-9a-z])|(^{0}[^0-9a-z])", term), RegexOptions.IgnoreCase).IsMatch(query);
}

you can see how it works here: regexstorm
see regex cheat sheet: cheatsheet1, cheatsheet2

notice this is not perfect since it might block a query with one of the keywords as a quote, but if you write the queries and its just a precaution then this might do the trick.

you can also take a different approach, try the query, and if it affects the database do a rollback:

public static bool IsDbAffected(string query, string conn, List<SqlParameter> parameters = null)
{
    var response = false;
    using (var sqlConnection = new SqlConnection(conn))
    {
        sqlConnection.Open();
        using (var transaction = sqlConnection.BeginTransaction("Test Transaction"))
        using (var command = new SqlCommand(query, sqlConnection, transaction))
        {
            command.Connection = sqlConnection;
            command.CommandType = CommandType.Text;
            command.CommandText = query;
            if (parameters != null)
                command.Parameters.AddRange(parameters.ToArray());
            // ExecuteNonQuery() does not return data at all: only the number of rows affected by an insert, update, or delete.
            if (command.ExecuteNonQuery() > 0)
            {
                transaction.Rollback("Test Transaction");
                response = true;
            }
            transaction.Dispose();
            command.Dispose();
        }
    }
    return response;
}

you can also combine the two.

Upvotes: 0

chanakya
chanakya

Reputation: 29

jus add-

Select * from ( your Query Here)x

Only Select Queries will be run and other will give an error.

Upvotes: 2

Geoff Wells
Geoff Wells

Reputation: 361

How about wrapping it in a transaction, read the data then always roll back the transaction. So if an malicious code is present it will never get committed.

Upvotes: 2

Mike Beeler
Mike Beeler

Reputation: 4101

Sqldatareader creates a forward only data reader. Selects are the only statements that will work.

As an aside selects with any sort of logic especially if they will be reused should be turned into a stored proc to allow for plan generation and caching.

Upvotes: 2

Travis
Travis

Reputation: 10547

While what you have looks like in some cases it might work, I'd think about taking it a step further and parsing the query for real. Parsing SQL Server Database's Script points to a few items that might be of interest. Then you can learn to ask the syntax tree what is really going on and make decisions based upon that. There's really no security in what you've done. And I can think of a few ways someone who's smart enough could get through your security. If it's an internal app though, you need to consider if the effort is worth it or not.

Upvotes: 1

Owen Pauling
Owen Pauling

Reputation: 11841

How about using the Builder Pattern along with a suitable GUI allowing the user to build the query?

Upvotes: 0

Related Questions