Top 7 PHP Security Blunders

[cyberscribe]

Just a quick note to say that rather than reproducing the functionality of magic_quotes_gpc, it is a better practice to turn magic_quotes_gpc off altogether, and use database-specific escaping functions (like mysql_real_escape_string) which will cover a much wider range of special characters.

[Matt]

Or switch to ASP.Net. The security features are much stronger and SQL injection is nearly impossible unless you do something really stupid.

[ajking]

@ Matt: Sure, switch to .Net, unless you would rather develop inexpensive, rapid, versatile web apps served by *nix and Apache as about 60 per cent of the websites on the Internet do. And it might mean you become a better coder in the end, teaching yourself to observe the good programming habits with a very useful language listed in this very good article.

[TxFrog1999]

Just a quick note about invalid input from a form, while its true you should never rely on a pure JavaScript solution to validating form input it is best to use JS and even a simple AJAX solution to do a pre-server validation either before or while the form is being filled out. By using JS before running the form through the server it should cut down on the amount of hits on the server-based script as the immediate feedback JS provides will allow most users to fix errors in the form before submission.

[PC_Freak]

To my opinion using open_basedir has less use because it only applies to PHP and not to any other language you may use: Perl, Ruby, Python, ...

[Icheb]

And it's "no one", not "no-one" ;) (front page).

[colder]

1) You have to quote your array indexes. You've luck that php assumes undefined constants as strings. Enabling notices' reporting would have displayed that.

2) Disabling register_globals at runtime using ini_set() is useless, variables will already be defined. Consider http://php.net/configuration.changes as a better alternative for shared hosts.

3) For SQL injections : if the input is supposed to be numeric, check it using is_numeric() OR cast it to a numeric value using sprintf()/intval(). If it's supposed to be a string, use mysql_real_escape_string() and quote it. There is absolutely no need to currupt the input deleting some keyword/chars.

[David]

Good, clear presentation of the key issues.

[colder]

1) disabling regsiter_globals at runtime is useless: variables will already be defined

2) quote your array indexes. (page1)

3) mysql_real_escape_string() > addslashes() and the correct method is to apply stripslashes when get_magic_quotes_gpc returns true, and always use mysql_real_escape_string() after it. You can also cast values that are supposed to be integers, using intval() for example.

[Icheb]

Good to see that you still didn't correct that typo. At least now i have a good laugh every time I open the front page .

[icdsoftware]

A good article overall but I think some of the methods you described arent really the best (or easiest) way to go around solving the problems.

The thing that really jumped out to me was "enable magic quotes gpc" to solve this problem... That is one of the worst solutions to the problem!

IMO, the best method is to check for magic quotes gpc with

if( get_magic_quotes_gpc() == 1 ){ ... } 

And if so use a recursive stripslashes on your $_POST, $_GET and $_COOKIE arrays, I usually use something like this:

if( get_magic_quotes_gpc() == 1 ){

function cleanVar( &$var ){
    if( is_array( $var ) ){
        $var = array_map( "cleanVar", $var );
    } else {
        $var = stripslashes( $var );
    }
    return $var;
}

cleanVar($_POST);
cleanVar($_GET);
cleanVar($_COOKIE);

} 

and then from there, use the appropriate escaping functions such as mysql_real_escape_string() when using MySQL (this is important, addslashes may not be enough).

Safe mode is very annoying in many cases, and I'm glad to see that many servers are turning it off (and it (along with Register Globals and Magic Quotes) will very likley be removed in PHP6 (source: http://www.php.net/~derick/meeting-notes.html)). It would be good, but in many cases it just gives loads of problems when you want to write to a file. Only the most insecure servers really benefit from it.

Come to think of it, if you read the PHP6 notes you can see right away that relying on Magic Quotes is not a good idea, as its likley to be removed in the future.

EDIT: Didnt realise this linked to a bigger discussion, I'm sure much of what I have said has been said already.

[redbone]

Magic quotes are a pain (e.g. http://www.sitepoint.com/blogs/2005/...tes-headaches/) To stop SQL injection attacks use Pear::DB or AdoDB placeholders

[Anonymous]

$month = $_GET[month];

That raised a red flag right off for me. Always quote strings, such as array keys. You clearly don't have error_reporting set correctly (E_ALL or, for PHP5, E_ALL | E_STRICT). Combined with the suggestion about using magic_quotes_gpc and addslashes (as mentioned already, never use addslashes; always use the database-specific escape function), this article loses a bit of credibility.

[Data Firm Inc]

I think it would be best to let PHP speak on the issue. For me, the greatest resource ever for PHP is the PHP manual.

Read the security section of the PHP manual and get to know it well.

They do recommend the magic quotes is disabled in the reasons why not to have magic quotes enabled.

Compare
http://us3.php.net/manual/en/securit...quotes.why.php
to
http://us3.php.net/manual/en/securit...tes.whynot.php

Choose accordingly...

[dingocheck]

any more new PHP security tips? it's too old.

[Icheb]

Guys, stop trying to correct their articles. SitePoint clearly doesn't give a **** .

[confined]

For an Administration script I wrote, I retrieve the password FIRST and then compare the md5'ed USER INPUT to the stored md5 version, which I retrieved first.

User data never touches my database, unless I trust the user or I enforce data structure limitations/format.

[Tvienti]

When you're expecting an integer to be used in a SQL...

(SELECT * FROM `t` WHERE `id` = $UserInput)

... the malicious user could supply input such as "0; DELETE FROM `t`;" to attack you. I find a simple and effective solution is to cast user input to an int before using it in the query. Any string data will be chopped off, no more SQL injection.

[shiflett]

Guys, stop trying to correct their articles. SitePoint clearly doesn't give a **** .

Yes, and I find that very disappointing. This is exactly the type of article that we don't need.

It has been bookmarked by more than 500 people on del.icio.us:

http://del.icio.us/url/6a721e590a294c9278ff8c4396ad65f8

(This is almost exactly how many people have bookmarked the PHP Security Guide.)

SitePoint is ranked 681 on Alexa:

http://www.alexa.com/data/details/?url=sitepoint.com/

In other words, this is a very popular and very misinformative article. This is hurting the PHP community, and I am extremely disappointed that neither the author nor SitePoint seem to care.

If there was a more appropriate place for me to have directed my initial comments, please feel free to let me know. I'm very interested in getting this article corrected.

[Kevin Yank]

Actually, I've just updated the article to correct most of the critiques that were raised against it in this thread. If anyone is motivated to do so, please let me know if there are any significant issues I have missed.

[shiflett]

Actually, I've just updated the article to correct most of the critiques that were raised against it in this thread. If anyone is motivated to do so, please let me know if there are any significant issues I have missed.

Thanks for getting this corrected. The article is now vastly improved.

I'll restate the items from my original (likely incomplete) list that have not been completely addressed. Hopefully others will do the same, and I'll be happy to clarify anything that needs it.

User-provided data simply cannot be trusted.

This is not an error, but it can be misleading. I think a simple reference to the fact that no input can be trusted would be sufficient, and user input can be emphasized.

You should check the user's access privileges upon every load of a restricted page of your PHP application. If you check the user's credentials on the index page only, a malicious user could directly enter a URL to a "deeper" page, which would bypass this credential checking process.

The "user's credentials" to "user's access privileges" change is good, and it might be what the author meant to say. However, as this brief quote demonstrates, many other instances of "user's credentials" and "credential" need to be changed as well.

It's also advisable to layer your security, for example, by restricting user access on the basis of the user's IP address as well as their user name, if you have the luxury of writing an application for users that will have predictable or fixed IPs.

This qualification eliminates the error, but it's still misleading. Many people reading this article will be developing public web sites, and it might not be immediately obvious to them that their users don't fit this criteria (static IPs). Intranet web sites are about the only realistic candidates for IP consistency enforcement.

Always name these include files with a .php extension, so that even if all your protection is bypassed, the Web server will parse the PHP code, and will not display it to the user.

This has not been corrected. I'll restate my original comment.

Executing code out of context is an additional and unnecessary risk. I would argue that it's safer to instruct Apache to deny access to .inc resources (in httpd.conf) and store includes outside of document root. That's Defense in Depth. Naming includes .php eliminates the potential to add this extra safeguard, plus it just exchanges one risk for another.

To prevent this type of attack, you need to be careful about displaying user-submitted content verbatim on a Web page. The easiest way to protect against this is simply to escape the characters that make up HTML syntax (in particular, < and >) to HTML character entities (&lt; and &gt;), so that the submitted data is treated as plain text for display purposes. Just pass the data through PHP's htmlspecialchars function as you are producing the output.

This required a lot of rewriting, and you did a good job.

However, I think it's pretty important that we remind developers of the importance of character encoding consistency. I blogged about this recently, using Google's XSS vulnerability as an example:

http://shiflett.org/archive/178

This shows how htmlentities() doesn't offer sufficient XSS protection without indicating the character encoding.

SQL Insertion Vulnerabilities

The link at the bottom of the first page needs to be updated. All other references to SQL insertion appear to be corrected.

This feature safeguards against inexperienced developers who might otherwise leave security holes like the one described above, but it has an unfortunate impact on performance when input values do not need to be escaped for use in database queries.

I think a much bigger disadvantage (with particular relevancy to this article) is that magic_quotes_gpc and addslashes() offer insufficient protection against SQL injection. For example, they don't take character encoding into account.

Credit card numbers and customer information are the most common types of secured data, but if you transmit usernames and passwords over a regular HTTP connection, and those usernames and passwords allow access to sensitive material, you might as well transmit the sensitive material itself over an unencrypted connection.

As I mentioned before, this statement is valid, but shouldn't it be mentioned in context? For example, this is from the first page:

The www and admin directories are the only directories whose files can be accessed directly by a URL; the admin directory is protected by an .htaccess file that allows users entry only if they know a user name and password that's stored in the .htpasswd file in the root directory of the site.

One can reasonably assume that a "directory protected by a .htaccess file" is a reference to HTTP authentication, which sends a username and password in the clear. No mention is made of SSL, so the later comment appears to contradict the earlier recommendations. In addition, novice developers (those who need the most help) are less likely to make the connection.

If you're on a shared host, use the ini_set() function in all your pages and disable this setting.

This error still exists. Disabling register_globals with ini_set() only works on very old versions of PHP. I think this is just a case of not trying out something before writing about it. No big crime, but it should be corrected.

Thanks again for your efforts. It helps your users to know that their attempts to help out are worthwhile. :-)

[ermau]

I find it sad that even after it was 'updated', it still won't even run without produce notices. Not to mention the fact that the terribly slow regex is being used instead of is_numeric().

[pompopom]

Let's solve the security problem in two pages! YAY!

lolz, as if...

[Tino Didriksen]

Quote: "If you're on a shared host, use the ini_set() function in all your pages and disable this setting."

ini_set() happens a long time after register_globals has done its evil deed. The best way to turn off register_globals in a shared environment is to use "php_value register_globals 0" in your .htaccess.

[oops]

It will be very hard to find even sha-1 collision if you don't know the actual hash. And how and why should the user ever know that?