Are SQL Injection vulnerabilities in a PHP application acceptable if mod_security is enabled?

Question

I've been asked to audit a PHP application. No framework, no router, no model. Pure PHP. Few shared functions. HTML, CSS, and JS all mixed together. I've discovered numerous places where SQL injection would be easily possible. There are other problems with the application (XSS vulnerabilities, rampant inline CSS, code copy-pasted everywhere) but this is the biggest. Sometimes they escape inputs, not using a prepared query or even mysql_real_escape_string(), mind you, but using addslashes(). Often, though, their queries look exactly like this (pasted from their code but with columns and variable names changed):

$user = mysql_query("select * from profile where profile_id='".$_REQUEST["profile_id"]."'");

The developers in question claimed that they were unable to hack their application. I tried, and found mod_security to be enabled, resulting in HTTP 406 for some obvious SQL injection attacks. I believe there to be sophisticated workarounds for mod_security, but I don't have time to chase them down.

They claim that this is a "conceptual" matter and not a "practical" one since the application can't easily be hacked. Their internal auditor agreed that there were problems, but emphasized the conceptual nature of the issues.

They also use this conceptual/practical argument to defend against inline CSS and JS, absence of code organization, XSS vulnerabilities, and massive amounts of repetition.

My client (rightly so, perhaps) just wants this to go away so they can launch their product. The site works. You can log in, do what you need to do, and things are visibly functional, if slow. SQL Injection would indeed be hard to do, given mod_security. Further, their talk of "conceptual vs. practical" is rhetorically brilliant, considering that my client doesn't understand web application security. I worry that they've succeeded in making me sound like an angry puritan.

In many ways, this is a problem of politics, not technology, but I am at a loss. As a developer, I want to tell them to toss the whole project and start over with a new team, but I face a strong defense from the team that built it and a client who really needs to ship their product.

Is my position here too harsh? Even if they fix the SQL Injection and XSS problems can I ever endorse the release of an unmaintainable tangle of spaghetti code?

EDIT: As to my role on this project, I'm not a security expert, I'm a web developer generalist. the client initially asked me to load test the app and make recommendations for scalability. I haven't gotten that far yet, because when I first read the output of the home page, I saw inline CSS, table based layouts, and all sorts of crazy JS, so I requested a copy of the code, where I found the unholy mess described above. They don't even abstract the HTML header and footer. Every PHP file has a slightly different pasted version.

And as to the app itself, it does store personal information, and it processes (but does not store) credit card data.

Answer 1

Well....it depends. If your client is a bank - no, they can't go live. They should fire the team, and start again. If your client is a site dedicated to showing funny pictures of kittens, doesn't deal with money or personally identifiable information - then sure, go live.

The fact they asked you to do an audit in the first place suggests there is at least some level of risk.

The "conceptual versus practical" argument is masterstroke, but you can counter with "Security is a process, not a product". If you don't have a process for managing security (and being ignorant of XSS and SQL injection suggests you don't), it's only a question of time before you become a victim. Sure, mod_security protects you against known vulnerabilities - but a poor codebase is far more vulnerable to new exploits. If your client's site is attractive to attackers, poorly constructed code is far more likely to offer some kind of way in - and it may not be SQL injection or XSS; it could be information leaking (what happens when you force an error? do you see information you shouldn't?), it might be poorly handling parameters (what happens if I manipulate HTTP requests to push invalid parameters into the app?).

My recommendation would be to ask the client to agree what their security process should be, what standards they think they should adhere to - you could do worse than using Drupal's, and how they will measure their targets. I'd suggest that getting a clean bill of health from a static analysis tool and being able to switch off mod_security would be a good starting point.

In my experience, a poor codebase tends to degrade very quickly; I've seen a startup literally having to throw away 90% of their code less than 18 months after it was written because the overall quality was so poor.

Once you know what the security process is, you can agree a plan for adhering to it. I think you can then take a business decision on whether to launch the current iteration of the site. I'd suggest this business decision needs to take into account protecting the investment in the current codebase, as well as the security concerns.

Answer 2

Are SQL Injection vulnerabilities in a PHP application acceptable if mod_security is enabled?

No, at least not if you ask me.

If SQL injections are acceptable for the client, there is nothing you need to worry about. You could ask yourself why they did ask you to review the scripts anyway, but I won't bother, take the money and say goodbye. There is not much to argue, it's a pretty simple answer:

No.

If it's yes for them, they know what they do, not your business.

Answer 3

Is my position here too harsh? Even if they fix the SQL Injection and XSS problems can I ever endorse the release of an unmaintainable tangle of spaghetti code?

No, your position is not too harsh. You need to adopt a very simple approach: Fix the vulnerabilities, organize the code, and then it will get endorsed.

Anything less than this will ensure that the client has even larger issues down the road...as I discovered when I took over development lead for an open source HRM project. I have spent a year re-writing 95% of the code from scratch, because the original developers wouldn't take the time to follow proper coding standards such as logical folder structure, using classes and functions for repetition, universal headers, normalized databases, etc.

The long and short is, if it's going to be served to anyone via the web, it is no longer a "conceptual" problem, it is very much practical.

Answer 4

Your initial reaction is that of a developer purist. You see rubbish and you think it should be fixed.

Trouble with this approach is that commercially it is very unlikely to be acceptable to the client, and it is probably not commercially realistic.

You need to give the client an answer which addresses all the relevant points, as well as giving a plan for the future (give your client a solution, not a problem).

You might for example give a written report (and I strongly suggest you put this in writing - if you were engaged to do an evaluation, you should deliver something in writing).

The report might say something like (greatly expanded, of course):

• the product is poorly written (cite examples you have already listed);
• the product contains security vulnerabilities and even though you are not an expert in the field it is your view that these will be exploited at some time;
• the product will not scale well (this was your original brief) because of poor code structure, lack of consistency, use of numerous instances of cut-and-paste code which detracts from maintainability.

You can then go on to say:

• in a perfect world, unconstrained by commercial realities of release dates or budget, the product should be used as a prototype and development should be started again [Saying used as a prototype means you are not saying its garbage, you are saying it is a quickie that can be used to evaluate look / feel / style / workflow, etc... its politically softer.];
• because the product must be released, a critical step is to do a security vulnerability analysis - this is an urgent task. Any patch up must be applied quickly, even if this makes the code-base more complex;
• a secondary activity should begin to commence a replacement design that will scale and be more maintainable. This might use a framework, or it might take the existing code and improve it;
• if the outcome of the secondary task is to embark on process of refactoring and improvement, this is acceptable... and you might suggest places to start (for example, common menu system, common HTML header / footer code, moving CSS out of code, etc etc). Each of these items could be listed, and these could be work items to be budgeted and worked on as small projects as part of the refactor.

So what I suggest you present is a staged solution rather than throwing your hands up and saying "its a mess start again" which is emotional and not very constructive.

Having submitted your report you may or may not become part of the solution. You probably won't be very popular with the developers - the fact that they have done what they have done probably also means they won't understand most of what you submit. The danger is that the client won't understand it either. So there is a high likelihood that having submitted your report you will get paid and never hear from them again. So be it. The written report covers your arse [ass for Americans].

Answer 5

To look at it from a different angle, if the developers think that massive repetition is a "conceptual problem, not a practical one," then you have more serious concerns than security issues that--at this point at least--truly are theoretical.

A codebase that's full of needless repetition is going to be unmaintainable: when they try to change something, they'll have to change it in X different places. They'll generally find X-1 of them (maybe less, depending on the complexity) and this will introduce subtle bugs in the codebase. Fixing them will require more changes... and you can see where this is leading. On any product that needs to evolve, (which is basically any product,) this can be the kiss of death.

And the fact that they dismiss it as a mere theoretical concern shows that they don't understand that it's a real problem, which means that they won't know how to fix it once it starts causing trouble for them, or even recognizing that this is at the root of their problems. If I was auditing this project, I'd fail them on that basis alone, even if the security was airtight.