12

I am in charge of maintaining and extending a PHP codebase which began in 2007 and uses the original mysql module. All user input is escaped using casting for values expected to be numerical, mysql_real_escape_string() quoted using single quotes for strings, possibly being further filtered through in_array() for ENUM fields or array_intersect() for SET fields. All unconstrained string fields are then passed through either htmlspecialchars() or htmlentities() when outputting HTML. Where a value represents a foreign key, that key is verified extant first.
I believe that by following these procedures rigorously, the app is as safe as it can be against injection and other forms of attack. (bonus points: am I correct? If not, what am I missing?)

Converting this app to mysqli or PDOs would be a fairly large task (and, to avoid accidental breakage, not something I would want to automate). So finally to my question: Are there any specific vulnerabilities that cannot be mitigated when using the old mysql module, which require migration to the newer modules?

Bounty Info:
To be clear, I am hoping for a list of CVE numbers or a statement from the PHP developers that the mysql module is patched against all known vulnerabilities as of such-and-such a date. I am also assuming that following Best Current Practices in using the module does not expose me to additional attack vectors. BCPs already include escaping data taken from the db before inserting it into a new statement. Going on and on about that isn't really addressing the question.

Nicholas Shanks
  • 10,623
  • 4
  • 56
  • 80
  • 3
    If you look at the [`mysql_real_escape_string()` page](http://php.net/mysql_real_escape_string), there is a yellow banner saying you need to set the charset (either directly on the server or by using [`mysql_set_charset()`](http://www.php.net/manual/en/function.mysql-set-charset.php)) for mysql_real_escape_string to take effect. – MarcDefiant Mar 18 '13 at 09:21
  • 1
    I'd suggest to split this question into 2 separate ones. Deprecated mysql module has absolutely nothing to do with XSS (as well as XSRF and dozens other) attacks. – Your Common Sense Mar 18 '13 at 09:36
  • @Mogria Please add your comment as a full answer so I can up-vote it :) – Nicholas Shanks Mar 22 '13 at 12:58
  • I was holding myself from this comment desperately, but now I can't help it :) May be I am too greedy a person, but 50 reputation points is too cheap, in my point of view, for the "complete list of CVE numbers". To make it clear, I wouldn't be able/do it even for 500 though. – Your Common Sense Aug 05 '13 at 07:40
  • @YourCommonSense That's fine. I was not expecting you to provide the answer I would like, the bounty is to bring this question to the attention of folks who have not yet seen it. Hopefully one of them will either know of a URL providing this info or have superior Googling abilities to me :) Also I am trying to get to 2k rep so that I can edit posts without moderation, and this was my first ever bounty offer, so I have started small! You don't need the rep anyway ;-) When I get to 62½k I would certainly offer more on bounties. – Nicholas Shanks Aug 05 '13 at 07:51
  • 1
    Nicholas, please check if the mysql extension you use is using internally the mysqlnd driver which is under active development. I would go with that "internal upgrade" and keep an eye on the progress of that mysqlnd development as it's one of PHPs key feature with Mysql. Next to that you haven't shown any code nor configuration, so it's not clear which kinds of your application and database server interaction is potentially more exploitable and by which CVE report. http://dev.mysql.com/downloads/connector/php-mysqlnd/ – hakre Aug 06 '13 at 07:35
  • @hakre Fair point regarding code and config. I was trying to solicit general responses rather than post anything specific to my own configuration as we are currently running on an older PHP and am intending to update it to the latest version sometime soon. I did not know about the libmysql/mysqlnd split, so that is certainly helpful. I will make sure I compile in the newer one. – Nicholas Shanks Aug 06 '13 at 10:34

4 Answers4

7

I have but 2 objections

  • All user input is escaped is a critical fault, leading to second order injection. "All dynamical data for SQL" is the right approach and phrasing
  • there are no identifiers mentioned in your post but I can't believe you don't have a query with dynamical identifier in your code since 2007.

There is also a minor inconvenience: in a couple of years (3-4 probably), your PHP will start issuing E_DEPRECATED level errors. But they can be simply turned off.

Anyway, just a mechanical move from one API to another won't make too much sense.
Refactor your SQL handling code only to make use of some abstraction mechanism, be it ORM, AR, QueryBuilder or whatever else technology which will wipe raw API calls from the application code. It will not only make your code less bloated, but also will make it independent from whatever else whim that will strike PHP developers in the future.

To answer the edited question.

There are no essential vulnerabilities in the old mysql ext. The only way it is commonly used is vulnerable and error-prone.
So, instead of looking for strains on the module, better audit your code. If it is not using a centralized library for database interaction utilizing prepared statements, most likely it is vulnerable.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • Thanks for your response. Could you expand those bullet points a bit? By "user input" I was meaning "stuff that isn't hard-coded". The only source for such data I can think of is the $_POST variable (or php://input stream for non-POST, non-safe [in the HTTP sense] methods). Please provide examples of 'second-order' sources. Also, I am not sure what you mean by 'identifiers' and 'dynamical identifiers' (are they the same thing?). Are you referring to primary keys? – Nicholas Shanks Mar 18 '13 at 09:35
  • I think by 'second order injection', he means SQL-Injection by data which has already been stored on the server. For Example: Filtering a text for an username from your DB. And the username happens to contain a `'` and some nasty code. – MarcDefiant Mar 18 '13 at 09:50
  • I bet you if you can provide one example for Blind or Second Order Injection in case that the application uses mysql_real_escape_string()+Correct Charset and nothing more. –  Aug 06 '13 at 07:35
2

My answer will be somewhat off, and instead of answering your specific questions, I would rather suggest the approach that will actually help you out.

No matter what vulnerabilities could be left or could arise in the future using mysql, and no matter how solid your current codebase approaches (rather avoids for) SQLinjection (it seems to do so pretty well, though), I get the feeling you still would rather migrate to mysqli anyhow but aim for delaying in doing so by seeing short term possibilities to further hack your way around the unsafe and to-be-fully-deprecated mysql.

I would suggest refactoring. Period. Just do that. Keep in mind to refactor, and not to change or extend your codebase while doing so - it's a nasty pitfall. Eventhough it'll be some work - the refactoring, just start your refactoring process (branched of course). It will be very satisfactory on completing it. Expect some long tail issues.

I assume every functionality you describe is wrapped already, so refactoring should be fairly doable. If things had not been wrapped.. ($#@!), figure out a way to uniquely track your function calls (including context) project-wide, (possibly using regular expressions in finding them all) and replace with new unique to-be-used wrapper functions. First explore this, thoroughly. In half a day you would be able to enlist all your regular expression that you will need. So plan it first, by exploring your path first.

Fill the new wrappers with the current (old) functional code and see if all still works as was.

Then start migrating to mysqli, and rebuild your wrappers internally.

Seems to be a simple as possible approach, avoiding all the matters and questions that will stay in the back of your mind despite whatever you try do in hacking your way deeper around common mysql. I don't need to tell you about the benefits mysqli will bring, you know those already. Futhermore, it simply is good practice to every once in while actually undertake those kind of issues for once and for all. Plan, discuss, branch, try, do, test, complete, and conquer! On top off all: make sure you don't fall for the opportunities to extend your codebase and functional scope while refactoring - you will be tempted to do so: just refactor. Add, extend, or improve later.

LenArt
  • 333
  • 3
  • 6
  • Nobody's answer came close to what I was looking for, but this one is both aligned with where I am going and also the author had the lowest score so could do with the bounty points. :-) – Nicholas Shanks Aug 07 '13 at 19:35
1

I don't feel qualified to answer your question definitively so please be careful using the information I provide, but I do have some experience in managing the sort of changes you might be looking at. AFAICS however your statements should protect you against XSS and SQL injection if what you say is true.

I have recently begun the process of migrating an entire large application from mysql_ to mysqli_ and in doing so I have also set the goal that all user input shall go through prepared statements.

EDIT in response to YCs fair comment below: For the avoidance of ambiguity, I always put anything that a user has generated through a prepared statement, even if it has already been stored in the db. Where possible I avoid using re-insertion of user data though as it is unreliable so system functions tend to depend on automatically generated indices.

In fairness, the pages are short (no more than 1000 lines) so fixing does not take long per page and they have few queries so the performance reduction is not noticeable, and certainly eaten up by improvements in server technology since I wrote the original code. I suspect you will find that the reduction in escaping etc will far outweigh any performance hit on prep statements though you'll have to check if it's critical.

It has dismayed me how many vulnerabilities I found in my code whilst doing this review (I included security as best I could when it was written and set myself rules much the same as yours) and ultimately I have found the need to rewrite large code chunks to improve security. Performance has improved markedly as a result of greater experience and code tweaks too.

The way I'm going about the change is to add a mysqli connection to my database header file. All new code uses this. As I find time, I'm updating older code and testing each page using a header file that does not have the old mysql connection. It's dead quick to find bits you've missed that way in a dev environment and it can be quite a good way to use time that would otherwise be wasted as each page takes only a few minutes to update so can be done during brain-fade periods.

A note on the second order injection BTW as this was the most common vulnerability I had built in:

Most SQL injection prevention kind of assumes that the injection attack will only take place at login, from a malicious non-registered user who once foiled will never return and that registered users can be trusted. Not so.

It is conceivable that code could be injected through your protection then used later. This is most unlikely to work as it is heavily dependent on clumsy db and application design, but some of the smartest people in the world are hackers. Why make their life easier?

The attack is made more probable if your sql is simple and your application does any subquerying using data previously obtained from the db. Remember that

' OR 1=1 gets converted to
\' OR 1=1 by mysql_real_escape_string but is stored as 
' OR 1=1 in the db 

so if retrieved and placed into a PHP variable which is then used unescaped in a sql query, it can then cause problems just the same as if it had never been escaped. If you just use prep statements for all queries, the risk goes away permanently though do remember the prep statement will still store the malicious code so you still have to use a prepared statement again when you next need to use the data that has been entered.

This Blog gives a decent discussion and examples so I'll not expand further, but if you ensure that all user input data is passed through prepared statements if it is to be used as part of a query, even after it has been stored in the db, you should be safe.

At the risk of repetition, it is worth also becoming very friendly with the OWASP site which has valuable security discussions.

Robert Seddon-Smith
  • 987
  • 1
  • 9
  • 13
  • -1 for contradicting "**user input** shall go through prepared statements" vs. "second order injection" and mentioning OWASP site which actually advocate unsafe solutions. – Your Common Sense Aug 04 '13 at 11:56
  • IMHO not contradictory but I deserve the -1 for being unclear. Correction made and your comments always appreciated! Which security site would you advise? – Robert Seddon-Smith Aug 04 '13 at 20:03
  • So, you're dividing data to "user-supplied" and "non-user supplied" and always keep in mind data source, and treat it differently. – Your Common Sense Aug 04 '13 at 20:05
  • That is the idea. Some data is inherently safe, being generated internally. Wherever possible I use a reference to the user data, usually a SERIAL rather than the data itself, The user data is displayed (through htmlspecialchars for XSS) but not re-integrated in the database save through prepared statements. The db is fully normalized so it is most unusual to have to reintroduce user-entered data as it tends to exist in only one place. I am aware of the risk though of making a mistake so tend to be very conservative and if in doubt use prep stmnt. – Robert Seddon-Smith Aug 05 '13 at 07:26
  • And as soon as it grows to a more-one-man project and another developer has no idea of your separation - it become a scenario of an injection, as it was 1000 times already. Very clever. – Your Common Sense Aug 05 '13 at 07:29
  • @YourCommonSense, can you supply details for things OWASP does incorrectly? – Owen Beresford Aug 05 '13 at 18:58
  • @OwenBeresford [SQL Injection Defense Option 3: Escaping All User Supplied Input](https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet#Defense_Option_3%3a_Escaping_All_User_Supplied_Input) – Your Common Sense Aug 05 '13 at 19:05
1

Okay so i've made a little research and the only vulnerability I’ve found in regard to mysql extension also seems to affect mysqli equally extension is CVE-2007-4889 which is a "Safe Mode Bypass" vulnerability and has been fixed long time ago whats more is that both mysql.so and mysqli.so modules are sharing pretty much the same imports as can be seen -

/usr/lib/php5/20090626/mysql.so:

 0x0000000000000001 (NEEDED)             Shared library: [libmysqlclient.so.18]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000e (SONAME)             Library soname: [mysqli.so]

/usr/lib/php5/20090626/mysqli.so:

 0x0000000000000001 (NEEDED)             Shared library: [libmysqlclient.so.18]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000e (SONAME)             Library soname: [mysql.so]

There is also some sort of likelihood where new vulnerabilities may raise in both modules because of their shared import nature And there is always a chance where a distinctive flaw is originated from the actual code of one of these modules. But so far it seems like the vulnerability record of both modules is pretty much the same to this date

As mentioned here I’d invest more time in auditing the PHP source code making sure few things -

  1. SQL injections - Parametrized SQL queries is the best solution against this kind of flaws.

  2. XSS(Cross Site Scripting) - use htmlspecialchars() to filter dangerous characters

  3. CSRF(Cross Site Request Forgery) - Always perform referrer check on forms to be sure the data arrived from the right place

  4. File Inclusion - make sure no user input can directly affect inclusion(require(), require_once() , include(), include_once()) of files

  5. File Upload - in case where file upload is enabled for some reason make sure not to let the user control the file extension or save the files and set its presmission to be "non-execute". this is done to prevent from at attacker to upload malicious file and execute code on your server.

Xeus
  • 280
  • 1
  • 2
  • 14