SQL Injections
-
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.
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
-
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
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?
-
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
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.
-
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.
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 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.
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\]); }}
-
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\]); }}
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
-
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?
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.
-
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.
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.
-
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
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.
-
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!
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.
-
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.
-
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.
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.
-
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
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.
-
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.
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
-
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?
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
-
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?
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
-
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
I found myself doing the same thing recently. This looks a LOT like Erland Sommarskog's code from his site (http://www.sommarskog.se). Not a problem, he rocks. I use this very same Common Table Expression to parse delimited strings into one, two, and three column tables for web application that I am developing. I use it to insert double-metaphone terms and the base term into a table for a text-based search mechanism.
-
I found myself doing the same thing recently. This looks a LOT like Erland Sommarskog's code from his site (http://www.sommarskog.se). Not a problem, he rocks. I use this very same Common Table Expression to parse delimited strings into one, two, and three column tables for web application that I am developing. I use it to insert double-metaphone terms and the base term into a table for a text-based search mechanism.