• Bob (unregistered)

    On the upside, you don't need to worry about what preventSQLInjection does, since the $query is passed by value, and the modified string is not returned.

  • (nodebb)

    And, of course, the database api mysql_query has been deprecated for almost seven years, and gone in the latest version for almost five.

  • Brian (unregistered)

    I was gonna say that calling toupper on a query string can be wrong in certain cases - if you're inserting case-sensitive data like a Base64 string, for example. But then I realized that, except for the last check, this code does nothing but burn cycles since it doesn't actually output the "sanitized" string.

  • P (unregistered)

    But why is the PHP code returning 500? I call it dangling plot threads.

  • (nodebb)

    I can easily see where forcing all characters would cause a problem, such as:

    SELECT [Given Name], [Surname]
FROM Customer
WHERE [Given Name] IN ("David", "Dave")
ORDER BY [Surname], [Given Name];

    No problem, until the case-sensitive comparisons in the WHERE clause. Instead of finding David or Dave, you are now looking for DAVID or DAVE, which is not quite the same. So that line is in fact wrong.

    If your function is always returning the same value, why bother have it return anything at all?

    Addendum 2020-04-06 09:06: Oops, forcing all characters TO UPPER CASE would ...

  • King (unregistered)

    I guess the function could be renamed to something like "dieIfUnionHasSelect", then this would not be a WTF.

  • Michal Molhanec (google) in reply to Bob

    You've missed the point. This is not sanitize_query(), this is die_if_there_is_union_outside_comments() So you have to care what it does as it might die()

  • Somebody Somewhere (unregistered)

    You're all missing the worst bit: they even misspelled "error".

  • Kleyguerth (github) in reply to Somebody Somewhere

    Or their logs are in a different language, one that spells "error" as "erro", like portuguese

  • sizer99 (google)

    My favorite phrase for it is 'This wasn't designed, it was accreted.'

    And I have code like that too - designed for some tiny purpose and written well for that (a clickable red/green light to show if a test station is being used). Then the user demanded feature creep begins - it now has chat, timestamped log entries saved to database, synced with the CalStand software, yadda yadda yadda. I'm not proud of how it is now - it's accreted - but it's not deemed important enough for a rewrite.

  • Thomas Harris (unregistered)

    Connect Prevent Injection Query Close Return Result

    Sounds like the WHO to me.

  • MiserableOldGit (unregistered) in reply to Thomas Harris

    You may think so, I can't explain

    https://www.youtube.com/watch?v=h3h--K5928M

  • (nodebb)

    SQL injection preventovano.

  • (nodebb) in reply to P

    One guess, following on from OllieJones (modulo the age of this story): the version of PHP on the new host is less than five years old.

    So +1 for turning off those error messages in production, -7 for not logging them and leaving Frankie with the need to go dumpster-diving.

  • (nodebb)

    "Which… again, sounds like they're worried about comments, but this is a greedy operation, so SELECT /* some fields / x, y, z FROM / some table */ my_table would become SELECT / / my_table."

    As a matter of fact, it wouldn't. The /U modifier on the end switches the greediness of quantifiers in the expression so that they're ungreedy by default.

    As for the other modifiers, /S is like Perl's "study" modifier that (a) takes longer to analyse the expression in hopes of finding a faster implementation, and (b) would be totally ineffective in this instance; /i makes matching case-insensitive — noteworthy, since the string has already been UPPERCASED and the pattern doesn't contain any case-sensitive characters anyway.

    Addendum 2020-04-06 17:13: So, yeah. Even disregarding the rest of the problems here, that use of "iSU" itself reeks of "I don't know, but it seems to work."

  • Aspie Architect (unregistered)

    I really hate things that try and get clever with SQL.

    I was inflicted with a DB deployment pipeline that had a white list of allowed SQL as understood by the developer. It understood basic CRUD but the other 99% of the language gave it an infarc. I've never seen a DBA so happy when I deprecated the wretched thing. It meant that the DBAs could spend time DBAing and not manually running scripts unsupported by the pipeliine.

  • estou erro (unregistered) in reply to Kleyguerth 
    die("erro");

    It looks like somebody started to type an error message, got distracted, and saved the file anyway.

  • 🤷 (unregistered)

    I've seen my fair share of tries to "prevent against SQL injections", but non as horribly as this. And on the upside, when I told the person responsible for this to just use parameters they listened to reason (after I showed them there was still a vulnerability in the code) and changed it. So, I guess not all hope is lost...

  • (nodebb) in reply to sizer99

    My favorite phrase for it is 'This wasn't designed, it was accreted.'

    Or, in some cases, excreted.

  • (nodebb)

    First, we make the string upper case. Of course, regexes can be case insensitive, so we don't need to do that. It's unnecessary, but I suppose not wrong.

    YES, BECAUSE THERE'S NEVER A CASE WHEN YOU WOULD WANT TO PRESERVE THE CASE OF AN SQL STATEMENT.

Leave a comment on “Now I Need an Injection”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article