Top 7 PHP Security Blunders

[ArticleBot]

This is an article discussion thread for discussing the SitePoint article, "Top 7 PHP Security Blunders"

[dscriptor]

yes, it's a good article! great idea too!
still many things i need to know when it comes to PHP security.. :'(

[spinmaster]

nice article! enjoyed reading it...

[AlexW]

Wow. Now that's what I call a comprehensive critique. Impressive, Shiflett.

[petersj]

An easy way to secure files from being directly accessed is to include the following code at the top of each file. This method has the advantage that does not rely upon the hosting environment to secure the files.

if ($_SERVER['REQUEST_URI'] == $_SERVER['PHP_SELF']) exit(); 

[stereofrog]

In addition to what's already said, "$_GET[month]" is an invalid syntax.

[cyberlot]

No mention by the article or anyone else about the location of there config file..

If at all possible ALWAYS put your config files outside of your web path. This reduces the risk of exposure due to things like a bad php upgrade.

[LinhGB]

Here we go again... addslashes()

SELECT * FROM users WHERE name='$username' AND pass='$password';

However, if the user who's logging in is devious, he may enter the following as his password:

' OR '1'='1

This results in the query being sent to the database as:

SELECT * FROM users WHERE name='known_user' AND pass='' OR '1'='1';

<snip>

if (magic_quotes_gpc()){
$username = $_GET["username"];
} else {
$username = addslashes($_GET["username"]);
}

Not everyone quotes integer. So here's a version that'll break your addslashes:

SELECT * FROM users WHERE name='$username' AND pass='$password';

$password = blah' OR 1=1 OR 'blah' = 'blah

It still amazes me how many people get SQL injection wrong.

Solution to this: firstly always have a list of accepted formats for your inputs, and reject whatever doesn't match one of those. Secondly, use a query builder or prepared query, like what PEAR-DB or ADODB support. 1=1 will not work if you use prepared queries.

Some more:

Configuring PHP For Security ->

open_basedir - should set this in your vhost:

<Directory /path/to/website>
     php_admin_value open_basedir "/path/to/website:/tmp"
</Directory>

Having modsecurity installed will also catch most of the stuff you mentioned in the article.

[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, ...

[Snortles]

Magic quotes will almost assuredly be removed from PHP6.

Here's the developers notes from a meeting about PHP6.

http://www.php.net/~derick/meeting-n...l#magic-quotes

Not every server will have magic quotes enabled anyways, so no matter what you have to check all the incoming data.

[wei]

Beware of XSS! Some examples of possible ways to inject javascript into your output.
http://ha.ckers.org/xss.html

So far, i have found http://pixel-apes.com/safehtml to be a good solution in escape the output. Other solution to reduce XSS in the output are most welcome.

Wei.

[Jaffa The Cake]

A good protection against session-jacking.

When a user logs in, set their ip to a session var using $_SERVER['REMOTE_ADDR']. On ever page, ensure this session var equals $_SERVER['REMOTE_ADDR'], if not, log them out.

[wogboy]

Argh. http://phpsec.org should definately be consulted. http://www.hardened-php.net/ is another good site. There are enough mediocre PHP developers out there, we don't need articles like this.

[shiflett]

Can we please get this article corrected? I tried to politely (and quickly) point out the errors I noticed at first glance, and others have done the same.

Spreading this type of misinformation from such a popular site is hurting the PHP community.

[Vennie]

Originally Posted by shiflett

Can we please get this article corrected? ...
Spreading this type of misinformation from such a popular site is hurting the PHP community.

I agree with shifflett here. There are some serious mistakes in this article that should be corrected asap.

I know it's hard to cover a subject like this, but the amount of missers in this one make this article a 'blunder' in it's own right.

[Data Firm Inc]

Originally Posted by shiflett

Can we please get this article corrected? I tried to politely (and quickly) point out the errors I noticed at first glance, and others have done the same.

Spreading this type of misinformation from such a popular site is hurting the PHP community.

I agree as well... I think the article is a good start and has good direction for PHP security, but I think something like making sure the right terms are used is a big deal. I'm not a big fan of talking with someone who's new to PHP and using phrases like "SQL Insertion". I don't want to be anal about it but we should be holding ourselves to a higher standard especially on a well respected site such as SitePoint ("To whom much is given, much will be required"). Is it really that hard to make a few changes to the article?

Shiflett has helped out by clarifying some critical points (especially in the area of input and Magic Quotes). Thanks Shiflett.

Anyways... that's my two cents...

-Aaron

[Hammer65]

I agree, this article is downright dangerous. This makes our job in the forums, that much harder. addslashes and magic_quotes should NEVER be recommended. They should only be mentioned as being insecure. Any time I see this sort of code published anywhere, I don't hesitate to raise some hell about it. I hate to see this kind of misinformation associated with Sitepoint.

Originally Posted by Data Firm Inc

"SQL Insertion". I don't want to be anal about it but we should be holding ourselves to a higher standard especially on a well respected site such as SitePoint ("To whom much is given, much will be required"). Is it really that hard to make a few changes to the article?
-Aaron

I agree, although it's more commonly called SQL injection. Something about having the words Insertion and anal in the same paragraph makes me a little nervous.

[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...

[Icheb]

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

[shiflett]

Originally Posted by Icheb

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.

[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.

[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]

Originally Posted by 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.

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().

[cmuench]

That is a good solution but if someone has access to your .htaccess then your screwed. But if they have access to you .htaccess then they probably have access to your files to.