SQL Injections

[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:

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.

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.

[Brady Kelly]

I don't get this code. What does UnsafeFromString do? It seems more intuitive to me that this method be applied to the user input, which is unsafe and from a string.

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.

[Tomz_KV]

Using stored procedures (no dynamic query) not only prevent sql injection but also give the flexibility that some data modification can be performed in database alone without modifying the main program.

TOMZ_KV

[Daniel Grunwald]

Unsafe from string simply takes a string and treats it has HTML. Unlike the implicit conversion operator which is safe because it encodes the user input, UnsafeFromString simply takes the HTML string as it is. It is in line with the naming pattern used in the .NET framework - methods which allow unsafe operations are prefixed with "Unsafe". See ThreadPool.UnsafeQueueUserWorkItem, Marshal.UnsafeAddrOfPinnedArrayElement, etc. The idea is to make all places where data is "converted" to code very explicit by requiring a UnsafeFromString call. Of course, this only really works well if there are only a handful of UnsafeFromString calls in the whole project - hence the need for custom literals in the language.

[dojohansen]

But using stored procedures does NOT in itself prevent injection.

string sql = "exec findProducts " + userQuery;

If userQuery is user provided and we go ahead and use the sql variable as command text we are every bit as vulnerable to attack as if we'd used an ad-hoc query. So SQL injection is an issue regardless. Besides, there are many things that are not best done with stored procedures. A simple example: Say you've got a view in the db, and users should be able to see 15 columns from this view, and sort by whatever columns they like. Even if they only sort by one column at a time, you have 32K ways of ordering the table. Obviously it wouldn't be a good solution to have a stored procedure for every sort order. In MSSQL for example we could use sp_executeSql to "parameterize" this, but it would mean we never got to reuse execution plans, even if 95% of the time the data was requested in default order. Also, it's often easier to write the logic that generates queries in the app layer rather than in SQL itself. I completely agree otherwise - stored procs are often easier to deploy, increase encapsulation and modularity, facilitate reuse of database logic, can be developed/tested/maintained independently from application logic, can be easily shared across applications (regardless of technology), reduce network traffic, and so on. In short, stored procs are very good and deserve to be used a lot. It is however important to be very clear about the fact that SQL injection is not a question of procs or ad-hoc queries, but rather about building command texts with unsafe user input. It doesn't actually matter if the command text is to call a proc or run a query or a DDL statement or anything else.

[Tomz_KV]

I did not express my thought clearly enough when I indicated "no dynamic query" in stored procedures, which is another term for non-parametized ad-hoc. As long as a parametized query (user query or stored proc) is used, sql injection will not work. I agree with your analysis on the pros and cons of using stored procedures.

TOMZ_KV

[app1dak]

on any ANSI SQL-92 compliant DB the following should work... Create an SQL Function to parse variable length delimited list of values and create a temporary table in memory: CREATE FUNCTION [dbo].[CteSplitString] (          -- Add the parameters for the function here      @list   nvarchar(MAX),       @delim nchar(1) = ',' ) RETURNS TABLE AS RETURN (       WITH ctePos(lStartPos, lStopPos) AS       (             SELECT lStartPos = CONVERT(bigint, 1),                      lStopPos   = CHARINDEX(@delim COLLATE Slovenian_BIN2, @list + @delim)             UNION ALL             SELECT lStartPos = lStopPos + 1,                      lStopPos   = CHARINDEX(@delim COLLATE Slovenian_BIN2, @list + @delim, lStopPos + 1)             FROM   ctePos             WHERE   lStopPos > 0       )       SELECT LTRIM(RTRIM(SUBSTRING(@list, lStartPos, CASE WHEN lStopPos > 0                                                                                     THEN lStopPos - lStartPos                                                                                     ELSE 0

[app1dak]

on any ANSI SQL-92 compliant DB the following should work... Create an SQL Function to parse variable length delimited list of values and create a temporary table in memory: <pre>CREATE FUNCTION [dbo].[CteSplitString] (                @list   nvarchar(MAX),       @delim nchar(1) = ',' ) RETURNS TABLE AS RETURN (       WITH ctePos(lStartPos, lStopPos) AS       (             SELECT lStartPos = CONVERT(bigint, 1),                      lStopPos   = CHARINDEX(@delim COLLATE Slovenian_BIN2, @list + @delim)             UNION ALL             SELECT lStartPos = lStopPos + 1,                      lStopPos   = CHARINDEX(@delim COLLATE Slovenian_BIN2, @list + @delim, lStopPos + 1)             FROM   ctePos             WHERE   lStopPos > 0       )       SELECT LTRIM(RTRIM(SUBSTRING(@list, lStartPos, CASE WHEN lStopPos > 0                                                                                     THEN lStopPos - lStartPos                                                                                     ELSE 0                                                                                     END