• Hans (unregistered)

    And for bonus point, when you pass null, it will try and execute it

  • Sauron (unregistered)

    strSQL = "UPDATE wtf_comment SET content = 'frist';"

  • Zatapatique (unregistered)
    strStrBuilder.Append(" update sometable set ") strStrBuilder.Append(" POSITION = 'FRIST', ")
  • (nodebb)

    There is no IDisposable handing here at all. Or in other words, whoever wrote this should be never be allowed to write any code other than Logo.

  • Long Time Lurker (unregistered) in reply to Hans

    Or maybe If strSQL.Equals(String.Empty) will throw a NullReferenceException?

  • Richard Brantley (unregistered)

    strStrBuilder is a name that just makes my brain hurt.

  • Jonathan (unregistered)

    Well, in Oracle Pro*C (SQL statements embedded in C code), a variable preceded by a colon is a "host variable", and is left as-is in the sql statement when it is added to the sql_context mega-structure, which is shipped off to the DB server, along with the value of that variable, stuck somewhere else in the sql_context mega-structure. Then the server uses that value when executing the sql.

    In other words, host variables are like bind variables, and if what we are seeing are host variables, there is no injection vulnerability. But I don't know VB.net, and maybe ":a" means "interpolate the value of the 'a' variable in this string". But that didn't show up in my brief search for "VB.net stringbuilder interpolate".

  • (nodebb)

    In VB.Net, you can put a calculation, even just a variable reference, in a string using string interpolation. Prefix the string literal with $ and then put { } around the calculation.

        strStrBuilder.Append($" SOMECOLUMN = '{p_somevalue}', ")

    You have to check if there are single-quotes in the p_somevalue variable or this gets really interesting in a later step.

    The version given will just generate SQL errors when you try to run it, unless that notation means something to a specific SQL engine.

  • (nodebb) in reply to MaxiTB

    To be fair, we don't know if "cmd.Dispose()" was in the snipped code. But it should have been declared in a Using block.

  • (nodebb) in reply to Jonathan

    and if what we are seeing are host variables, there is no injection vulnerability

    Except for the whole, you know, "pass literally any string you send it on to the database server part". :-D

  • (nodebb) in reply to Scott_Coleman

    Dispose without an exception scope is equally worse. So I'm pretty sure that that wasn't cut out, considering the the DbConnection is a member which is an anti-pattern, because it should be a short lived object managed by the connection pooling. So this code is big sign of "Fire me now!" long before you need to start reading the functional implementation :-)

  • (nodebb) in reply to MaxiTB

    So I'm pretty sure that that wasn't cut out, considering the the DbConnection is a member which is an anti-pattern, because it should be a short lived object managed by the connection pooling.

    We don't know how long it lives. If you're going to do a sequence of commands you might as well use one connection for the whole sequence and you have to if you're using transactions. If it's global you're right but this snippet doesn't prove it is, only that it lives at some higher level.

  • Griffyn (unregistered)

    while I'm never a huge fan of building SQL queries by appending strings together

    I embed SQL in my code, not because I want to, but because I don't know of a better way. Sure, I could pop them all into stored procedures on the SQL server, but then that becomes a dependency that I'd have to manage by writing code to create them, and then I'm right back to where I was.

    What's a better way than than embedding SQL into code?

  • Officer Johnny Holzkopf (unregistered)

    The true beauty of this Function is that it always Returns True. That, and strStrStrStrStr...

  • (nodebb) in reply to Griffyn

    What's a better way than than embedding SQL into code?

    I know only ways that are equally bad or worse.

    SQL is best kept as either code literals or in its own files entirely, or you can use stored procedures if the DB engine supports them. Or you can use an ORM, which has a whole raft of pluses and minuses.

  • (nodebb) in reply to LorenPechtel

    It actually doesn't matter, connections are pooled so it's important to dispose the it as soon as reasonable. In fact, not only the client connection pool is limited, on the server side connections are limited as well.

    Keep in mind, managed connections have nothing to do with unmanaged connections. In fact all ADO objects are just logical representations that sometimes don't even have a corresponding unmanaged object. An good example are DbTransaction which many providers incl. Oracle just realize by opening another unmanaged connection. Or DbCommand which don't really exist and hence you can have two commands running at the same time (especially because DbDataReader as well are sometimes linked in weird ways).

    Long story short, as always is it very important to properly dispose an IDisposable properly scoped as soon as possible (or use the unscoped using to let the compiler scope properly).

  • konnichimade (unregistered) in reply to dkf
    Comment held for moderation.
  • (nodebb) in reply to Griffyn
    What's a better way than than embedding SQL into code?
    In VB.Net, you can create a database QueryDef with parameters. Populate the parameters with values at run time and you are quite resistant to SQL injections. 
    Dim CustomerQuery as QueryDef
Dim CustomerRS as Recordset

    CustomerQuery=MyDB.CreateQueryDef("")  ' Anonymous querydef
CustomerQuery.SQL = _
"SELECT [Given Name], Surname, CustomerStatus.Description AS [Status Description] " & _
"FROM Customer INNER JOIN CustomerStatus ON Customer.[Status Code] = CustomerStatus.Code"
"WHERE Customer.ID = [Customer Code]"
    

    ...
    

    CustomerQuery.Parameters("Customer Code") = 273
CustomerRS = CustomerQuery.CreateRecordset()

    I left out a lot of details, but that should get you an idea of how to use QueryDef parameters. No matter what value is given to the parameter, it would be very difficult to get any code injection to be accepted. At worst your customer name becomes "; DROP Customer; --"

    Addendum 2023-02-15 10:00: Oops, looks like I did not define my Code block properly.

  • wolferitza (unregistered)
    Comment held for moderation.
  • Griffyn (unregistered) in reply to Nutster

    I always use query parameters. I typically use Delphi and ADO components. My understanding of the "issue" is embedding SQL statements as string literals, or building a SQL statement as a string within your program code. So, your code example would still qualify as an example of the "issue".

  • Craig (unregistered) in reply to Griffyn

    The issue is that strings from user-land should only ever go into parameters. The problem isn't with building a SQL statement in code in and of itself, it's with writing user-provided values into it e.g. "INSERT INTO sometable VALUES ('" & myValue & "')" which is vulnerable to a Bobby Tables attack.

  • Mohit (unregistered) in reply to Richard Brantley
    Comment held for moderation.
  • (nodebb) in reply to Griffyn

    I don't think there is anything particularly wrong with putting the SQL strings in your code. What is a problem is creating parameterised queries by building the query in this way as in this example from an ASP classic application I once had :

    sql = "insert into students (name) values ('" & studentName & "')"

    How we laughed when Little Bobby Tables Roberts (full name "Robert'); DROP TABLE students;-- Roberts" ) tried to register.

    (omitting link to XKCD to try to avoid the eternal moderation trap).

  • (nodebb) in reply to Richard Brantley

    It does sound like a name that could write C#++

  • (nodebb) in reply to Richard Brantley

    It does sound like a name that could write C#++

Leave a comment on “Injectables are Fun”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article