Is this code better than my earlier code, security wise

J

Joey Martin

One of my servers got hacked with the SQL injection due to poor coding.
So, I had someone write a stored procedure and new code.

But, to me, it looks just as flawed, even using the stored procedure.

email=request("email")
password=request("pw")

OLD CODE:
sql="select * from tablename where email='" & email & "' and password='"
& password & "'"
set rs=conn.execute(sql)

NEW CODE
sql="sp_CheckLogin '" & email & "','" & password & "'"
set rs=conn.execute(sql)

Stored Procedure:
CREATE PROCEDURE sp_CheckLogin
@Email VARCHAR(100), @Password VARCHAR(100)
AS
SELECT * FROM tablename WHERE email=@Email AND Password=@Password
GO


Thanks for your help!!
 
B

Bob Barrows [MVP]

Joey said:
One of my servers got hacked with the SQL injection due to poor
coding. So, I had someone write a stored procedure and new code.

But, to me, it looks just as flawed, even using the stored procedure.

email=request("email")
password=request("pw")

OLD CODE:
sql="select * from tablename where email='" & email & "' and
password='" & password & "'"
set rs=conn.execute(sql)

NEW CODE
sql="sp_CheckLogin '" & email & "','" & password & "'"
set rs=conn.execute(sql)

Yes, it is just as flawed.You need to use parameters. Like this:

set rs=createobject("adodb.recordset")
conn.sp_CheckLogin email,password, rs

In addition:
Nothing to do with your problem, but you should avoid using the "sp_"
prefix for your stored procedures. "sp_" indicates to the query engine
that the procedure is a system stored procedure, which means that it
will waste time by first looking for the procedure in the Master
database, only looking in the current database when it is not found in
Master. Granted, the time wasted in very small, but the real problem
comes in when you give your procedure the same name as a current system
procedure. You will find that the system procedure will inexplicably be
executed when you attempt to call your procedure. Instead of trying to
remember to avoid names that conflict with system procedure names, you
can make your life a lot simpler by simply never using "sp_" to prefix
your procedure names unless you are creating a system procedure.
 
J

Joey Martin

Thanks for the information.
In my old code, I was able to successfully run sample sql injection
hacks. In the new SP code, those same sample hacks did not work. In that
case, is it still flawed?

Thanks again.
 
B

Bob Barrows [MVP]

Joey said:
Thanks for the information.
In my old code, I was able to successfully run sample sql injection
hacks. In the new SP code, those same sample hacks did not work. In
that case, is it still flawed?
If you are using & to create dynamic sql statements, then yes, it is
flawed and you are simply not using the hack that exposes the flaw.
Use parameters!
And validate all inputs before using them.
 
O

Old Pedant

In my old code, I was able to successfully run sample sql injection
hacks. In the new SP code, those same sample hacks did not work. In that
case, is it still flawed?

Try something like this with your new code:

password = "foo'; exec sp_HelpText sp_CheckLogin;"

sql="sp_CheckLogin '" & email & "','" & password & "'"
set rs=conn.execute(sql)
set rs = rs.NextRecordSet
Do Until rs.EOF
Response.Write rs(0) & "<br/>"
rs.MoveNext
Loop

Hmmm?? What happens?
 
J

Joey Martin

I see your point, but as far as the latest SQL Injection attacks, would
something like that be common?
 
D

Daniel Crichton

Joey wrote on Thu, 19 Jun 2008 19:33:24 -0700:
I see your point, but as far as the latest SQL Injection attacks, would
something like that be common?


Why bother coding for a specific type of injection attack just because
that's what you're seeing in your logs? You want to code to avoid injection
full stop, otherwise what happens next week when someone comes to your site
and tries out a simple one like the example and ruins your database?
 
J

Joey Martin

Thanks again.

Ok, so by adding this:
set rs=createobject("adodb.recordset")
conn.sp_CheckLogin email,password, rs


instead of this:
sql="sp_CheckLogin '" & email & "','" & password & "'"
set rs=conn.execute(sql)


makes it secure?? If so, I have done it and it works. Just confirming so
I can go thru my other login pages.
 
B

Bob Barrows [MVP]

Joey said:
Thanks again.

Ok, so by adding this:
set rs=createobject("adodb.recordset")
conn.sp_CheckLogin email,password, rs


instead of this:
sql="sp_CheckLogin '" & email & "','" & password & "'"
set rs=conn.execute(sql)


makes it secure?? If so, I have done it and it works. Just confirming
so I can go thru my other login pages.
Yes. Try the Pedant's simple example. Because this technique I suggested
(commonly called the "procedure-as-connection-method technique) passes
values by parameters, the query engine will not attempt to parse and execute
the injected sql. Here is the difference:

When you cobble together a sql statement by concatenating separate strings
together and pass it to a database, you are leaving it up to the database to
treat it the same way it would handle any statement or set of batched
statements handed to it: i.e., it parses the statement(s), and if valid,
compiles a plan and executes the statements, no matter what destructive
actions they perform - hey, you handed this statement to it - you must know
what you are doing, right?

When using parameters on the other hand, you are handing the database a
"tokenized" sql statement or a stored procedure to execute, along with
values for it to substitute for the tokens (or pass as parameters to the
procedure). So it only parses and compiles a plan (if one is not found in
cache) for the sql statement, and then substitutes the tokens with the
supplied values (or passes the values as arguments to the procedure), making
no attempt to parse or execute those supplied values.

So yes, using parameters will foil all primary sql injection attempts.

But you need to go beyond that. If you do not validate all user inputs, you
will wind up with data filled with sql injection attempts. And if you use
that data in dynamic sql in oter places, then you will wind up accomplishing
the hacker's purpose yourself. This is known as "secondary sql injection".

Also, the procedure that was written for you properly uses the parameters in
the sql statement it executes:
WHERE email=@Email AND Password=@Password

See? it's a tokenized sql statement, so passed parameter values will not be
executed.

If, on the other hand, the person who wrote the procedure did something like
this:

set @sql = 'select ... where email=''' + @email + ''''
execute(@sql)

Then you are back at square one: any sql injected into that @email parameter
value will be parsed and executed.
So, when I say that dynamic sql should be avoided, I mean to say it should
be avoided everywhere: in client code and in stored procedure code.

There are situations where dynamic sql cannot be avoided (when you need to
specify column or field names dynamically). In these situations, proper
validation is essential. You must reat the passed data and verify it
contains nothing untoward BEFORE using it in dynamic sql.
 
B

Bob Barrows [MVP]

Neil said:
How is this "tried"? Is it entered into a form field, or somehow
executed as ASP remotely?
It's just executed as is in server-side code as a test. It is supposed to
simulate a hacker entering "foo'; exec sp_HelpText sp_CheckLogin;" into a
form textbox and submitting it.
 
N

Neil Gould

Recently said:
Try something like this with your new code:

password = "foo'; exec sp_HelpText sp_CheckLogin;"

sql="sp_CheckLogin '" & email & "','" & password & "'"
set rs=conn.execute(sql)
set rs = rs.NextRecordSet
Do Until rs.EOF
Response.Write rs(0) & "<br/>"
rs.MoveNext
Loop

Hmmm?? What happens?
How is this "tried"? Is it entered into a form field, or somehow executed
as ASP remotely?

Neil
 
B

Bob Barrows [MVP]

Neil said:
Wouldn't it be good practice to parse & qualify form submissions
before executing any SQL statements? That way, even legitimate entry
errors could be trapped and dealt with appropriately.
Absolutely. I believe I have recommended this several times in this thread.

However, this should only be part of your defensive strategy. The most
airtight defense to sql injection is to never give it a chance to occur.
Since sql injection depends on the use of dynamic sql, then logically, sql
injection attempts can never work if you never use dynamic sql. Of course,
the alternative it to use parameters.

Read my other posts in this thread.
 
N

Neil Gould

Recently said:
It's just executed as is in server-side code as a test. It is
supposed to simulate a hacker entering "foo'; exec sp_HelpText
sp_CheckLogin;" into a form textbox and submitting it.
I see. I was somewhat concerned that there may be some way "in" to the
server-side code from a browser.

Wouldn't it be good practice to parse & qualify form submissions before
executing any SQL statements? That way, even legitimate entry errors could
be trapped and dealt with appropriately.

Neil
 
M

Mike Brind [MVP]

Neil Gould said:
I've followed this thread with interest, and even looked into the
references that you cited.

Having been involved with database design for the last 3+ decades, I think
that much of what is said is just a important for "closed" systems that
have no opportunity to be hacked.

What I'm new to is integrating database functions with ASP, so my security
concerns are mostly in the area of unwanted access to server-side code by
client-side apps or actions.

Be aware that client-side actions can include tampering with querystring
values, saving a local copy of a form, amending the form fields and
submitting it from eg your Desktop etc.
 
N

Neil Gould

Recently said:
Absolutely. I believe I have recommended this several times in this
thread.

However, this should only be part of your defensive strategy. The most
airtight defense to sql injection is to never give it a chance to
occur. Since sql injection depends on the use of dynamic sql, then
logically, sql injection attempts can never work if you never use
dynamic sql. Of course, the alternative it to use parameters.

Read my other posts in this thread.
I've followed this thread with interest, and even looked into the
references that you cited.

Having been involved with database design for the last 3+ decades, I think
that much of what is said is just a important for "closed" systems that
have no opportunity to be hacked.

What I'm new to is integrating database functions with ASP, so my security
concerns are mostly in the area of unwanted access to server-side code by
client-side apps or actions.

Neil
 
N

Neil Gould

Recently said:
Neil Gould said:
[...]
What I'm new to is integrating database functions with ASP, so my
security concerns are mostly in the area of unwanted access to
server-side code by client-side apps or actions.

Be aware that client-side actions can include tampering with
querystring values, saving a local copy of a form, amending the form
fields and submitting it from eg your Desktop etc.
Yes, I am aware of those possibilities, and that is why I'm curious about
other such opportunities.

Offhand, though, I don't know what advantage there would be to amending
form fields unless there is also some access to the code that the form
calls. If someone changes the field names in the form without such access,
the form submission should just fail. Added fields should have no impact,
as there isn't code to support the new fields. Any examples of the kind of
risk you had in mind?

Neil
 
M

Mike Brind [MVP]

Neil Gould said:
Recently said:
Neil Gould said:
[...]
What I'm new to is integrating database functions with ASP, so my
security concerns are mostly in the area of unwanted access to
server-side code by client-side apps or actions.

Be aware that client-side actions can include tampering with
querystring values, saving a local copy of a form, amending the form
fields and submitting it from eg your Desktop etc.
Yes, I am aware of those possibilities, and that is why I'm curious about
other such opportunities.

Offhand, though, I don't know what advantage there would be to amending
form fields unless there is also some access to the code that the form
calls. If someone changes the field names in the form without such access,
the form submission should just fail. Added fields should have no impact,
as there isn't code to support the new fields. Any examples of the kind of
risk you had in mind?

LOL. Straight off the top of my head, I once found a site that had the
actual SQL that was being executed stored in a hidden field. Get the
picture?
 
B

Bob Barrows [MVP]

Neil said:
Recently said:
Neil Gould said:
[...]
What I'm new to is integrating database functions with ASP, so my
security concerns are mostly in the area of unwanted access to
server-side code by client-side apps or actions.

Be aware that client-side actions can include tampering with
querystring values, saving a local copy of a form, amending the form
fields and submitting it from eg your Desktop etc.
Yes, I am aware of those possibilities, and that is why I'm curious
about other such opportunities.

Offhand, though, I don't know what advantage there would be to
amending form fields unless there is also some access to the code
that the form calls. If someone changes the field names in the form
without such access, the form submission should just fail. Added
fields should have no impact, as there isn't code to support the new
fields. Any examples of the kind of risk you had in mind?
I've seen people try to create generic form processing pages that looped
through submitted form fields, dynamically creating variables to contain the
submitted data ... ugh!
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,768
Messages
2,569,574
Members
45,048
Latest member
verona

Latest Threads

Top