SQL Injections

Nemanja Trifunovic

How to avoid Bobby Tables[^] reminded me of a discussion we had here a while ago. Just want to emphasise one line from the text: The strip gets one thing crucially wrong. The answer is not to "sanitize your database inputs" yourself. It is prone to error.

Programming Blog utf8-cpp

Dario Solera

Nemanja Trifunovic wrote:

sanitize your database inputs

That is so... PHP-ish. It's nice to have DbParameters. :cool:

If you truly believe you need to pick a mobile phone that "says something" about your personality, don't bother. You don't have a personality. A mental illness, maybe - but not a personality. - Charlie Brooker My Photos/CP Flickr Group - ScrewTurn Wiki

Electron Shepherd

Nemanja Trifunovic wrote:

The answer is not to "sanitize your database inputs" yourself. It is prone to error.

That's not a reason for not doing it. Writing a program is prone to error, but professional developers still do it. It's perfectly possible to make a system secure against SQL injection attacks, if you process the inputs poroperly. You don't need stored procedures, and you don't need parameterised queries (and using parameterized queries with a variable number of arguments to an IN clause is really messy). I'm not saying they don't help. In certain situations, stored procedures can help a lot, since most databases allow them to be "pre-optimised". The flip side of that is that you need to make sure the query plan is still correct as the data volume and cardinality changes, of course.

Server and Network Monitoring

Daniel Grunwald

The underlying problem is very simple: People are confusing code with data. SQL is code. A string literal is data. As soon as you put SQL into strings, you have to be very careful to avoid security problems. Exactly the same happens with HTML and user input. Here millions of web developers still haven't realized it's the same problem as SQL injection, and that sanitizing strings isn't a good solution. Use the features of your language to avoid the problem!

Html start = Html.UnsafeFromString("<b>");
string userInput = "<script>alert('XSS');</script>";
Html end = Html.UnsafeFromString("</b>");

Html combined = start + userInput + end; // results in "<b><script>alert('XSS');</script></b>"

"Html" is just a simple class with a + operator, which can even be implemented to be faster than string concatenation - it just builds a tree of concatened Html objects; only when it is finally output to the page all text is copied together using StringBuilder. There's an implicit conversion operator from string to Html, but that is safe (uses HtmlEncode). The only thing I don't like about this approach are all those Html.UnsafeFromString calls. Having HTML literals in the language would solve that (and also guarantee that only literals are passed to UnsafeFromString). Or better, allow user defined literals. C++0x has them!

Rama Krishna Vavilala

Daniel Grunwald wrote:

SQL is code. A string literal is data.

That's a good description.

Christian Graus

You raise several points here.

Electron Shepherd wrote:

That's not a reason for not doing it.

Agreed.

Electron Shepherd wrote:

It's perfectly possible to make a system secure against SQL injection attacks, if you process the inputs poroperly.

Well, it's POSSIBLE, but why would you set out to write your own code in this vital area, to the point of refusing to use tools that have been tested by thousands of other developers ?

Electron Shepherd wrote:

You don't need stored procedures, and you don't need parameterised queries (and using parameterized queries with a variable number of arguments to an IN clause is really messy).

In my mind, the people writing straight SQL, are often the people placing that SQL in the code behind of ASPX pages. Which is to say, I equate it with bad practice.

Christian Graus Driven to the arms of OSX by Vista. Read my blog to find out how I've worked around bugs in Microsoft tools and frameworks.

Electron Shepherd

Christian Graus wrote:

Well, it's POSSIBLE, but why would you set out to write your own code in this vital area

Because: 1) You may not have permission to install stored procedures 2) If you are targetting multiple database systems (SQL Server, Oracle, DB/2 and Sybase, for example), the syntax for stored procedures will be different for different databases, and you may not want the overhead of maintaining multiple versions of the stored procedure code, or you may need to provide support for any ANSI-99 compatible database. 3) Your requirements may preclude parameterised queries. If there is a good way to use an IN clause with a number of values, all of which are only known at run-time, I'd love to know what it is.

Server and Network Monitoring

dojohansen

Electron Shepherd wrote:

Writing a program is prone to error, but professional developers still do it.

Indeed it is. Generally however those programs try to achieve something different from what you already have. If you already have a mechanism built into your tools that does what you need, it's usually a pretty bad idea to set out to duplicate it yourself. It's a waste of time and money, and will likely have a negative impact on performance, security, and reliability.

Electron Shepherd wrote:

You don't need stored procedures, and you don't need parameterised queries

How can you say something like that!? Stored procedures are good for a great many reasons. For starters, if you create procedures the DBA can create a user for your application that doesn't have the rights to touch any tables directly. It facilitates reuse of your database code, improves performance by reducing network traffic and using compiled execution plans in the database, increases encapsulation, makes it easier to support multiple databases with fewer changes in the DALs, and is easier to develop, maintain, and deploy. I'm aware that you can get to re-use compiled execution plans with queries as well, however you only get that benefit if you use *parameterized* queries - at least with SQL Server.

Electron Shepherd wrote:

The flip side of that is that you need to make sure the query plan is still correct as the data volume and cardinality changes, of course.

That's not a "flip side" of using stored procs. That's a consequence of using a relational database - and it applies just the same to ad-hoc queries.

dojohansen

Daniel Grunwald wrote:

which can even be implemented to be faster than string concatenation

Well that doesn't take much! Few things are as slow as string concatenation. :) I like this solution, but would perhaps modify it to use the StringBuilder approach, using a single instance (less alloc/dealloc) and returning a reference to it from methods for appending to it. And drop the "append" prefix since two appending methods is probably all you need. Then the code would become something like this:

// results in "<script>alert('XSS');</script>"
string markup = new Html().Code("").Text("alert('XSS');").Code("").ToString();

Electron Shepherd

dojohansen wrote:

If you already have a mechanism built into your tools that does what you need, it's usually a pretty bad idea to set out to duplicate it yourself. It's a waste of time and money, and will likely have a negative impact on performance, security, and reliability.

See my reply to Christian as to why you sometimes need to do this.

dojohansen wrote:

Stored procedures are good for a great many reasons.

True. But you missed my point. I'm not saying stored procedures have no purpose. I'm saying they aren't necessary to prevent SQL injection attacks, which was the subject of the original post.

dojohansen wrote:

That's a consequence of using a relational database - and it applies just the same to ad-hoc queries.

No it doesn't. Ad-hoc queries are processed by the query optimiser at execution time, and the query plan is determined at point of execution. Having the wrong query plan is a facet of generating a query plan against one set of data, and running it against a different set.

Server and Network Monitoring

Sinisa Hajnal

Electron Shepherd wrote:

Your requirements may preclude parameterised queries. If there is a good way to use an IN clause with a number of values, all of which are only known at run-time, I'd love to know what it is.

This would be xml parameter in stored procedure in SQL Server. Or any database supporting such. Syntax of xml parsing is little bit clumsy, but it works

dojohansen

Electron Shepherd wrote:

See my reply to Christian as to why you sometimes need to do this.

I saw your reasons, and I don't think any of them are good. Since CP is so cleverly hiding the whole thread when we reply I won't use quotes but just copy them directly into the text and comment on them. 1) You may not have permission to install stored procedures You're on thin ice here. It would be highly unusual to say the least, if you're making the db, since you must then be able to create and drop tables as you want. Since that's where the data is, no additional security could possibly come from not letting you create procedures. More importantly, it has nothing to do with the question of sanitation; whether you use ad-hoc queries or procs, what matters is whether you build strings or use parameter objects (letting the provider take care of sanitation). So we can delete this reason. 2) If you are targetting multiple database systems (SQL Server, Oracle, DB/2 and Sybase, for example), the syntax for stored procedures will be different for different databases, and you may not want the overhead of maintaining multiple versions of the stored procedure code, or you may need to provide support for any ANSI-99 compatible database. You're shooting yourself in the foot. The syntax differences for how to invoke procs vary far less than the syntax differences in the entire set of language constructs on each database. If you use procs, you only need to handle the different ways of CALLING them, but if you use ad-hoc queries, your application code must contain ALL the differences in the entire implementation. And, this like (1) is in any case irrelevant to the question of sanitation. 3) Your requirements may preclude parameterised queries. If there is a good way to use an IN clause with a number of values, all of which are only known at run-time, I'd love to know what it is. The in-clause can contain parameters just as well as literals. So again, you are making a case for sometimes using an ad-hoc query, not for implementing your own sanitation. Your code should build the string with parameters instead of user input and create the parameter objects. Of course, it is not always *necessary* to sanitize - if input is not potentially unsafe, go right ahead and use literals. But there is no better or worse reason to write your own sanitation than there is to make your own shoes. In other words, if you can do it better it may be worthwhile, but chances

Graham Bradshaw

And for non-SQL Server databases? We build commercial software (ie not bespoke to a particualr client), so it has to work with pretty much any "proper" database out there. How would solve the problem for say Oracle, DB/2 Sybase and MySQL?

Graham Bradshaw

dojohansen wrote:

It would be highly unusual to say the least, if you're making the db, since you must then be able to create and drop tables as you want.

And it would be very common to be working on an application where the DDL is fixed, and you can't change it at all. I'm thinking of maintenance of existing applications.

dojohansen wrote:

It may well be that you intended to say you don't need them in order to prevent injection attacks,

That is what I meant.

dojohansen wrote:

The in-clause can contain parameters just as well as literals.

Really? Can you give me an example? I couldn't get it to work.

nistrum404

Heh, I remember these from a long time ago. These days my database layers are so abstracted they're writing the stored procedures themselves '; TRU.NCATE TABLE Posts; --M ;)

Matt Dockerty

dojohansen

Graham Bradshaw wrote:

Really? Can you give me an example? I couldn't get it to work.

Well the trivial example goes like this: declare @p0 int declare @p1 int select ... from ... where ... in (@p0, @01) For the SqlProvider, you could use this:

public class InClause<T>
{
string colName;

public InClause(string colName)
{
    this.colName = colName;
}


public List<T> Values = new List<T>();


public override string ToString()
{
    if (Values.Count == 0) throw new InvalidOperationException();

    var sb = new StringBuilder(colName).Append(" IN (");
    for (int i = 0; i < Values.Count; i++)
        sb.Append("@").Append(colName).Append(i).Append(',');

    sb.Length -= 1;
    sb.Append(")");
    return sb.ToString();
}


public void AddParameters(SqlCommand cmd)
{
    for (int i = 0; i < Values.Count; i++)
        cmd.Parameters.AddWithValue("@" + colName + i, Values\[i\]);
}

}

dojohansen

And use it like this:

void test()
{
// Get the columns of type int and char from tables and views in master.
string query = "select table_name, column_name from information_schema.columns where {0}";
var dataTypesClause = new InClause<string>("data_type");
dataTypesClause.Values.AddRange(new string[] {"int", "char"});
query = string.Format(query, dataTypesClause);

// Create the command and add parameter objects.
SqlCommand cmd = new SqlCommand(string.Format(query, dataTypesClause));
dataTypesClause.AddParameters(cmd);

// Execute.
using (SqlConnection cnx = new SqlConnection(@"Data Source=.; Integrated Security=SSPI"))
{
    cmd.Connection = cnx;
    cnx.Open();
    using (SqlDataReader r = cmd.ExecuteReader())
        while (r.Read())
            Debug.WriteLine(string.Format("{0}.{1}", r\["table\_name"\], r\["column\_name"\]));
}

}

modified on Wednesday, September 16, 2009 8:43 AM

Sinisa Hajnal

Oracle has xml. Otherwise Varchar with some functions for parsing of the string to the same effect. I'm not saying it's so wrong that it may not be used, just that one should do maximum to avoid it. And it's not just security or performance issue, there's maintainability too.

Brady Kelly

I do that sometimes, when prototyping and for YAGNI. If it only ever gets used by that one page, and I don't already have a provider model, it stays with that page. That said, even in straight SQL, all my parameters except 'in()' phrases, are parametrised. All the 'in()' phrases are generated from selection lists, impervious to external input values.

I have been trying for weeks to get this little site indexed. If you wonder what it is, or would like some informal accommodation for the 2010 World Cup, please click on this link for Rhino Cottages.

Brady Kelly

dojohansen wrote:

The in-clause can contain parameters just as well as literals.

There goes my fiendishly clever, home-rolled 'in clause parameter scheme' using regex replace and tokens in the query. Know what, I think I'll stick with it. It's coded, and, currently, injection proof.

I have been trying for weeks to get this little site indexed. If you wonder what it is, or would like some informal accommodation for the 2010 World Cup, please click on this link for Rhino Cottages.