Is this code better than my earlier code, security wise

Discussion in 'ASP General' started by Joey Martin, Jun 19, 2008.

  1. Joey Martin

    Joey Martin Guest

    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!!


    *** Sent via Developersdex http://www.developersdex.com ***
    Joey Martin, Jun 19, 2008
    #1
    1. Advertising

  2. Joey Martin wrote:
    > 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.




    --
    Microsoft MVP -- ASP/ASP.NET
    Please reply to the newsgroup. The email account listed in my From
    header is my spam trap, so I don't check it very often. You will get a
    quicker response by posting to the newsgroup.
    Bob Barrows [MVP], Jun 19, 2008
    #2
    1. Advertising

  3. Joey Martin

    Joey Martin Guest

    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.



    *** Sent via Developersdex http://www.developersdex.com ***
    Joey Martin, Jun 19, 2008
    #3
  4. Joey Martin wrote:
    > 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.

    --
    Microsoft MVP -- ASP/ASP.NET
    Please reply to the newsgroup. The email account listed in my From
    header is my spam trap, so I don't check it very often. You will get a
    quicker response by posting to the newsgroup.
    Bob Barrows [MVP], Jun 19, 2008
    #4
  5. Joey Martin

    Old Pedant Guest

    > 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?
    Old Pedant, Jun 20, 2008
    #5
  6. Joey Martin

    Joey Martin Guest

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



    *** Sent via Developersdex http://www.developersdex.com ***
    Joey Martin, Jun 20, 2008
    #6
  7. 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?

    --
    Dan
    Daniel Crichton, Jun 20, 2008
    #7
  8. Joey Martin

    Joey Martin Guest

    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.



    *** Sent via Developersdex http://www.developersdex.com ***
    Joey Martin, Jun 20, 2008
    #8
  9. Joey Martin wrote:
    > 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.


    --
    Microsoft MVP - ASP/ASP.NET
    Please reply to the newsgroup. This email account is my spam trap so I
    don't check it very often. If you must reply off-line, then remove the
    "NO SPAM"
    Bob Barrows [MVP], Jun 20, 2008
    #9
  10. Joey Martin

    Joey Martin Guest

    Joey Martin, Jun 20, 2008
    #10
  11. Joey Martin

    Joey Martin Guest

    Joey Martin, Jun 20, 2008
    #11
  12. Neil Gould wrote:
    > Recently, Old Pedant <> posted:
    >
    >>> 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?
    >>

    > 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.

    --
    Microsoft MVP - ASP/ASP.NET
    Please reply to the newsgroup. This email account is my spam trap so I
    don't check it very often. If you must reply off-line, then remove the
    "NO SPAM"
    Bob Barrows [MVP], Jun 20, 2008
    #12
  13. Joey Martin

    Neil Gould Guest

    Recently, Old Pedant <> posted:

    >> 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?
    >

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

    Neil
    Neil Gould, Jun 20, 2008
    #13
  14. Neil Gould wrote:
    > 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.

    --
    Microsoft MVP - ASP/ASP.NET
    Please reply to the newsgroup. This email account is my spam trap so I
    don't check it very often. If you must reply off-line, then remove the
    "NO SPAM"
    Bob Barrows [MVP], Jun 20, 2008
    #14
  15. Joey Martin

    Neil Gould Guest

    Recently, Bob Barrows [MVP] <> posted:

    > Neil Gould wrote:
    >> Recently, Old Pedant <> posted:
    >>
    >>>> 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?
    >>>

    >> 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.
    >

    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
    Neil Gould, Jun 20, 2008
    #15
  16. "Neil Gould" <> wrote in message
    news:%...
    > Recently, Bob Barrows [MVP] <> posted:
    >
    >> Neil Gould wrote:
    >>> 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.
    >>

    > 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.

    --
    Mike Brind
    Microsoft MVP - ASP/ASP.NET
    Mike Brind [MVP], Jun 20, 2008
    #16
  17. Joey Martin

    Neil Gould Guest

    Recently, Bob Barrows [MVP] <> posted:

    > Neil Gould wrote:
    >> 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.
    >

    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
    Neil Gould, Jun 20, 2008
    #17
  18. Joey Martin

    Neil Gould Guest

    Recently, Mike Brind [MVP] <> posted:

    > "Neil Gould" <> wrote in message
    >> [...]
    >> 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
    Neil Gould, Jun 21, 2008
    #18
  19. "Neil Gould" <> wrote in message
    news:u6%...
    > Recently, Mike Brind [MVP] <> posted:
    >
    >> "Neil Gould" <> wrote in message
    >>> [...]
    >>> 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?

    --
    Mike Brind
    Microsoft MVP - ASP/ASP.NET
    Mike Brind [MVP], Jun 23, 2008
    #19
  20. Neil Gould wrote:
    > Recently, Mike Brind [MVP] <> posted:
    >
    >> "Neil Gould" <> wrote in message
    >>> [...]
    >>> 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!

    --
    Microsoft MVP - ASP/ASP.NET
    Please reply to the newsgroup. This email account is my spam trap so I
    don't check it very often. If you must reply off-line, then remove the
    "NO SPAM"
    Bob Barrows [MVP], Jun 23, 2008
    #20
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Rogue Chameleon
    Replies:
    16
    Views:
    572
    Gerbrand van Dieijen
    Nov 26, 2004
  2. Peter Bencsik
    Replies:
    2
    Views:
    822
  3. Enrique Cruiz

    Row-wise vs. column-wise image processing

    Enrique Cruiz, Jan 25, 2007, in forum: C Programming
    Replies:
    10
    Views:
    717
    christian.bau
    Jan 26, 2007
  4. Enrique Cruiz
    Replies:
    5
    Views:
    415
    Jim Langston
    Jan 25, 2007
  5. Showjumper

    Performance wise whats better?

    Showjumper, Mar 3, 2005, in forum: ASP .Net Building Controls
    Replies:
    4
    Views:
    122
Loading...

Share This Page