91

I use an API that expects a SQL string. I take a user input, escape it and pass it along to the API. The user input is quite simple. It asks for column values. Like so:

string name = userInput.Value;

Then I construct a SQL query:

string sql = string.Format("SELECT * FROM SOME_TABLE WHERE Name = '{0}'",
                           name.replace("'", "''"));

Is this safe enough? If it isn't, is there a simple library function that make column values safe:

string sql = string.Format("SELECT * FROM SOME_TABLE WHERE Name = '{0}'",
                           SqlSafeColumnValue(name));

The API uses SQLServer as the database.

saluce
  • 13,035
  • 3
  • 50
  • 67
sc45
  • 1,916
  • 2
  • 16
  • 25
  • 3
    You **need** to use parameters. – SLaks Mar 08 '10 at 18:30
  • 2
    @SLaks, Obvious the API doesn't allow it. Maybe he needs a new API. – C. Ross Mar 08 '10 at 18:48
  • 3
    @sri you'll get better answers if you explain why you can't/won't use parameters. – Foole Mar 09 '10 at 05:46
  • Parameterise it! A [Google search](http://www.google.com/search?q=site%3Astackoverflow.com+sql+injection&spell=1) of SO for "SQL injection" Edit: In response to Seva Alekseyev, [SO answer with character 8 injection](http://stackoverflow.com/questions/1800013/does-this-code-prevent-sql-injection) – gbn Mar 08 '10 at 18:27
  • is there a way to parametrize queries with lists? `SELECT [Name], [Value] FROM [SomeTable] WHERE [Name] IN (@ListOfNames)` I mean some other way than `var listParNames = listNames.Select(name => { var pname = string.Format("@n{0}", cmd.Parameters.Count); cmd.Parameters.AddWithValue(pname, name); return pname; });` `cmd.CommandText = string.Format("SELECT [Name], [Value] FROM [SomeTable] WHERE [Name] in ({0})", string.Join(",", listParNames.ToArray()));` more like direct `cmd.Parameters.AddWithValue("@ListOfNames", listNames.ToArray());` – mizuki nakeshu Apr 15 '14 at 12:20
  • I'm sorry to downvote the question, but, this is a REALLY BAD api design in itself... What happens when a requirement comes to change the database from, say MSSQL to MySQL or so? Your API will break spectacularly. If your API is so tightly coupled with your DB implementation, why don't you just allow a direct access to your DB and strenghten up the security around that? – Mladen B. Apr 06 '20 at 16:43
  • @MladenB. Legacy design exists, and replacing entire systems with "best practice" code isn't always viable. Whilst I agree that this should be param based and decoupled, there are tons of reasons that this may not be the case. Try getting a business to invest in refactoring an API which has been working for 10 years, has a cost attributed to "fixing" it and carries high risk if changed! – JustAnotherDeveloper Dec 22 '20 at 09:13

6 Answers6

161

Since using SqlParameter is not an option, just replace ' with '' (that's two single quotes, not one double quote) in the string literals. That's it.

To would-be downvoters: re-read the first line of the question. "Use parameters" was my gut reaction also.

EDIT: yes, I know about SQL injection attacks. If you think this quoting is vulnerable to those, please provide a working counterexample. I think it's not.

Seva Alekseyev
  • 59,826
  • 25
  • 160
  • 281
  • 2
    -1: And how does that protect against things like Little Bobby? (http://xkcd.com/327/ if you don't know what that means). While parameters may not be an option, the solution of replacing single with double quotes is certainly not 'it' as your answer states. – Greg Beech Mar 08 '10 at 18:37
  • 14
    There's a little ' after Robert. That's how. When properly quoted, Bobby's unorthodox name would stored in the database in its entirety. – Seva Alekseyev Mar 08 '10 at 18:37
  • I was speaking figuratively. You wouldn't have quotes with, for example, numeric arguments. It's this *kind* of thing that is the problem, not that exact example. (For example, that exact syntax wouldn't work with SQL Server, but the problem still applies.) – Greg Beech Mar 08 '10 at 18:39
  • you can invalidate this replace by using character 8, backspace. Or "high" unicode characters matching ' – gbn Mar 08 '10 at 18:42
  • 3
    @Greg: I'm with you. This method is a nonstarter and should not be encouraged. – Steven Sudit Mar 08 '10 at 18:42
  • 3
    Not double quotes - two single quotes. That's how single quote is escaped in SQL Server. Sorry it came out that way, I've already edited. Yes, I know about SQL injection. And I said "string literals" - numeric arguments are not string literals. – Seva Alekseyev Mar 08 '10 at 18:43
  • 9
    Sure... character 8 injection: http://stackoverflow.com/questions/1800013/does-this-code-prevent-sql-injection – gbn Mar 08 '10 at 18:50
  • 5
    @gbn: If you're basing that suggestion completely on the evidence of the accepted answer in that post, I suggest reading the comment I added and trying it for yourself. – Jon Seigel Mar 08 '10 at 19:56
  • 2
    @gbn: debunked... Keep trying :) String quoting is like pointer arithmetic in C or dancing on a minefield: inherently dangerous, but works if done right. – Seva Alekseyev Mar 08 '10 at 20:27
  • 58
    Seva Alekseyev thank you so, so, so much for reading my question and trying to answer it. Thank you for not preaching to me about "best practices". And thank you for not trying to "educate me" Really, thank you! – sc45 Mar 08 '10 at 22:30
  • 14
    Um, for the record, I still think that parameters are the way to go :) However, I've been in this business long enough to understand the existence of constraints on your designs. Been there, done that. – Seva Alekseyev Mar 08 '10 at 22:33
  • 14
    @Seva While everyone knows the code above is subject to injections, sometimes you are tied to an API that just doesn't allow you to follow best practices. Too often programmers worry about stuff that isn't within the circle of concern of a particular task. Good answer to the specific question. – Keith Adler Mar 08 '10 at 22:36
  • 5
    @Seva I would do the same thing, but in this case I can't. A couple of other times I've asked questions on SO and people want to "educate me" or question why things are setup that way, which is pretty frustrating :-) (And yes I understand about SQL injection and the other best practices...) – sc45 Mar 08 '10 at 22:49
  • What about \0 in the text? – nh43de Mar 22 '23 at 16:27
1

SqlCommand and Entity Framework use exec sp_executesql....

So there really is an alternative to raw strings with your own escaping pattern presumably. With SqlCommand you are technically using parameterised queries but you're bypassing the ADO.Net abstraction of the underlying SQL code.

So while your code doesn't prevent SQL Injection, the ultimate answer is sp_executesql not SqlCommand.

Having said that, I'm sure there are special handling requirements for generating an SQL Injection-proof string which utilizes sp_executesql.

see: How to return values from a dynamic SQL Stored Procedure to the Entity Framework?

Community
  • 1
  • 1
Kind Contributor
  • 17,547
  • 6
  • 53
  • 70
0

I was using dynamic sql (I can hear the firing squad loading their rifles) for search functionality, but it would break whenever a user searched for somebody with a surname like "O'Reilly".

I managed to figure out a work-around (read "hack"):

Created a scalar-valued function in sql that replaced a single quote with two single quotes, effectively escaping the offending single quote, so
"...Surname LIKE '%O'Reilly%' AND..." becomes "...Surname LIKE '%O''Reilly%' AND..."

This function gets invoked from within sql whenever I suspect fields could contain a single quote character ie: firstname, lastname.

CREATE FUNCTION [dbo].[fnEscapeSingleQuote]
    (@StringToCheck NVARCHAR(MAX))
RETURNS NVARCHAR(MAX)
AS
BEGIN
    DECLARE @Result NVARCHAR(MAX)
    SELECT @Result = REPLACE(@StringToCheck, CHAR(39), CHAR(39) + CHAR(39))
    RETURN @Result
END

Not very elegant or efficient, but it works when you're in a pinch.

0

One may wish to replace ' with '' instead of parameterizing when needing to address the ' problem in a large amount of ad hoc sql in a short time with minimal risk of breakage and minimal testing.

-5

Simple:

const string sql = "SELECT * FROM SOME_TABLE WHERE Name = @name";

and add the @name parameter with value:

cmd.CommandText = sql;
cmd.Parameters.AddWithValue("@name", name);
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • 36
    He still needs to get the actual SQL statement out of the command object to pass to the API. – reustmd Mar 08 '10 at 18:36
  • He explicitly states that he is using an API that accepts a SQL string. He is NOT using ADO.Net. – MHollis Jun 13 '19 at 14:31
  • @MHollis exactly; which is the first problem they need to fix. Sorry, but there is **no advisable way** of doing this without parameters. There is a *reason* folks are shouting about parameters. – Marc Gravell Jun 13 '19 at 14:55
-11

If you need to escape a string for a MSSQL query try this:

System.Security.SecurityElement.Escape(Value)
Kevin
  • 9
  • 12
    Sorry Kevin but that Escape function only escapes XML not SQL (http://msdn.microsoft.com/en-us/library/system.security.securityelement.escape.aspx) – Pete Duncanson May 22 '12 at 08:46