6

We have a SQL statement that uses the SqlBuilder set the table name in the from clause. The database is SQL Server 2008 and up.

var sqlBuilder = new SqlBuilder();

sqlBuilder.Select("*").From(tableName);
sqlBuilder.Where("...");

Connection.BuilderQuery<dynamic>(sqlBulder).Select(Map);

I am wondering if this is a SQL injection risk? and how can I mitigate that risk? Or does the SqlBuilder take care of these things?

Could I mitigate the risk simply by wrapping the table name in square brackets? e.g.

sqlBuilder.From("[" + tableName + "]");

Also it would be most appreciated if someone could provide some examples of a SQL injection attack in the FROM clause so that I can understand how it works and create tests.

John Saunders
  • 160,644
  • 26
  • 247
  • 397
There is no spoon
  • 1,775
  • 2
  • 22
  • 53
  • 8
    What is `SqlBuilder`? A class you created or from a 3rd party library? – Trevor Pilley May 19 '15 at 21:19
  • You need to parameter the sql query -- simply surrounding the tables in brackets does not protect you from sql injections. – Michael G May 19 '15 at 21:22
  • If there is any user input in a dynamic query there is a danger of SQL injection. Often this won't be obvious and vulnerabilities may exist even if you think they don't. If `tableName` is, say, a constant, and that is the only dynamic element of the query then it should be safe. Generally dynamic SQL should be avoided. – garryp May 19 '15 at 21:22
  • 2
    If you need to dynamically create the query to such a degree that your table name and column names should be variables, then you are probably doing something wrong. and by probably I mean at least 80% of the time. – Zohar Peled May 19 '15 at 21:28
  • Unfortunately, when it comes to things like column names and table names, parameterization is ***not*** sufficient, to prevent SQL Injection. And blind input "cleansing" (like throwing brackets ("[..]") around the input text) is almost never sufficient on its own. To do this correctly you need to both A) parameterize the inputs *and* B) validate that those inputs are really objects the user/client app should be accessing. See here (http://stackoverflow.com/a/1246848/109122) for a farily complete answer to this question along with example SQL code. – RBarryYoung May 19 '15 at 21:36
  • 1
    Closing the question now because it is unclear what SqlBuilder is. No valid answers are possible at this point. I downvoted all of them right now because they do not answer the question. – usr May 19 '15 at 21:41
  • @usr: would you say this is a duplicate of http://stackoverflow.com/questions/1246760/how-should-i-pass-a-table-name-into-a-stored-proc? If so, I can hammer it. – John Saunders May 19 '15 at 21:46
  • @JohnSaunders I think nobody knows what this is about until we know what SqlBuilder does with its arguments. If you ask me I think it is inappropriate to close as a dupe. I voted "unclear". – usr May 19 '15 at 21:48
  • 1
    @usr: I _did_ ask you :-) Voted to close as unclear. – John Saunders May 19 '15 at 21:53
  • In addition to what `SqlBuilder` is, we also need to know how the `tableName` variable is populated before we can know whether or not this is a problem. It may be that `SqlBuilder` is generally vulnerable here, but the `tableName` variable is only every populated by selecting from a trusted source. Also voting to place this on hold until the OP can edit the question to contain the missing details. – Joel Coehoorn May 19 '15 at 21:54
  • FWIW, I think it's likely he's using this (but we can't be sure): https://github.com/maxtoroq/DbExtensions/blob/master/docs/SqlBuilder.md – Joel Coehoorn May 19 '15 at 22:04
  • Where is little bobby tables when you need him? – Zohar Peled May 19 '15 at 22:08
  • Regardless of what `sqlBuilder` is... I understand what it is - this is custom mechanism of building the query – T.S. May 19 '15 at 22:18
  • Thanks everyone for your advice. I now realize that the need to know what SqlBuilder is and how tableName is populate is critical to answering the question. – There is no spoon May 19 '15 at 22:58

5 Answers5

3

I don't know what SqlBuilder is, but here's an example of exploiting an injection: Suppose you have a code that does:

var myFullQuery = string.Format("SELECT * FROM {0} WHERE A = 1", externalInput);

and then executes this against the database. If a malicious user sent this string as an input: ValidTableName; DELETE FROM ValidTableName; SELECT * FROM ValidTableName

the myFullQuery variable would be set to: SELECT * FROM ValidTableName; DELETE FROM ValidTableName; SELECT * FROM ValidTableName WHERE A = 1 and you've lost you entire table... Obviously much more devestating commands can be implemented this way...

Amit
  • 45,440
  • 9
  • 78
  • 110
3

It's unclear from the information in the question whether or not the code is vulnerable. It's unlikely that the SqlBuilder object is handling the table name in a safe way, but we can't know for sure until we know the exact details of SqlBuilder, which is not a standard part of the .Net framework. We also need to know more about the tableName variable, because even an unsafe SqlBuilder implementation might be okay with a safe source for the table name.

That out of the way, I can with certainty say that the proposed fix using square brackets does not solve the problem. Given this code:

sqlBuilder.From("[" + tableName + "]");

I could still supply input like this:

information_schema.columns];DROP TABLE x;SELECT * FROM [y

and the section in the middle would still be injected and executed.

Also, it looks like you may be using this:

https://github.com/maxtoroq/DbExtensions/blob/master/docs/SqlBuilder.md

If so, after looking through the docs for that type, you should be aware that this is definitely not parameter safe for the table name, and was likely written with MySql in mind (it uses the LIMIT clause, which is a non-standard MySql extension to SQL).

Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794
1

Or does the SqlBuilder take care of these things?

Nope. You have to use the technology correctly to protect yourself. Just like having a car with seatbelt, you're only protected if you use it properly.

I am wondering if this is a SQL injection risk?

Maybe. If you use any type on end-user data, then most likely yes. If you have to ask your end user which table they want to populate, i'd recommend an enum. Dropdown box with string title and int value;

public enum WhitelistTable
{
  Undefined = 0,
  MyTable1,
  MyTable2
}

If the user data isn't an int, it's not valid. You can either put a DescriptionAttribute on each enum value or simply .ToString() the enum value and insert that. Impementing Whitelists are better than Blacklists.

var myTable = WhitelistTable.MyTable1;

sqlBuilder.From(myTable.ToString());

Could I mitigate the risk simply by wrapping the table name in square brackets?

I don't think so, not generally (I'm not 100% sure with SqlBuilder). I wouldn't even chance it by relying on a third party source. Example Value:

SomeTable]; Drop Table SomeTable; --

Yields:

sqlBuilder.From("[SomeTable]; Drop Table SomeTable; --]");
Erik Philips
  • 53,428
  • 11
  • 128
  • 150
0

I have previously had to do something similar, and whilst I dont know the SqlBuilder, I solved it by checking the result of ExecuteScalar() for the following query

select count(*) from information_Schema.tables where table_name = @tbl

If the result was > 0, I would then use do the following (if the result is 0 then you must break away)

string sql = "Select a, b, c from ["+tblName+"]"

It's dirty, but using the information schema should guarantee the table name exists in the database and the variable doesn't contain anything nasty.

Neil P
  • 2,920
  • 5
  • 33
  • 64
0
sqlBuilder.Select("*").From(tableName);

If tableName data comes from user input, something user can type on keyboard or upload in the file = you're vulnerable to Sql Injection 

sqlBuilder.Where("...");

Same thing. Where usually comes from user input. User types something and you use that value in the where clause. Does SqlBuilder do parametarization? - if the answer is NO you're vulnerable.

Quote: Or does the SqlBuilder take care of these things?

Answer: You tell us. Most people here are about to close your question because it is unclear what is SqlBuilder and how it works. Although, I understand at least what it is because we used similar concept. Our Table names were constants and our sql generator parametarized values that were added to where clause.

T.S.
  • 18,195
  • 11
  • 58
  • 78