PHP Worm workable fix?

Looking for an easy way to mend my sites.

[Sypher_5]

As a hobbist Webmaster I've been teaching myself PHP and have came back to looking for a fix to the php worm that attacks using your includes.

This is what I've currently doing,

ht tp://www.site.com/?p=includesname

$p = $incl

if (is_file("$incl")) {
include($incl);
}
else {
include('_pages/error.inc');
}

Would using this be better or even fix my problem with my includes being insecure?

$p = $_REQUEST["p"];
if (is_file("$p")) {
$incl = $p;
}
else {
$p = 'error.inc');
}

I dont really want to change all my pages with numeric names and check they're valid pages. I have a few pages that get varibles from the query strings & the checking could get somewhat complicated.

[mincklerstraat]

No, this probably wouldn't make such a big difference in the security of your code.

The problem is your only check of your incoming variable is if is_file($var); , whether you get this from $_REQUEST or not (meaning register_globals needs to be on).

I didn't even know about is_file so I looked it up, and it looks like it could be used as a sort of informal ad-hoc security check if you use php4, since the manual notes that only php5 supports url wrappers for it (meaning, in short, that hackers can use it to include off-site files - like nasty code on their own site, or even better, someone else's site they've compromised).

However, there isn't any mention of using this in conjunction with security, so a clever hacker could probably compromise it.

Have you been hit by a worm, btw?

Better would be to keep register globals off, and do something like:

$p = $_REQUEST['p']; 
if(!preg_match('#^[a-zA-Z0-9_-]*\.[a-z]*$#', $p)) die('invalid characters in parameter'); 

This stops script execution if anything is entered that isn't a string consisting of alphanumeric characters plus - and _, followed by a dot and then followed by a group of lowercase characters (for your .php and .inc). It will stop, for example, things like // or ../, which you don't want.

[Timotheos]

Hi Sypher_5,

Depending on your version of PHP and your setup this may not work. What I mean is that in later version is_file can accept url wrappers. Best thing to do is to match it off a list of known files (which could be automated).

For other tips see this link
[devx.com...]

Tim

[Sypher_5]

So basically just checking for slashes in the URL/include etc would do it?

[Sypher_5]

Also, because I add the .inc on in code* & check for the file's exsistence in the specified folder wouldn't that also stop it?

Because for the file to be used it'd have to be on the site & if you already have that much access this would be a waste of time.

*$incl = "$p.inc";

[mincklerstraat]

Checking for slashes and using is_file are going to help you, but as soon as you switch to php5, is_file won't tell you any more if your file is on your server - it'll just tell you that a file exists, and that could be a nasty file on somebody else's server.

It's better to 'whitelist' what you want than 'blacklist'. I.e., checking for slashes is good, but it's better to think, I only have letters, numbers, dashes and underlines in the filename, with one dot for the extension, followed by all lowercase letters - so when I include, let me check for that pattern. Don't think - 'throw it out if it's got this in it' - think : 'include it only if it's in the list of things that are ok'.

[Sypher_5]

Wouldn't adding .inc in code after you get the varible to what ever the varible is mess it up though?

nastyfile.vir would become nastyfile.vir.inc and is_file would say it doesn't exsist?

[mincklerstraat]

Adding .inc would help, but still isn't as good as whitelisting what you want included. As a hobbyist developer, just checking for slashes and adding .inc might be all you want to be bothered with for security, and this would indeed be better security than a lot of sites out there have. However, your scriptkiddie just has to rename the file to nastyvir.inc and your system includes it. What I like to do is have a naming convention for include files - 'all files that are going to get included start out with a few lowercase letters, followed by - ... ' etc., so any string can be checked with preg_match to see if it matches this pattern. This way you can be a lot more sure that the file you're including is actually one of your own files.

It's great you're just thinking about security to begin with, though - there are developers who don't consider themselves hobbyists who still don't get this far.

[Sypher_5]

Hmm, but wouldn't the renamed file become nastyvir.inc.inc and then of course not exsist?Because the code adds .inc again to the end of whatever is taken by the REQUEST/GET/etc

[jadebox]

Wouldn't adding .inc in code after you get the variable to what ever the variable is mess it up though?

Strip ".inc" from the filename first:

$file = str_replace(".inc", "", $file) . ".inc";

-- Roger

[Sypher_5]

No matter, I see how it could be done.

File named nasty.inc added like so &p=htt p://nasty.site.com/nasty

:s