INSERT script to SQL Server

[teddysnips]

A friend has been tasked with designing a website. It all looks very
swish, but he's been asked to add a page to allow people to register
for events etc. He wants to add some script to save these details to
a SQL Server database. Could someone please take a brief look at the
script below and tell me if this looks ok?

Thanks

Edward

<%
Set Conn = Server.CreateObject("ADODB.Connection")

Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial Catalog=Test;Data
Source=YOURSERVERHERE;"

sqlstring = "INSERT INTO UserDetails (UserName, Tel, Email) VALUES
('"

sqltemp = document.getElementById('UserName').value
sqlstring = sqlstring + sqltemp + "', '"

sqltemp = document.getElementById('Tel').value
sqlstring = sqlstring + sqltemp + "', '"

sqltemp = document.getElementById('Email').value
sqlstring = sqlstring + sqltemp + "', ')"

conn.execute(sqlstring)
conn.close
set conn = nothing
%>

 

[Evertjan.]

wrote on 14 feb 2008 in microsoft.public.inetserver.asp.general:

A friend has been tasked with designing a website. It all looks very
swish, but he's been asked to add a page to allow people to register
for events etc. He wants to add some script to save these details to
a SQL Server database. Could someone please take a brief look at the
script below and tell me if this looks ok?

Thanks

Edward

<%
Set Conn = Server.CreateObject("ADODB.Connection")

Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial Catalog=Test;Data
Source=YOURSERVERHERE;"

sqlstring = "INSERT INTO UserDetails (UserName, Tel, Email) VALUES
('"

sqltemp = document.getElementById('UserName').value
sqlstring = sqlstring + sqltemp + "', '"

This would be very dangerous on the web.
Read up on SQL injection.
Always validate data comming from clientside against this.
Bob even has a better alternative that I never use.

 

[Bob Barrows [MVP]]

A friend has been tasked with designing a website. It all looks very
swish, but he's been asked to add a page to allow people to register
for events etc. He wants to add some script to save these details to
a SQL Server database. Could someone please take a brief look at the
script below and tell me if this looks ok?

Thanks

Edward

<%
Set Conn = Server.CreateObject("ADODB.Connection")

Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial Catalog=Test;Data
Source=YOURSERVERHERE;"

sqlstring = "INSERT INTO UserDetails (UserName, Tel, Email) VALUES
('"

sqltemp = document.getElementById('UserName').value
sqlstring = sqlstring + sqltemp + "', '"

& should be used for string concatenation, not +.

sqltemp = document.getElementById('Tel').value
sqlstring = sqlstring + sqltemp + "', '"

sqltemp = document.getElementById('Email').value
sqlstring = sqlstring + sqltemp + "', ')"

conn.execute(sqlstring)
conn.close
set conn = nothing
%>

Your friend's use of dynamic sql is leaving you vulnerable to hackers using
sql
injection:
http://mvp.unixwiz.net/techtips/sql-injection.html
http://www.sqlsecurity.com/DesktopDefault.aspx?tabid=23

See here for a better, more secure way to execute your queries by using
parameter markers:
http://groups-beta.google.com/group/microsoft.public.inetserver.asp.db/msg/72e36562fee7804e


Personally, I prefer using stored procedures:
http://groups.google.com/group/microsoft.public.inetserver.asp.general/msg/5d3c9d4409dc1701?hl=en&

Using parameters is only one weapon that should be used to counter sql
injection: data validation should also be performed. Do not depend on
client-side validation. Always validate user input in server-side code

 

[teddysnips]

On 14 Feb, 12:26, "Bob Barrows [MVP]" <[email protected]>
wrote:
[...]

Your friend's use of dynamic sql is leaving you vulnerable to hackers using
sql
injection:http://mvp.unixwiz.net/techtips/sql....sqlsecurity.com/DesktopDefault.aspx?tabid=23

See here for a better, more secure way to execute your queries by using
parameter markers:http://groups-beta.google.com/group/microsoft.public.inetserver.asp.d...

Personally, I prefer using stored procedures:http://groups.google.com/group/microsoft.public.inetserver.asp.genera...

Using parameters is only one weapon that should be used to counter sql
injection: data validation should also be performed. Do not depend on
client-side validation. Always validate user input in server-side code

Thanks. I've taken a look at the sites you suggest and have come up
with two possibilities derived from the various suggestions:

<%
Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

sqlstring = "INSERT INTO UserDetails (UserName, Email) VALUES
(@UserName, @Email)"
cmd = new SqlCommand(sqlstring)
cmd.Connection = Conn

' Add UserName to the above defined paramter @UserName
cmd.Parameters.Add(
new SqlParameter(
"@UserName", System.Data.SqlDbType.NVarChar, 255, "UserName"))

cmd.Parameters["@UserName"].Value =
document.getElementById('UserName').value

' Add Email to the above defined paramter @Email
cmd.Parameters.Add(
new SqlParameter(
"@Email", System.Data.SqlDbType.NVarChar, 100, "Email"))

cmd.Parameters["@Email"].Value =
document.getElementById('Email').value

' Execute the query
cmd.ExecuteReader()

conn.close
set conn = nothing
set cmd = nothing
%>

OR

<%

Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

strSQL = "exec strMySPROC @UserName='" &
document.getElementById('Email').value & "', @Email= '" &
document.getElementById('Email').value & '"


conn.Execute(strSQL)
%>

Am I doing this right?

Thanks

Edward

 

[Bob Barrows [MVP]]

On 14 Feb, 12:26, "Bob Barrows [MVP]" <[email protected]>
wrote:
[...]

Your friend's use of dynamic sql is leaving you vulnerable to
hackers using
sql
injection:http://mvp.unixwiz.net/techtips/sql....sqlsecurity.com/DesktopDefault.aspx?tabid=23

See here for a better, more secure way to execute your queries by
using
parameter
markers:http://groups-beta.google.com/group/microsoft.public.inetserver.asp.d...

Personally, I prefer using stored
procedures:http://groups.google.com/group/microsoft.public.inetserver.asp.genera...

Using parameters is only one weapon that should be used to counter
sql
injection: data validation should also be performed. Do not depend on
client-side validation. Always validate user input in server-side
code

Thanks. I've taken a look at the sites you suggest and have come up
with two possibilities derived from the various suggestions:

None of my examples resemble what follows. But continue.

<%
Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

sqlstring = "INSERT INTO UserDetails (UserName, Email) VALUES
(@UserName, @Email)"

The above statements are vbscript code. Now, for some strange reason, you
switch to VB.Net??

cmd = new SqlCommand(sqlstring)
cmd.Connection = Conn

You cannot assign an ADODB connection object to a .Net SQLCommand object.

' Add UserName to the above defined paramter @UserName
cmd.Parameters.Add(
new SqlParameter(
"@UserName", System.Data.SqlDbType.NVarChar, 255, "UserName"))

cmd.Parameters["@UserName"].Value =
document.getElementById('UserName').value

' Add Email to the above defined paramter @Email
cmd.Parameters.Add(
new SqlParameter(
"@Email", System.Data.SqlDbType.NVarChar, 100, "Email"))

cmd.Parameters["@Email"].Value =
document.getElementById('Email').value

' Execute the query
cmd.ExecuteReader()

conn.close
set conn = nothing
set cmd = nothing
%>

???
You are mixing vbscript and VB.Net. They are not the same!!!

OR

<%

Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

strSQL = "exec strMySPROC @UserName='" &
document.getElementById('Email').value & "', @Email= '" &
document.getElementById('Email').value & '"

No, this is just as bad as your original example. You have created a stored
procedure and undone your good work by using dynamic sql to execute it!! The
data in that xml document is the result of user input, is it not? What if a
malicious user input something like this into the email field:

';truncate table userdetails; --

Using concatenation, that statement would get executed.

Go back and read
http://groups.google.com/group/microsoft.public.inetserver.asp.general/msg/5d3c9d4409dc1701?hl=en&

conn.Execute(strSQL)
%>

Am I doing this right?

No. First of all, you need to decide it you are creating an ASP.Net page
(its extension would be .aspx) or a classic asp page (its extension would be
..asp). If you are doing ASP.Net, then this is not the correct newsgroup: try
microsoft.public.dotnet.framework.aspnet or the forums at www.asp.net.

If you are doing classic asp, then you need to go back and reread the
examples I posted.

 

[teddysnips]

On 14 Feb, 16:54, "Bob Barrows [MVP]" <[email protected]>
wrote:

[...]

If you are doing classic asp, then you need to go back and reread the
examples I posted.

Ok, third time lucky! By the way, thanks for your patience. You're
clearly an adherent of the "teach a man to fish" school ;¬}

<%
Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

strUserName = document.getElementById('UserName').value
strEmail = document.getElementById('Email').value
conn.stpMyProcedure strUserName, strEmail

conn.close
set conn = nothing

%>

 

[Bob Barrows [MVP]]

On 14 Feb, 16:54, "Bob Barrows [MVP]" <[email protected]>
wrote:

[...]

If you are doing classic asp, then you need to go back and reread the
examples I posted.

Ok, third time lucky! By the way, thanks for your patience. You're
clearly an adherent of the "teach a man to fish" school ;¬}

<%
Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

strUserName = document.getElementById('UserName').value
strEmail = document.getElementById('Email').value
conn.stpMyProcedure strUserName, strEmail

conn.close
set conn = nothing

%>

There you go - you're halfway there! The other half is to make sure nothing
invalid or malicious is in those strUserName and strEmail variables BEFORE
opening the database connection. Again, don't depend on client-side
validation. The place to perform validation is right here in the server-side
code.

 

[teddysnips]

On 14 Feb, 16:54, "Bob Barrows [MVP]" <[email protected]>
wrote:

[...]

If you are doing classic asp, then you need to go back and reread the
examples I posted.

Ok, third time lucky!  By the way, thanks for your patience.  You're
clearly an adherent of the "teach a man to fish" school ;¬}

<%
   Set Conn = Server.CreateObject("ADODB.Connection")
   Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
      Catalog=Test;Data Source=YOURSERVERHERE;"

   strUserName = document.getElementById('UserName').value
   strEmail = document.getElementById('Email').value
   conn.stpMyProcedure strUserName, strEmail

   conn.close
   set conn = nothing

%>

There you go - you're halfway there! The other half is to make sure nothing
invalid or malicious is in those strUserName and strEmail variables BEFORE
opening the database connection. Again, don't depend on client-side
validation. The place to perform validation is right here in the server-side
code.

We're only concerned here with text - there are no numeric or date
fields. Is it sufficient simply to pass strings through a function
like:

FUNCTION FixString(str)
FixString = replace(str,"'","''")
END FUNCTION


As ever, I'm very grateful for the time and patience you have shown -
thanks

Edward

 

[Bob Barrows [MVP]]

On 14 Feb, 16:54, "Bob Barrows [MVP]" <[email protected]>
wrote:

[...]

If you are doing classic asp, then you need to go back and reread
the examples I posted.

Ok, third time lucky! By the way, thanks for your patience. You're
clearly an adherent of the "teach a man to fish" school ;¬}

<%
Set Conn = Server.CreateObject("ADODB.Connection")
Conn.Open "Provider=SQLOLEDB.1;Password=pwd;Persist Security
Info=True;User ID=uid;Initial
Catalog=Test;Data Source=YOURSERVERHERE;"

strUserName = document.getElementById('UserName').value
strEmail = document.getElementById('Email').value
conn.stpMyProcedure strUserName, strEmail

conn.close
set conn = nothing

%>

There you go - you're halfway there! The other half is to make sure
nothing invalid or malicious is in those strUserName and strEmail
variables BEFORE opening the database connection. Again, don't
depend on client-side validation. The place to perform validation is
right here in the server-side code.

We're only concerned here with text - there are no numeric or date
fields. Is it sufficient simply to pass strings through a function
like:

FUNCTION FixString(str)
FixString = replace(str,"'","''")
END FUNCTION

No, this step is absolutely not necessary when using parameters: it will
only result in your storing double apostrophes in the database. People
sometimes think that this type of function makes their dynamic sql safe, but
experienced hackers can easily get past this protection.

What I am talking about is, in addition to verifying email addresses are in
the proper format: also check the user input for malicious hack attempts:
check the data for the presence of sql keywords and function names that
absolutely should not be there (DELETE, TRUNCATE, DROP, xp_cmdshell, etc.).
How you wish to respond to such attempts is up to you. But there is no need
to allow the insertion of such hack attempts into your database, is there?

 

[teddysnips]

On 15 Feb, 12:55, "Bob Barrows [MVP]" <[email protected]>
wrote:
[...]

What I am talking about is, in addition to verifying email addresses are in
the proper format: also check the user input for malicious hack attempts:
check the data for the presence of sql keywords and function names that
absolutely should not be there (DELETE, TRUNCATE, DROP, xp_cmdshell, etc.).
How you wish to respond to such attempts is up to you. But there is no need
to allow the insertion of such hack attempts into your database, is there?

I agree, but I have some reservations. I've written other systems
where e-mail addresses need to be validated and I'm sure I can dig out
a reliable RegExp for this. But is there a finite number of sql
keywords and function names that need to be checked? The ingenuity of
the cracker community presumably means that we need a pretty robust
approach. In all the sites and pages you've helpfully provided, I
haven't seen any that explain how to go about checking for the
presence of potentially dangerous words. For example, thinking on the
fly, what if your surname was Dropman - ok, I've never met anyone
called that, but it's not impossible?

Thanks

Edward

 

[Bob Barrows [MVP]]

On 15 Feb, 12:55, "Bob Barrows [MVP]" <[email protected]>
wrote:
[...]

What I am talking about is, in addition to verifying email addresses
are in the proper format: also check the user input for malicious
hack attempts: check the data for the presence of sql keywords and
function names that absolutely should not be there (DELETE,
TRUNCATE, DROP, xp_cmdshell, etc.). How you wish to respond to such
attempts is up to you. But there is no need to allow the insertion
of such hack attempts into your database, is there?

I agree, but I have some reservations. I've written other systems
where e-mail addresses need to be validated and I'm sure I can dig out
a reliable RegExp for this. But is there a finite number of sql
keywords and function names that need to be checked? The ingenuity of
the cracker community presumably means that we need a pretty robust
approach.

Not really. The initial hack attempts are likely to be simple exploits
that should be easily caught by checking for the existence of a few sql
keywords. By the time they start escalating to seriously devious
attempts, you or the web admin should have been alerted to the initial
attempts by the validation routine, and can intervene personally.

In all the sites and pages you've helpfully provided, I
haven't seen any that explain how to go about checking for the
presence of potentially dangerous words. For example, thinking on the
fly, what if your surname was Dropman - ok, I've never met anyone
called that, but it's not impossible?

That's the rub, sure enough. One thing to keep in mind is to check for
words. For example, DROP is always followed by a space in a sql
statement, so you would check for "DROP ", not "DROP". Another thing to
check for is apostrophes, semicolons, plus symbols and double-hyphens,
which will typically be used in simple sql injection exploits. The
presence of an apostrophe in the same string containing the word "select
" should be a huge tipoff. This page has some good tips that can be
applied in vbscript:
http://h71028.www7.hp.com/ERC/cache/571032-0-0-0-121.html&ERL=true?jumpid=reg_R1002_USEN

 

[Dave Anderson]

What I am talking about is, in addition to verifying email addresses
are in the proper format: also check the user input for malicious
hack attempts: check the data for the presence of sql keywords and
function names that absolutely should not be there (DELETE, TRUNCATE,
DROP, xp_cmdshell, etc.).

I'm confused, Bob. How could those values be problematic in the SP-as-method
approach[1]? What if the database was for a forum and contained this
conversation?

I can think of no reason to be that kind of content police.



[1] Assuming the SP does not use EXEC(string containing parameter data)

 

[Bob Barrows [MVP]]

What I am talking about is, in addition to verifying email addresses
are in the proper format: also check the user input for malicious
hack attempts: check the data for the presence of sql keywords and
function names that absolutely should not be there (DELETE, TRUNCATE,
DROP, xp_cmdshell, etc.).

I'm confused, Bob. How could those values be problematic in the
SP-as-method approach[1]? What if the database was for a forum and
contained this conversation?

I can think of no reason to be that kind of content police.

I can think of several:

1. There is no point in putting this garbage into the database
2. A web admin can be notified that a hacker is attempting a crack
before the hacker escalates to more sophisticated techniques
3. The programmer can make it more uncomfortable for the hacker, perhaps
redirecting him to a page that takes forever to load in hopes of
persuading him to stop wasting time with this site.
4. If this crap is allowed to be put into the database, secondary sql
injection is now a possibility, since the programmer may trust the data
that is stored in the database and fail to validate it properly or use
it in dynamic sql statements

 

[Dave Anderson]

I'm confused, Bob. How could those values be problematic in
the SP-as-method approach[1]? What if the database was for a
forum and contained this conversation?

I can think of no reason to be that kind of content police.

1. There is no point in putting this garbage into the database

So it would be pointless to store this conversation in a database?

2. A web admin can be notified that a hacker is attempting a
crack before the hacker escalates to more sophisticated techniques

You didn't really answer my question. HOW are those values problematic? What
is the SQL injection vulnerability when the SP-as-method technique is
employed and EXEC(dynamicString) is forbidden? AFAIC, sieving for SQL
keywords is just bloat.

3. The programmer can make it more uncomfortable for the hacker,
perhaps redirecting him to a page that takes forever to load in
hopes of persuading him to stop wasting time with this site.

The issue is not the comfort or convenience for the attacker. The issue is
vulnerability. If there is no injection threat, then let him sit in comfort
and watch his SQL keywords show up as plain text in the page output. He'll
get the point and move on.

4. If this crap is allowed to be put into the database, secondary
sql injection is now a possibility, since the programmer may trust
the data that is stored in the database and fail to validate it
properly or use it in dynamic sql statements

I would fire a programmer that used EXEC(dynamicString).

Validation is a completely different issue, BTW. My question concerned
vulnerability.

 

[Bob Barrows [MVP]]

I'm confused, Bob. How could those values be problematic in
the SP-as-method approach[1]? What if the database was for a
forum and contained this conversation?

I can think of no reason to be that kind of content police.

1. There is no point in putting this garbage into the database

So it would be pointless to store this conversation in a database?

I missed the part about the hypothetical forum database. Of course, the
validation routine would need to conform to the type of data being stored. I
was responding to allowing this type of data for email addresses and user
names.

You didn't really answer my question. HOW are those values
problematic? What is the SQL injection vulnerability when the
SP-as-method technique is employed and EXEC(dynamicString) is
forbidden? AFAIC, sieving for SQL keywords is just bloat.

Given those two pre-conditions, there is no sql injection vulnerability.
However, the hacker could escalate to different types of exploits once he is
convinced that sql injection is locked down, especially if he is not
"penalized" for these attempts

The issue is not the comfort or convenience for the attacker. The
issue is vulnerability. If there is no injection threat, then let him
sit in comfort and watch his SQL keywords show up as plain text in
the page output. He'll get the point and move on.

Not necessarily. True, he may get the point and move on, but we can increase
the likelihood of his moving on by slowing down his attempts. If he is using
a script to programattically make his hack attempts, we may be able to
disrupt that script and at least force him to make his attempts manually.

Of course, a determined hacker (especially one with an axe to grind) may not
even be deterred by this.

I would fire a programmer that used EXEC(dynamicString).

It's not just the issue of using it in T-SQL ... there is also the issue of
the retrieving data in a recordset and failing to treat that data as user
input.

Not everyone has your awareness of security. Hopefully, people reading this
thread will gain a better understanding of the basic security principles you
have already learned. My advice was aimed at them, not at you.

Validation is a completely different issue, BTW. My question concerned
vulnerability.

I should have said: "fail to validate it properly before using it in dynamic
sql statements"

 

[Adrienne Boswell]

Not everyone has your awareness of security. Hopefully, people reading
this thread will gain a better understanding of the basic security
principles you have already learned. My advice was aimed at them, not
at you.

I have been reading this thread very carefully, in light of the fact that
our server _did_ have something happen to it. Luckily, it was the web
server, not the db server. I'm still investigating exactly was what left
behind, and how to prevent it from taking site site down again, or worse,
spreading to the db server.