Comma Separated Parameter.



  • If this is not the right section for this, I apologize.  I looked at all the sections of this forum, and this one seemed to be the best fit to me.

    As I've mentioned in other threads, I'm a fresh-out-of-college newbie.  While I do consider my self capable and ready to learn, there's just some stuff I don't know yet.  Like the best way to handle parameters in stored procedures in some situations.

    I'm looking at some T-SQL stored procedure code that accepts a comma delimited string as one of its parameters.  The user of the front end selects a few things and I guess since that user could potentially select anywhere from 1 thing to 8 things, the author of this SP descided it would be best to just have the user's selections represented by one comma delimited string parameter.  The SP then processes that string appropriately.

    After seeing this, my WTF-o-meter started to move, but then became unsure of itself.  Is this a good way to do things?  If not, why, and what would a better way be?

    I appreciate any help and advice you all can give me.



  • OK a few thoughts.

    If the maximum number of parameters is 8 things then use 8 parameters, and allow nulls as default values for the ones that are not used.

    Passing in and use a parameter in dynamic sql (for example a select where whatever in @parameterName) is very open to sql injection attacks.

    If you must do it with a comma seperated list I'd write a user defined function to split the values into a temporary table, then use that to avoid any dynamic SQL, which is a bad bad thing.



  • <FONT style="BACKGROUND-COLOR: #ffffff">Well, the max number at the moment is 8, but that could very easily change.</FONT>

    Currently, this comma separated list is just broken up and the values are put into the appropriate table, so I don't think I have to worry about SQL injection attacks.  At least, not with this part of the system.

     

    Is all dynamic SQL inherently bad?  I've read quite a bit about SQL injection attacks and how they are to be avoided and I agree; they're scary.  But it seems like you'd lose quite a bit of functionality to give up dynamic SQL altogether.  How else can you select some data based on user input?  Perhaps I'm just misunderstanding what dynamic SQL is exactly.  Please know that I'm asking honestly and humbly, and that I appreciate your answers.


  • ♿ (Parody)

    @UncleMidriff said:

    If this is not the right section for this, I apologize.  I looked at all the sections of this forum, and this one seemed to be the best fit to me. [...]

    I'm looking at some T-SQL stored procedure code that accepts a comma delimited string as one of its parameters.

    Actually, glad to see posts like this! It's general discussion, so anything (cept spam, flaming) is fair game.

    As for your question, I agree with the other poster: that should be broken down into 8 params, not one param. That defeats the purpose of params if you dump all in one. On top of that, that domain specific information (string list, 0-8 in size) is not conveyed in just a param name (unless of course it is "@SomeCsvListWith0To8Integers").

    @UncleMidriff said:

    Is all dynamic SQL inherently bad?  I've read quite a bit about SQL injection attacks and how they are to be avoided and I agree; they're scary.  But it seems like you'd lose quite a bit of functionality to give up dynamic SQL altogether.  How else can you select some data based on user input?  Perhaps I'm just misunderstanding what dynamic SQL is exactly.  Please know that I'm asking honestly and humbly, and that I appreciate your answers.

    I don't think there's an official definition, but here's what I go by.

    Inline SQL is an explciit query called by middle tier code. It can be done safely or unsafely, here is some sample psuedo code

    unsafeSql = "select * from tb where col='" & someVal & "'"
    cmd.Execute(unsafeSQL)

    safeSql = "select * from tb where col=?"
    cmd.Parameters.Add("col",someVal)
    cmd.Execute(safeSQL)

    I dont think there is anything wrong with using inline SQL, so long as you use parameters. As for dynamic SQL, that's a query executed within another query as a string. EG:

    set @tbName = 'tb'
    exec 'select * from [' + @tbName + ']'

    This is also susceptable to injection attacks, but that can easily be avoided by *never* allowing input params into your dynamic sql statements (or using sp_executesql when possible, and using parameters with that) or thourouly validating them.

    Some things can only be accomplished with dynamic sql (select top X rows), but its best to avoid it unless it makes the solution simpler.



  • I apologize if I am being exceptionally dense, but I still have a few questions:

    Inline SQL is an explciit query called by middle tier code. It can be done safely or unsafely, here is some sample psuedo code

    unsafeSql = "select * from tb where col='" & someVal & "'"
    cmd.Execute(unsafeSQL)

    safeSql = "select * from tb where col=?"
    cmd.Parameters.Add("col",someVal)
    cmd.Execute(safeSQL)

    I dont think there is anything wrong with using inline SQL, so long as you use parameters.

    I see the difference in those two bits of code, and I trust you when you say the latter is safer than the former, but I don't understand where the extra safety in the latter comes from.  My more experienced coworkers have often told me that using parameters is safer, but none of them have been able to explain why.  I'll google it some more later this evening, but I am interested in your explanation too, if you've the time and inclination to give it.

    The place where I work *despises* inline SQL, so we do pretty much everything through stored procedures.  Here recently, since our database lady wasn't at work that day, I had to write a little stored procedure which recieved a course number as input and returned all the sections of that course currently available.  The idea was that a user could select a course from a combo box on the front end webpage, and then that selection would be sent to the SP.  The incredibly simple SP essentially went like this:

    "CREATE PROCEDURE The_SP_What_Does_Stuff

    @CourseNumber

    AS

    select <some stuff> from S_C_S_L where C_S = @CourseNumber

    GO"

    Is this incredibly vulnerable to SQL injection attacks, and if so, how?  Given my limited understanding of the issue, I just don't see how it is.

    And yeah, Payback is still hell. :)


  • ♿ (Parody)

    @UncleMidriff said:

     ... I don't understand where the extra safety in the latter comes from.  My more experienced coworkers have often told me that using parameters is safer ...

    For the most part because you'll be entrusting the safety of user-inputted data arriving to the database in its indented form to the database drivers instead of your own code. It's too easy to make mistakes in your own code, even when you try to clean the input. Here's a good example (ASP VBScript):

    Function DbClean(s) : DbClean = Replace(s,"'","''") : End Function

    colVal = Request("colVal") : rec_no = Request("rec_no")

    Conn.Execute("UPDATE tb SET col='" & DbClean(colVal) & "' WHERE rec_no=" & DbClean(rec_no))

    Seems safe enough, but you're still susceptible to an injection attack. When rec_no is set to " 1 or ' = ' ", you are in for a world of hurt. So now you have to CInt() or CDate() everything before concatenating it to your string. And then on top of that you'll have to find some way of updating/inserting NULL values which can be a fun pain. It becomes just as much of a PITA as using SP.

    And the SP route needs no data scrubbing code; that's completely managed by the driver (which, again likely knows more about the DB than you do). Here's an interesting fact, did you know MySQL supports \' as a quote escape? Many developers don't. Hello Mr. "\' or 1=1 --"!

    @UncleMidriff said:

    The place where I work *despises* inline SQL, so we do pretty much everything through stored procedures.

    I think inline vs sproc really is a system-by-system basis. For huge systems, I'd go 100% SP. It's more work but much more explicit. For smaller projects, I don't go that route because I just don't see it worth the overhead.

    @UncleMidriff said:


    CREATE PROCEDURE The_SP_What_Does_Stuff
    @CourseNumber
    AS
    select <some stuff> from S_C_S_L where C_S = @CourseNumber
    GO"

    Is this incredibly vulnerable to SQL injection attacks, and if so, how?

     

    Not vulnerable at all. @CourseNumber is never executed. It's a value stored in memory that's compared to the C_S column on each row of S_C_S_L.



  • So, to summarize in an effort to make sure I understand everything, using parameters allows the database driver of whatever database you happen to be using to validate inputs in order to make sure nothing fishy is going on.  You could try to do this on your own, but since the database driver likely knows more about the proper way to do this than you do, it'd be best just to let it take care of business.

    Looking at your examples from before,

    unsafeSql = "select * from tb where col='" & someVal & "'"
    cmd.Execute(unsafeSQL)

    safeSql = "select * from tb where col=?"
    cmd.Parameters.Add("col",someVal)
    cmd.Execute(safeSQL)

    let's say that someVal = "ValidColName or '' = '" .

    So now,

    unsafeSql = "select * from tb where col='ValidColName or '' = ''"

    and cmd.Execute(unsafeSql) will return every record from tb, which is undesired.

    However, in the safe example,

    safeSql = "select * from tb where col=?"
    cmd.Parameters.Add("col","ValidColName or '' = '")
    cmd.Execute(safeSQL)

    either cmd.Parameters.Add("col","ValidColName or '' = '") or cmd.Execute(safeSql) will raise the eyebrow of the database driver, which will then keep anything bad or undesireable from happening.  Am I right?  Or close, even?

    Thanks for your help...I owe you a pizza.



  • Yep, you're catching on just fine...



    To add to the comments above, sprocs are also desirable over inline sql
    for statements that get executed frequently (inside loops, due to high
    server load, etc), because most SQL DBs will precompile their stored
    procedures, so they don't have to determine an execution plan every
    time the code is called.



    Unless, of course, you choose to implement dynamic statements (via exec
    or whatever mechanism you choose), at which point the compiler is
    unable to generate and store an execution plan since the result of the
    "dynamic" part is obviously unpredictable.



    Our shop uses about 90% sprocs, and 10% inline.  0% dynamic.






  • <FONT style="BACKGROUND-COLOR: #9acd32" color=#ffffff>either cmd.Parameters.Add("col","ValidColName or '' = '") or cmd.Execute(safeSql) will raise the eyebrow of the database driver, which will then keep anything bad or undesireable from happening.  Am I right?  Or close, even?</FONT>

    In this case the ? syntax is throwing me off track a bit. Let me give you an example for Microsoft SQL:

    unsafe:
    unsafeSQL = "SELECT * FROM tbdSecret WHERE fldPassword='" + password + "'"

    In this example, if someone entered their password as:
    ' OR 'a'='a
    and the unsafeSQL is executed, the full statement would return ALL rows from the table, as it would read:
    SELECT * FROM tbdSecret WHERE fldPassword='' OR 'a'='a'

    'a'='a' is always true, so all rows will be returned.

     

    safe:
    safeSQL = "SELECT * FROM tbdSecret WHERE fldPassword=@password"
    cmd.parameters.add("@password", "' OR 'a'='a")

    This would probably return no rows, as it returns any row where the column fldPassword contains the value ' OR 'a'='a.

    So using the parameter makes sure that the whole password is compared, and not just the bit up to the first single quote.

    Hope this helps.

    Drak



  • SQL injection attacks (and their prevention) interest me as I develop a lot of web apps...



    Unless you are outputting whatever came back from the table to the web
    page in your code (ie a table of results), there is little that can
    really be accomplished in a well-written app by returning all rows
    instead of one row.



    The scary thing is when the SQL injection performs an INSERT or UPDATE
    query, and modifies table data.  (Changing product prices, giving
    themselves undeserved access levels, etc).  This would require
    that they have some knowledge of the DB structure, or are really
    persistent / good guesssers.



    Using parameters is definately key.   Virtually every query
    in my shop doesn't select based on text values - it's nearly always an
    integer row ID field, which can be converted to an int in code before
    being passed as a parameter or used otherwise in queries...



    Certainly, in the case of auth tables, you will often be selecting
    records based on username, or username and password, and there of
    course is where one must be most careful!  I prefer to validate
    the password in code rather than in SQL.  I could be totally
    offbase, but I feel I have more control in the code to check it and
    less worry about clever injectors doing strange things. 




  • <FONT style="BACKGROUND-COLOR: #efefef">We always store passwords in an encoded format. So any injected SQL would be encoded too and become meaningless [:D]</FONT>

    <FONT style="BACKGROUND-COLOR: #efefef">Drak</FONT>



  • Good point.   I wish my company did.  
    Unfortunately, frequently employees need to login as external users,
    and need to find out their passwords in order to do so.



    Gotta love it!



  • ♿ (Parody)

    @UncleMidriff said:

    either cmd.Parameters.Add("col","ValidColName or '' = '") or cmd.Execute(safeSql) will raise the eyebrow of the database driver, which will then keep anything bad or undesireable from happening.  Am I right?  Or close, even?

    For the most part, yes. The driver also uses a different mechanism to execute your query than Search & Replace. A low-level driver would pass your parameter values byte by byte, and then use them internally to compare (i.e., vals are never executed as part of the statement). Higher level drivers (OLE/ODBC) will still utilize text commands, but you can be certain they are properly escaping them out as well. For example, the SQL OLE drivers will do something like this for commands:

    "select * from tb where col1=? and col2=?" {... inputting two params ... }
    becomes
    "EXEC sp_executesql N'select * from tb where col1= @p1 and col2= @p2', @p1=N'Some Val ...', @p2=8283

    @blue said:

    Unless you are outputting whatever came back from the table to the web page in your code (ie a table of results), there is little that can really be accomplished in a well-written app by returning all rows instead of one row.

    Unless of course you're looking for the record named " '; EXEC 'DROP DATABASE ' + @@CURRENT; --".



  • @Blue said:

    Good point.   I wish my company did.   Unfortunately, frequently employees need to login as external users, and need to find out their passwords in order to do so.

    Gotta love it!

    Sadly, we use a reversible encoding. So it's still possible to decode passwords and use them to log on. I'd rather use a hash or something similar so that there's no way to get back a password from the data in the database.

    Drak



  • RE: "Unless of course you're looking for the record named " '; EXEC 'DROP DATABASE ' + @@CURRENT; --".



    Fair enough, but that would only work on specific DBs.  Not all define @@CURRENT, or have the same meaning for it.





Log in to reply