• P (unregistered)

    Not TRWTF enough. Where's my_do_query and my_is_numeric?

  • LCrawford (unregistered)

    "high risk change to a stable system"

    Nancy works for Equifax, which values stability over security.

  • Ejfsk (unregistered)

    I would just quit that job.

  • my name is missing (unregistered)

    While the code is awful, the last paragraph is the killer. I hope this isn't a bank.

  • (nodebb)

    It's always amusing when the business has no idea what an actual risk is in the system, and refuses to listen to IT when they say it is. Yet if it was someone outside the company, say an auditor, who saw the vulnerability and said something, then suddenly they listen. Never understood that mindset.

  • David Mårtensson (unregistered) in reply to DocMonster

    I guess that they have a formal procedure for any suggested code change to evaluate how much of a risk of causing problems and this is very likely to be able to cause problems, especially if users of the function already found out about its deficiencies and applied correct escaping before calling.

    To then add more escaping you might break things

    So its stated correctly as a high risk change.

    Evaluating existing code is another thing altogether, what value can come out of that ? :P

  • P (unregistered) in reply to DocMonster

    Nah, the play nowadays is to report your vulnerability on Twitter to make as big a splash as possible, so the management and executive take it as a PR problem and demand a fix ASAP.

  • Pabz (unregistered) in reply to DocMonster

    Indeed it is amusing, but it's quite easy for the PHBs to blame the developers/IT when the database suddenly disappears, even if they have been warned about the risk multiple times....

  • Frank (unregistered) in reply to DocMonster

    It's easier to explain it than you'd think. Say you found a sparkly rock in the middle of the road, and you get it for free, how much do you think it might be worth? Say you enter a shop and buy a diamond for 5000$, how much do you think it might be worth? The diamond is actually worth less than you paid for, since companies withhold a huge stock of them just to make the price skyrockets, if you try to SELL that diamond, you're going to be surprised about its actual worth. Thus people give it value BECAUSE they paid for it: both for signaling status and for rationalizing their purchase. It's what you paid that made it valuable, the actual rock could be the same.

    "But Frank" you might say "this is a technical decision, it doesn't follow economic principles!" It shouldn't. But then again, economy is all they know. And Free stuff is worthless or there to push you into spending, as we all know.

  • Decius (unregistered)

    If you like PiU+00F1a Colada stdout(my_escape)

  • Jaloopa (unregistered)

    I don't see the problem. It doubles up any single quotes, which stops the robert'; DROP TABLE STUDENTS;-- exploit. What else do you need?

  • NickJ (unregistered)

    TRWTF is using PHP for anything.

  • Kleyguerth (github) in reply to Jaloopa

    At the risk of being whooshed... there is so much that can be done event with more filtered stuff: https://www.exploit-db.com/papers/17934

  • Kleyguerth (github) in reply to DocMonster

    I've made two managers, in two different companies, see the problem and demand an urgent change by showing how easy it was to log in to one of their own accounts. However, the same people who implemented the first broken version were assigned to fixing it. You can imagine how bad the "fix" was. It was enough to stop me (at one company an intern still in uni; at the other company a junior dev fresh from uni...) from being able to show how to exploit it, so in their heads it was 100% fixed.

  • eric bloedow (unregistered) in reply to DocMonster

    it's the "an expert is someone from out of town with a briefcase" mentality. it NEVER made sense.

  • (nodebb)

    I want my_redemption to be called after my_escape.

  • Wizofaus (unregistered) in reply to Kleyguerth

    I actually don't see any examples in there of how you'd perform injection if the only query is in the form of the one presented here, and all single quotes are doubled up by an escape function. Apparently with some databases with unicode character mapping it is possible though, but are PHP strings unicode? Would definitely be interested in seeing an example of a value that this code (terrible as it is) wouldn't handle.

  • Someone who's been audited (unregistered) in reply to DocMonster

    It's possible the auditor is there as either a legal obligation, or required by a client. In either case, ignoring an audit could directly lead to lawsuits and/or losing customers, and the pressure from the audit is coming from somewhere outside the manager's influence. Conversely, complaints from the IT department won't directly lead to lawsuits or losing customers, and the manager probably feels he/she has enough power to simply ignore them.

  • sizer99 (google) in reply to Wizofaus

    Oh there are still tons of exploits you can do this way, even when they try to just double all the single quotes. For example, what about all the numeric queries?

    | SELECT * FROM USERS where id=blah

    If they're dumb enough to be just pasting the blah in here, then they're often dumb enough to just be reading the blah from the form or url, and you can replace it with "3 OR 1=1" to get all info for all the users:

    | SELECT * FROM USERS WHERE id=3 OR 1=1

    Now you need another vuln here which is that it actually shows them to you, but it might use common code to view a single item or multiple items.

    There's another giant hole too. Lots of SQL servers let you escape single quotes, like with a backslash.

    So now you have:

    | SELECT * FROM users WHERE name='foo;

    Just escape foo and you have

    | SELECT * FROM users WHERE name='''; DROP TABLE users; --'; -- bobby tables in the house

    There's lots more you can do, but that should be enough to demonstrate. It's guaranteed that anyone who rolls their own quoting will get it wrong. Just use bound parameters. Or if you MUST, then call the DB's own quoting function, like mysql_real_escape_string()

  • sizer99 (google) in reply to my name is missing

    Odds are it is a bank, a health company, or anything where security is most important. Because they always have the worst security.

  • ooOOooGa (unregistered) in reply to sizer99

    Hmm... Banks and other financial institutions don't actually have security as their most important metric. We could argue that it should be, but it isn't.

    Most important to a system that handles people's money is that it works the same known-right way that it worked previously. Making sure that the processing that didn't get them in financial trouble last week also won't get them in financial trouble this week or next week.

    Like it or not - agree with it or not - but that is a huge hurdle to overcome to get something else instilled as the highest priority of the code development managers.

  • löchlein deluxe (unregistered)

    This is how my_real_escape() is born. And the configuration switch that turns, or not, my_escape into my_real_escape.

  • Andreas Ringlstetter (github) in reply to Wizofaus

    Trivial. \' OR TRUE;# which then unfolds to bla = '\'' OR TRUE;#'.

  • Jinks (unregistered)

    I'm just amazed that PHP has no function for this. It has the double_kitchen_sink() function, why not sql escaping?

  • Wizofaus (unregistered) in reply to sizer99

    But that's changing the query. I specifically asked if there were an example value for THAT query. Nobody's arguing it's an adequate general purpose SQL escape function (and given all modern day SQL databases support parameters there's not even a need for such a thing).

  • Junkfoodjunkie (unregistered)

    Holy shit. Dunno how old this code is, but anyone not using PDO or mysqli_ when doing php (use PDO) should be taken behind the shed and given a summary ball-snipping. With rusty, dull scissors.

  • Tgape (unregistered) in reply to Frank

    But Frank, they only paid the consultant $1k (two hours at $500/hr).

    They paid me many times that over the course of the year. My hourly rate is a lot less, but it adds up over time.

    It's just that they were already paying me, so they disregard that.

    Someone who's been audited, that works for external audits. To a lesser extent, it works for internal audits, as those are primarily to find things before external audits show them. It doesn't explain why they care more about what consultants say.

  • Rich (unregistered) in reply to sizer99

    I agree that the code is truly awful, but unless you assume that either there's an almost identical function for numbers which doesn't have the quotes (which isn't mentioned here), or that you're using an RDBMS which lets you escape single-quotes with something other than another single-quote (MSSQL uses '' to escape ', so that won't work), I still don't see a way of attacking it. There are a lot of comments held for moderation though, so maybe someone else here has already thought of one.

  • Eric Fast (unregistered) in reply to DocMonster

    It's the same mindset that leads to "we don't have time to fix bugs that are reported internally because we have too many reported by customers."

  • Erin (unregistered) in reply to DocMonster

    That can be flipped on you: You're only looking at the direct security risks. But there are plenty of other risks around as well:

    • A bug that crashes the application that somehow makes it through QA (especially given the quality of QA we can presume given the example.)
    • Or worse, a bug that doesn't crash the application but corrupts data for months or years before its discovered, especially if that's customer data.
    • And if that data is used by any external systems, they might already be using / working around the current setup, meaning you have to fix all of that other software (which is usually not the biggest problem -- that would be even figuring out what software that is.)
    • And double down on that if its something exposed to users as an external API (even if its a few steps down the chain.) Now they all potentially have to fix their code in addition to you possibly having to tell them that their data has been insecure in your hands for however many years and taking the PR hit.

    I'm obviously not arguing that security holes shouldn't be fixed, but they're not the only risk and depending where they are in a system, fixing them may well itself be a huge business risk, especially if they're buried deep in the system where anything less than an extremely targeted hack would be extremely unlikely to ever be a problem. My own company has a few holes where the only real attack vector is "an ops engineer goes rogue." Its technically a vulnerability, but not the most concerning one in the world.

    (Of course in the story's example, it sounds like its a systemic problem which likely means a host of easily-exploitable APIs exposed to the whole internet, and that's almost certainly a wide enough attack vector to override all of the risks of fixing it.)

  • Rich (unregistered) in reply to Rich

    If the dB is MySQL, and NO_BACKSLASH_ESCAPES isn't enabled, it's vulnerable to injection. Otherwise, it's just crap but maybe not dangerous.. depends on the RDBMS, but it's not mentioned here.

Leave a comment on “A Botched Escape”

Log In or post as a guest

Replying to comment #:

« Return to Article