apostrophes in text being inserted into sql

[Michel Posseth [MCP]]

Hello cj2


I strongly recomend you to use a Parameterized command object or
alternatively even bether a SQL server SP ( preferable a T-SQL version )
why is a SP in your situation even bether ? A stored procedure is an
database object wich can be granted rights on its own so in the case of a
web project this gives you an extra security layer as a potential hacker
has only the ability to execute your SP code and does not have rights on the
actual tables itself .
Also a SP might be compiled in a execution plan wich can potentially boost
SQL server performance .

I see still daily concanated SQL strings however ditch them ( rewrite ) them
as soon as you can as they are a potential security risk especially when
used in web projects , look up the term SQL injection on Google and show
that to your boss if he still wants you to use concanated SQL strings .

By the way a much simpler solution of creating "correct code" is to just
include a new dataset to your project , double click on the dataset to open
its designer right click , add a table adapter and just add your
parameterized SQL to a fill method .

In the background VS.Net creates everything for you without having to write
anny code ( just a few mouse clicks ) in your code you can now reach your
data through a strong typed dataset by a the table adapter that was
generated in the above steps .

If you would like to know more about this aproach let me know and i write
you a step by step guide

HTH

Michel

 

[Tom Shelton]

Steward,

What is better or worse is mostly very subjective.

However, in SQL server a stored procedure will be cached. Therefore as you
have transact code which is very intensively used, then the sproc can be a
better choose but that does not imply that it is forever a better choose, as
it is because of maintenance and deployment, then dynamic SQL what in a way
is Linq as well, can be a better choice.

Cor

Parameterized querys are also cached.

 

[Cor Ligthert[MVP]]

Tom,

Thanks, I often just follow the crowd, I never have deep investigated that,
as you write this, it seems to me more then obvious.

Cor

 

[Chris Diver]

Anytime you concatenate strings provided by the user you have a risk of an SQL injection attack. My
suggestion does not introduce the exposure. The original code has the same exposure.
By the way: http://msdn.microsoft.com/en-us/library/ms998271.aspx

Says at the bottom of the page:

Additional Considerations
Other things to consider when you develop countermeasures to prevent SQL injection include:

Use escape routines to handle special input characters.
Use a least-privileged database account.
Avoid disclosing error information.

Use Escape Routines to Handle Special Input Characters
In situations where parameterized SQL cannot be used and you are forced to use dynamic SQL instead,
you need to safeguard against input characters that have special meaning to SQL Server (such as the
single quote character). If not handled, special characters such as the single quote character in
the input can be utilized to cause SQL injection.

Note Special input characters pose a threat only with dynamic SQL and not when using parameterized
SQL.
Escape routines add an escape character to characters that have special meaning to SQL Server,
thereby making them harmless. This is illustrated in the following code fragment:

private string SafeSqlLiteral(string inputSQL)
{
return inputSQL.Replace("'", "''");}
}


Perhaps you can suggest that Microsoft change their recommendation.

1) I wasn't replying to your post, the question was:

"While I agree that using parameters is the way to handle this, what is
wrong with substituting two quotes for one?"

2) If your using SQL server then escaping single quotes is all well and
good AFAIK. I provided an example where this wouldn't be sufficient
using MySQL. It also could only happen in a situation where backslash
escaping is disabled.

Since when did Microsoft start writing documentation for other DBMS.

Your advice is fine. Although if the OP didn't know any better they
might take your advice and use it in a situation which could make them
vulnerable.

 

[amdrit]

So after reading through all these replies and drinking more coffee here are
the cliff notes:

Building a SQL string is not the ideal way to go, the community is frowning
on it as there are better methods already defined to facilitate your need.
A couple of reasons exist to not use this method:

--Readability: Code is less readable when you concatinate strings
--Character Escapting: You have to manually perform string manipulation,
while other methods support this internally.
--SQL Injection: You would have to analyze user input to ensure that
someone isn't hijacking their free ride to the database to do as they
please. Other methods natively prevent much of this.
--Security: Application code, and so users, have direct access to your
database objects with the security context that, at the very least, has
permissions to delete data and, at the very worst, drop database objects.
Other methods natively prevent this.
--Performance: Every query that is run against a SQL server, the debate
is still out on all RDMS, is cached to improve perfomance. Dynamic SQL
would have to be exactly the same string each time to benifit from this
otherwise a new cache is created anytime a filter statement was altered.
Paramaterized queries and stored procedures uses a template approach, and
so, allows for variances in filter criteria.

However, if you feel you must build a string, and there are reasons to do
so, it is recommend that you do the following:

--Perform escaping on user input: create a method that replaces a single
quote with two single quotes. formate datetime vaules as datetime strings.
--Use string.format over &: Help make your code more readable with
string.format and avoid using string concantication (&).
--Reserve this method for users with more stake in the application. The
fact remains that this type of SQL is still very dangerous and so the people
who have access to run this should experience that risk should they do
something wrong.

The running debate is weather to use in-line sql vs stored procedures, the
concensus is that it depends on your priorities and a decision process you
have to account for.

Paramaterized Queries support your need to be dynamic, supports the
community's need for your code to minimize SQL injection attacks. The trade
off is lack of security, if this is not a concern for you, then by all means
make use of this method. A benefit is that all your SQL code lives with the
code and so when you deprecate the use of a SQL statement, you are more sure
to remove this code than you would be with a stored procedure.

To use a paramaterized query:

VB
private const sp_insert as string = "Insert into mytable (field1,
field2) values (@field1, @field2)"

public function InsertData(field1 as string, field2 as string) as long
dim cmd as IDBCommand 'in case you wish to support multiple data libraries
...
cmd = dbfactory.createcommand(sp_insert, commandtext)
cmd.paramaters.add(dbfactory.createinparameter("@field1", field1)
cmd.paramaters.add(dbfactory.createinparameter("@field1", field1)
iResult = cmd.executenonquery
'or execute query if you want scope_identity back.
'Note: dbfactory is a made up object to represent a generic factory for
interacting with a one or more database libraries. There are several
examples of this on codeproject.com

Stored Procedures support all the goodness of RDBMS and is, generally, the
most preferred way to interact with the database. Stored procedures can be
granted execute permissions that perform the operations against database
object that you would normally hide from the user (i.e., drop table, alter
table, delete row, insert row and so on).

VB
private const sp_insert as string = "usp_insertrow"

public function InsertData(field1 as string, field2 as string) as long
dim cmd as IDBCommand 'in case you wish to support multiple data libraries
...
cmd = dbfactory.createcommand(sp_insert, procedure)
cmd.paramaters.add(dbfactory.createinparameter("@field1", field1)
cmd.paramaters.add(dbfactory.createinparameter("@field1", field1)
iResult = cmd.executenonquery
'or execute query if you want scope_identity back.
'Note: dbfactory is a made up object to represent a generic factory for
interacting with a one or more database libraries. There are several
examples of this on codeproject.com

If you'll notice the amount of code required to do this in any of the three
options is geared for stored procedures then paramaterized queries and
finally to dynamic sql. You actually have to do more work and get less
benefit for using dynamic sql.

Personal lessons learned (at least reinforced): I came to a company that
has a code base dating back to '95 using VB5/VB6 and all dynamic SQL. I was
brought in to migrate this code base from VB6 to .Net. Among the top
complaints from our customers (beyond the "The look and feel is dated." was
"Performance over the years is slowing.") A simple sql trace at a couple
customer sites (Some 5 concurrent users, some 100+) showed litterally
thousands of ("Select * from users where userid =<somevalue>"). In the
existing code base, my team and I went through and replaced all these
queries with parameters and getting only the columns we wanted ("Select
username, firstname, lastname. datelastpwdchng from users where userid =
@userid", N'@<somevalue>) and redeployed to our customers. They only
noticed that things where happening nearly a quick as they clicked a button
where as before it would take 2-3 sometimes 30 seconds for a button click to
respond (results may very, check with your code base for more details, not
valid in non-participating sweat-shops.) We noticed the performance
counters relaxing more than expoentially across the board. Then there was
the whole "CQ0509082133: Customer wanted to validate code prevented SQL
Injection and so typed ; drop table patients; in the username field on the
login screen and now everyone is receiving errors in their application" -
against production data mess, only to find out that that was the week their
disaster recovery solution was broken, they aren't customers anymore.

 

[Cor Ligthert[MVP]]

Amdrit,


"> However, if you feel you must build a string, and there are reasons to do

so, it is recommend that you do the following:

--formate datetime vaules as datetime strings.

Goes certainly wrong in countries as the Netherlands or Belgium where users
use a workstation setted in the Dutch or French mode while the Server has
almost forever US settings.

--Use string.format over &: Help make your code more readable with
string.format and avoid using string concantication (&).

Why, this is agains everything writen in this newsgroup, do you have a
reference who stated this beside you?

--Reserve this method for users with more stake in the application. The
fact remains that this type of SQL is still very dangerous and so the
people who have access to run this should experience that risk should they
do something wrong.

You cannot choose those who attacking you, do you have a reference who
stated this beside you?

Stored Procedures support all the goodness of RDBMS and is, generally, the
most preferred way to interact with the database. Stored procedures can
be granted execute permissions that perform the operations against
database object that you would normally hide from the user (i.e., drop
table, alter table, delete row, insert row and so on).

You can do that with dynamic Sql as well, do you have a reference who stated
this beside you?

Cor

 

[amdrit]

Cor,

I don't understand your concerns or your wish to challenge this. First and
formost I believe we both agree that Dynamic SQL is not the way to go.
Having said that, I consolidated guidelines for doing so.

Amdrit,


"> However, if you feel you must build a string, and there are reasons to
do

Goes certainly wrong in countries as the Netherlands or Belgium where
users use a workstation setted in the Dutch or French mode while the
Server has almost forever US settings.

Formatting datetime values to strings has to be done since you cannot pass a
date in a SQL statement without doing so. Since you only need to satisfy
the datetime settings of the target SQL server, locality is irrelevant. How
is this an issue? For all I care, pass in an EPOCH date value and let SQL
convert it.

Why, this is agains everything writen in this newsgroup, do you have a
reference who stated this beside you?

Hmm, nope this one was all me addressing readability.

string blah = "sometext" + "sometext" + "sometext" seems less readable than
string blah = string.format("{0}{0}{0}", "sometext") -- So this is my
assertion alone, since it is my comentary I am allowed to interject my
thoughts.

string SQL = string.format("Insert Into Table (Field1, Field2, Field3)
Values({0}, {1}, {2}", "test", 1, MyDateTime.ToString("LongDate")) may seem
trival but when the SQL is much more complex, I think I can present a very
clean view of the code over & or +

You cannot choose those who attacking you, do you have a reference who
stated this beside you?

Say WHAT? Are you not in control of your application? Do you not implement
the workflow for fulfill your user's needs? No one, and I am fairly
confident in this, outside of the development group cares how you make a SQL
statement, they only care that the application works as they expect. No,
anyone is a potential threat, and thus, everyone should be treated as such.
Granting more power to those with more to loose is a generally accepted
approach in workflow, and no I don't have anyone to quote, but you are
welcome to challenge the statement - bring your A game.

In general, normal users perform simple CRUD and rarely have need for
specialized queries. Oh sure there are occasions where developers attempt
to access dataobjects dynamically to gain generality or commonanlity
Select Count(*) from [TableName] where <Criteria> --Access's DCount for
example. And so, you may not beable to restrict who fires this code since
it has been made so generic and most likely incorporated throughout the
code. In this case, you have to test the input values or at a minimum
restrict the points of failure.

public int DCount(TableName table, params[] whereclause)
{
//Params[] is carefully managed by the UI to ensure that values make sense
string sqlcmd = string.empty;
switch(table)
{
case TableName.Users:
cmd.commandtext = string.format("select count(userid) from users");
foreach (param in params)
{
cmd.paramaters.add(dbfactory.createinparameter(param.fieldname,
param.fieldvalue)) //'In the case of a Between clause or an IN statement you
may want to do more work
}
break;
}
return cmd.ExecuteScalar();
}

However, there are times where you just can't/wont break down a SQL
statement into parameters:

public IUserListing GetUserListing(bool fullUser, bool
includeInActiveAccounts, int sortColumn)
{
string blah = string.empty; //stringbuilder would be my preferred option
here, but I don't want to have to defend that as well.

if (fullUser)
blah = "Select userid, username, firstname, lastname, dob,
datelastpwdchng from users ";
else
blah = "Select userid, username, datelastpwdchng from users ";

if (!includeInActiveAccounts)
blah = blah + " Where active = 0 ";

if(sortcolumn > 0 and sortcolumn < 6)
{
blah = blah + " order by ";
switch (sortColumn)
{
case 1:
blah = blah + " userid asc"; //Yeah I know ordering generally
shouldn't be done here, this is just for illustration.
break;
}
}

}

There are other examples as well. All I was proposing here is that when you
make SQL dynamic and allow the user to provide input, you restrict (as much
as you can) who has access to it. Convience store clerks are told to let
robbers take what they want and offer no resistance, while the store owner
might actually challenge the robber. My point is, rank and file endusers
have nothing at stake really and so if careless are likely to break the
application, it disgruntled go out of their way to do harm.

You can do that with dynamic Sql as well, do you have a reference who
stated this beside you?

You know, I really think you read this and thought "Who is this usupur, I'll
fix him with these obtuse statements." Cor, certainly you can issue DML/DDL
statements dynamically. That wasn't the point, the point was to control
when and how those statements were called and to what extent. Our
application is only responsible for it's user base and their actions, we are
not going to attempt to control direct access to the SQL server via
nefarious means beyond our control that is what the IT dept is for. So in
the event that some fool has figured out how to issue a buffer overrun or
issue a sql injection attact against our code, we stop them with good ol
fashioned security on the sql server -- they may have the application, but
the data is still ours. By revoking the user DML/DDL access to the tables
and views and grating execute permissions on a stored proc allows the
application to keep things more inline.

Now, these are my own interruptations of stored proc's, of which, this very
thread had like minded sentiments, since you wanted a reference.

Cor

In my original post, I covered the three ways mentioned throughout the
thread in an attempt to consolidate the responses and throw in my 2 cents at
the same time. You, Cor, took exception to my commentary and challenged my
thinking; however, you did not cite any reason, other than your own
commentary. I don't think we are too far offbase from each other, I think
we just have different rationale or implimentations in mind on the subject.

Please forgive the C# pseudo-code, it has been a while since I've used VB
and C# is so second nature now.

 

[Herfried K. Wagner [MVP]]

Cor --

"> However, if you feel you must build a string, and there are reasons to
do

Goes certainly wrong in countries as the Netherlands or Belgium where
users use a workstation setted in the Dutch or French mode while the
Server has almost forever US settings.

You can also overcome this problem by parsing the date entered by the user
using the current culture and by inserting it to the command string by use
of a parameterized command object (code written from scratch):

\\\

' In production code I'd use 'DateTime.TryParse' instead.
Dim StartDate As DateTime = DateTime.Parse(Me.TextBox1.Text)

Dim Command As New SqlCommand( _
"UPDATE events SET start_date = @start_date WHERE id = @id",
Connection _
)
Command.Parameters.Add("@id", SqlDbType.Int)
Command.Parameters("@id").Value = ...
Command.Parameters.Add("@start_date", SqlDbType.DateTime)
Command.Parameters("@start_date").Value = StartDate
Command.ExecuteNonQuery()
///

If you are using a more appropriate control than a textbox for entering a
date, e.g. the datetimepicker control, no parsing is required at all.

You can do that with dynamic Sql as well, do you have a reference who
stated this beside you?

I'd prefer stored procedures too. However, stored procedures are not a very
"agile" approach because they require setting up a stored procedure on the
database server, which is often maintained by another team member.

 

[Herfried K. Wagner [MVP]]

Hmm, nope this one was all me addressing readability.

string blah = "sometext" + "sometext" + "sometext" seems less readable
than
string blah = string.format("{0}{0}{0}", "sometext") -- So this is my
assertion alone, since it is my comentary I am allowed to interject my
thoughts.

If you are dealing with constants, using '&' is much faster because the
concatenated string literal is determined by the compiler.

string SQL = string.format("Insert Into Table (Field1, Field2, Field3)
Values({0}, {1}, {2}", "test", 1, MyDateTime.ToString("LongDate")) may
seem trival but when the SQL is much more complex, I think I can present a
very clean view of the code over & or +

I suggest to avoid using 'String.Format' to build up SQL command strings.
Instead, use a parameterized command object to guarantee that values are
escaped and formatted properly according to the rules of the DBMS.

 

[Herfried K. Wagner [MVP]]

By the way: http://msdn.microsoft.com/en-us/library/ms998271.aspx

Says at the bottom of the page:

[...]
Note Special input characters pose a threat only with dynamic SQL and
not when using parameterized
SQL.
Escape routines add an escape character to characters that have special
meaning to SQL Server,
thereby making them harmless. This is illustrated in the following code
fragment:

private string SafeSqlLiteral(string inputSQL)
{
return inputSQL.Replace("'", "''");}
}

Perhaps you can suggest that Microsoft change their recommendation.

The recommendation is really dangerous because it won't prevent from SQL
injection (it will only reduce the attack surface slightly). I wonder if
Microsoft would correct the article if somebody sends a bug report using the
"Site Feedback" link on the bottom.

 

[Stewart Berman]

Server control and audit trails are ultimately dependent upon how the
server is setup, not whether or not the SQL is dynamic or not.

Only if you consider the server recover logs as an audit trail. Audit trails in stored procedures
record the business effect of a transaction not the before and after images of the records being
changed. If a connection is authorized to change the contents of a table via a dynamic SQL
statement the business information becomes lost and it becomes much more difficult to determine if
the data was changed in accordance with company policies.

-snip-

If you concatenate the string that represents your stored procedure,
it's no better than any other piece of dynamic SQL.

Agreed, but that is a programming standard that is easily enforced within a database and easy to
review and audit before implementation. Having dynamic SQL statements coming from outside the
database makes it much harder to control.

 

[amdrit]

Herfried,

To illusrtate your point. I made a performance test.

This test combined 6 parts of a string into on. 4 were string values, 1 was
an integer, and 1 was a datetime. The types of tests used were
Concatenation, String.Format, and StringBuilder.

Each type of test processed 100,000 times over a 1000 times. Each loop
varied the order of the tests. Times were recorded by marking the start and
stop times of DateTime.Now.Ticks.

Once the test run was complete, the times were averaged. Here are the
results.

100,000,000 iterations
Concatenation: 2,742,859 ticks or 2.7 milliseconds
StringBuilder: 2,864,465 ticks or 2.9 milliseconds
String Format: 3,123,586 ticks or 3.1 milliseconds

Clearly, concatenation wins out, and string.format is the worse performer.

Since 100 million iterations only result in mere fractions of a millisecond,
I think it is ok to pay attention to code readability that the insignificant
gains. Granted, to each his own.

If this where a server depended upon for high output, then perhaps
concatenation would and should be the way to go. Since this is a UI and we
are fairly confident that the user isn't going to issue 100 million string
concatenations at one time, the former is more ideal to me.

 

[Herfried K. Wagner [MVP]]

To illusrtate your point. I made a performance test.

This test combined 6 parts of a string into on. 4 were string values, 1
was an integer, and 1 was a datetime. The types of tests used were
Concatenation, String.Format, and StringBuilder.

Well, I was only referring to concatenation of string literals (constants),
such as 'x = s & t & u'.

 

[cj2]

I got it. Thanks

Hello cj2


I strongly recomend you to use a Parameterized command object or
alternatively even bether a SQL server SP ( preferable a T-SQL version )
why is a SP in your situation even bether ? A stored procedure is an
database object wich can be granted rights on its own so in the case of a
web project this gives you an extra security layer as a potential hacker
has only the ability to execute your SP code and does not have rights on the
actual tables itself .
Also a SP might be compiled in a execution plan wich can potentially boost
SQL server performance .

I see still daily concanated SQL strings however ditch them ( rewrite ) them
as soon as you can as they are a potential security risk especially when
used in web projects , look up the term SQL injection on Google and show
that to your boss if he still wants you to use concanated SQL strings .

By the way a much simpler solution of creating "correct code" is to just
include a new dataset to your project , double click on the dataset to open
its designer right click , add a table adapter and just add your
parameterized SQL to a fill method .

In the background VS.Net creates everything for you without having to write
anny code ( just a few mouse clicks ) in your code you can now reach your
data through a strong typed dataset by a the table adapter that was
generated in the above steps .

If you would like to know more about this aproach let me know and i write
you a step by step guide

HTH

Michel

 

[Stewart Berman]

The difference between concatenation and StringBuilder is heavily dependent on the length of the
string. In http://www.winnershtriangle.com/w/VBNetHelp_StringBuilderClass.asp each iteration adds
an additional item to the string so it continues to grow. In that type of test StringBuilder is
orders of magnitude faster than concatenation.

However, as you point out the difference is only meaningful if many iterations are done and most UI
code does not do that.

Herfried,

To illusrtate your point. I made a performance test.

This test combined 6 parts of a string into on. 4 were string values, 1 was
an integer, and 1 was a datetime. The types of tests used were
Concatenation, String.Format, and StringBuilder.

Each type of test processed 100,000 times over a 1000 times. Each loop
varied the order of the tests. Times were recorded by marking the start and
stop times of DateTime.Now.Ticks.

Once the test run was complete, the times were averaged. Here are the
results.

100,000,000 iterations
Concatenation: 2,742,859 ticks or 2.7 milliseconds
StringBuilder: 2,864,465 ticks or 2.9 milliseconds
String Format: 3,123,586 ticks or 3.1 milliseconds

Clearly, concatenation wins out, and string.format is the worse performer.

Since 100 million iterations only result in mere fractions of a millisecond,
I think it is ok to pay attention to code readability that the insignificant
gains. Granted, to each his own.

If this where a server depended upon for high output, then perhaps
concatenation would and should be the way to go. Since this is a UI and we
are fairly confident that the user isn't going to issue 100 million string
concatenations at one time, the former is more ideal to me.

 

[Al Reid]

I'm executing a web service call which returns a structure of stings. I
then use the below insert command to store the data in a sql table. My
problem is several pieces of information sometimes contain apostrophes and
though I haven't found them yet possibly parenthesis. An example just
occurred where the full_name and Last_name contained O'Conner. This causes
and exception to be thrown. How do I get around this problem?

Dim MyInsertCmd As New SqlCommand("insert into MyCo.dbo.MyTable " & _
"(account, record_length, number_of_replies, full_name, last_name, " & _
"first_name, address, city, state, zip) " & _
"Values ('" & results.account & "', " & _
"'" & results.record_length & "', " & _
"'" & results.number_of_replies & "', " & _
"'" & results.full_name & "', " & _
"'" & results.last_name & "', " & _
"'" & results.first_name & "', " & _
"'" & results.address & "', " & _
"'" & results.city & "', " & _
"'" & results.state & "', " & _
"'" & results.zip & "')", MySqlConnection)
MySqlConnection.Open()
MyInsertCmd.ExecuteNonQuery()
MySqlConnection.Close()

You already got plenty of answers and lectures about SQL injection problems,
so I won't say any more about it.

It seems to me that if you are strictly retrieving records from a web
service and inserting them into a database, SQL injection is not an issue in
this case and probably the most expeditious way to solve your problem will
be to double up the apostrophes. If this was a multi-user data entry
application or web application I would strongly suggest using the parameters
approach.

As I see it there is little or no risk of SQL injection in this case.

 

[Jack Jackson]

By the way: http://msdn.microsoft.com/en-us/library/ms998271.aspx

Says at the bottom of the page:

[...]
Note Special input characters pose a threat only with dynamic SQL and
not when using parameterized
SQL.
Escape routines add an escape character to characters that have special
meaning to SQL Server,
thereby making them harmless. This is illustrated in the following code
fragment:

private string SafeSqlLiteral(string inputSQL)
{
return inputSQL.Replace("'", "''");}
}

Perhaps you can suggest that Microsoft change their recommendation.

The recommendation is really dangerous because it won't prevent from SQL
injection (it will only reduce the attack surface slightly). I wonder if
Microsoft would correct the article if somebody sends a bug report using the
"Site Feedback" link on the bottom.

 

[Cor Ligthert[MVP]]

I don't understand your concerns or your wish to challenge this. First

and formost I believe we both agree that Dynamic SQL is not the way to go.
Having said that, I consolidated guidelines for doing so.

Did you decide that for me or did you read any reply in this thread.
I can me not remember that I ever wrote that.

Why do you think it exist at all?

Cor