Top 7 PHP Security Blunders

[coffee_ninja]

OfficeOfTheLaw is exactly right: form mail spam is a big risk.

The way I solve this problem with my own sites is to generate the 'to' field completely server side. I see no reason why the 'to' field ever needs to include user input. Instead of having something like

<input type="hidden" name="recipient" value="me@mydomain.net"/>
...

<?php if(isset($_REQUEST['submit']))
{
    mail($_REQUEST['recipient'], ...);
}
?>

the recipient of the mail is configured locally, for instance,

<?php

if(isset($_REQUEST['submit']))
{
    $args = parse_ini_file('config/mail.ini');
    mail($args['to'], $_REQUEST['subject'], $_REQUEST['message'], $_REQUEST['from']);
}
?>

Any comments, good OR bad? I just can't think of too many situations where I would need to allow a user to send an email to an arbitrary address, unless of course I'm writing a webmail app.

[andre]

magic quotes? are you sure? personally i think it creates more problems than those it fixes.

[mjackson42]

Great article, but one problem: According to the PHP documentation, you can't change the value of register_globals from a script, since it would've already been applied by the time your script gets to that point. You can set it from Apache's config files, or from .htaccess, but NOT from your script.

[dpk]

With regards to the formmail comments:

Note that hiding the To variable on the server is definitely not sufficient. The attacker can just pack extra headers and all the content in the From header. You'll need to make sure that everything after any found \r or \n is chopped off to be more secure. Do the same for the Subject, too.

[artcoder]

Great article! A small typo in the example code. It should be ...

if ($_SESSION[sha1(password)] == sha1($userpass)) { 
 // do sensitive things here 
}

instead of ...

if ($_SESSION[sha1password] == sha1($userpass)) { 
 // do sensitive things here 
}

[heggaton]

OfficeOfTheLaw is exactly right: form mail spam is a big risk.

The way I solve this problem with my own sites is to generate the 'to' field completely server side. I see no reason why the 'to' field ever needs to include user input. Instead of having something like

...

Any comments, good OR bad? I just can't think of too many situations where I would need to allow a user to send an email to an arbitrary address, unless of course I'm writing a webmail app.

Actually, it's not just the 'to' header you need to be worried about. I too had a similar sense of security - until I discovered (the hard way) that the 'from' is also vunerable. In the instance:

mail($args['to'], $_REQUEST['subject'], $_REQUEST['message'], $_REQUEST['from']);

the last param could easily have additional headers injected (such as \r\nBCC:...) and whamo, someone's sending emails from your form to 200 random strangers!

[artcoder]

I don't trust myself to be able to write secure code. So I'm hoping to use some pre-packaged form script.

Is Matt Wright's FormMail CGI script found here

http://www.scriptarchive.com/formmail.html

consider secured?

[mjackson42]

I don't trust myself to be able to write secure code. So I'm hoping to use some pre-packaged form script.

Is Matt Wright's FormMail CGI script found here

http://www.scriptarchive.com/formmail.html

consider secured?

Yes, if by "secure" you mean "very liable to get your server hacked"

Seriously. Matt's FormMail script has a long history of being insecure. Even if it's been improved, I wouldn't trust it.

[OfficeOfTheLaw]

I don't trust myself to be able to write secure code. So I'm hoping to use some pre-packaged form script.

Is Matt Wright's FormMail CGI script found here

http://www.scriptarchive.com/formmail.html

consider secured?

Great joke.

[ega1]

mail($args['to'], $_REQUEST['subject'], $_REQUEST['message'], $_REQUEST['from']);

the last param could easily have additional headers injected (such as \r\nBCC:...) and whamo, someone's sending emails from your form to 200 random strangers!

In this example, the subject is vulnerable too. Any field that ends up being part of the header can be "injected."

It's very common to use people names as part of mail headers as well - check those too.

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

[Sentient]

Jaffa, IP addresses often change, if a user is behind a proxy, the IP they logged in with may not match the IP of the very next request.

[LinhGB]

In this example, the subject is vulnerable too. Any field that ends up being part of the header can be "injected."

It's very common to use people names as part of mail headers as well - check those too.

This form mail hijack is just a case of programmers not filtering user inputs before using them. I'd at least pregmatch the 'from' email and 'subject' and encode the 'content' with htmlentities.

[Packetloss]

Great article!

Another common mistake is sending sensitive form data through GET. That's fine if you are doing a search or something, but I've seen sites use GET to send usernames, passwords, etc.

[Mike]

Not really related to the security aspect of the article, but another common mistake in PHP.

Array indicies should be surrounded by quotes.

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

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]

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

[OfficeOfTheLaw]

This form mail hijack is just a case of programmers not filtering user inputs before using them. I'd at least pregmatch the 'from' email and 'subject' and encode the 'content' with htmlentities.

Of course, in addition to the rule that you should always check user input, this especially holds true to ANYTHING that goes into the headers of an email message.

[Data Firm Inc]

Shiflett just posted a link to a chapter from his security book.

This chapter discusses form processing and the most common types of attacks that you need to be aware of when dealing with data from forms and URLs. You will learn about attacks such as cross-site scripting (XSS) and cross-site request forgeries (CSRF), as well as how to spoof forms and raw HTTP requests manually. By the end of the chapter, you will not only see examples of these attacks, but also what practices you can employ to help prevent them.

Go to http://shiflett.org/ to find the link. He's also posted some recent XSS news there as well.

Hope it helps!

-Aaron

[www.corwin.ca]

Good article. One note on the "SQL Insertion" solution is that magic_quotes_gpc() is actually get_magic_quotes_gpc()...

[NicB]

The path of magic_quotes_gpc leads to madness, and it's not going to guarantee you won't end up with SQL injection vectors anyway.

Turn magic_quotes_gpc _off_, and use PEAR::DB to access your database instead, using placeholders. This will ensure that not only is the data escaped, but it's _escaped correctly for the particular database you're using_. It also makes it a lot easier to port to other databases.

On the XSS side of things, using a decent template engine makes this much easier to get right. Using a template engine also makes your code less likely to drive other programmers insane.

[longneck]

excellent article. this is eaxctly the article i will reccommend to my programmer friedns that want to learn PHP.

however, i have to strongly disagree with the suggestion of turning on magic quotes. i personally reccommend turning off magic quotes and escaping with the proper functions when necessary. the only thing that should ever be escaping data is the code that actually touches the database, i.e. the database abstraction class if one is used. otherwise it's too easy to run in to situations where input gets escaped too many times. magic quotes also lessens the likelyhood that an aspiring programmer will learn how to properly escape input early in their development.