Security-check for custom SQL SELECT query in C#

Question

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.

Why not add the security in the DB? Just give the user used to connect to DB Read permissions. — Adarsh Shah
– Adarsh Shah, Commented Jan 5, 2014 at 21:50
What about SELECT * FROM whatever WHERE somevarchar="drop" — Eugen Rieck
– Eugen Rieck, Commented Jan 5, 2014 at 21:51
It looks like it would be saner if the user could just supply the columns to retrieve and where condition using a data structure that allows for tighter control, and make your code create the actual query. — SJuan76
– SJuan76, Commented Jan 5, 2014 at 21:52
You have some sort of grey-list here - at minimum, it should be a strict whitelist (allow SELECT only) - what happens if a new dangerous command is added? Which is going to highlight the first large problem: you won't always find SELECT in the first six characters - you're eliminating CTEs, which can be a huge help in certain types of queries. It might help to know what the anticipated dataset is - if you work for a clothing company "Alter" is likely a common string... Heck, I'm assuming there's an "Executive" job title at your company... and Unicode might still bite you. — Clockwork-Muse
– Clockwork-Muse, Commented Jan 6, 2014 at 4:56
@Vlad T-SQL allows comments within SQL keywords, so SEL/*comment*/ECT would work. — Gumbo
– Gumbo, Commented Jan 6, 2014 at 14:54

Mike Beeler · Accepted Answer · 2014-01-06 00:26:12Z

2

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.

answered Jan 6, 2014 at 0:26

Mike Beeler

4,1112 gold badges32 silver badges44 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Vlad Over a year ago

important notice : it is not true that sqldatareader only accepts SELECT statements - DELETE works just fine.

Geoff Wells · Accepted Answer · 2014-11-28 13:35:11Z

2

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.

answered Nov 28, 2014 at 13:35

Geoff Wells

3613 silver badges10 bronze badges

1 Comment

panta82 Over a year ago

I just wanted to say, what a great idea! Upvote didn't seem enough.

chanakya · Accepted Answer · 2015-08-31 10:36:22Z

2

jus add-

Select * from ( your Query Here)x

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

answered Aug 31, 2015 at 10:36

chanakya

291 bronze badge

2 Comments

elle0087 Over a year ago

yes, but...what happened in case i have an Order By in my query?? Sql Server doesn t allow it...

chanakya Over a year ago

@user3061212 uh can check this out .... Select * from ( SELECT *,rownumber = ROW_NUMBER() over(order by employeeCode desc) FROM employee )x .. Although its not perfect but works perfectly and about rownumber column uh have to hide it.

Community · Accepted Answer · 2017-05-23 12:08:57Z

1

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.

edited May 23, 2017 at 12:08

CommunityBot

11 silver badge

answered Jan 6, 2014 at 0:15

Travis

10.6k2 gold badges30 silver badges49 bronze badges

1 Comment

Vlad Over a year ago

indeed, this post proposes few existing vendor tools for parsing SQL queries, so they can easily be validated, while preventing injection attacks etc. thanks for the information! by the way I would really appreciate if you could share a way to get through (if you have some time)

Owen Pauling · Accepted Answer · 2014-01-05 23:57:10Z

0

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

answered Jan 5, 2014 at 23:57

Owen Pauling

12.1k20 gold badges58 silver badges67 bronze badges

1 Comment

Vlad Over a year ago

no extra tools and funky GUIs, text-only. i know that the method is not 100% safe, but right now I am just trying to validate (& improve) the given code to the best extent, as written in the question :)

Yakir Manor · Accepted Answer · 2017-01-17 12:57:21Z

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.

Sonja · Accepted Answer · 2021-11-24 07:24:31Z

0

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

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

answered Nov 24, 2021 at 7:24

Sonja

5941 gold badge8 silver badges22 bronze badges

Collectives™ on Stack Overflow

Security-check for custom SQL SELECT query in C#

7 Answers 7

1 Comment

1 Comment

2 Comments

1 Comment

1 Comment

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

1 Comment

1 Comment

2 Comments

1 Comment

1 Comment

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related