5
\$\begingroup\$

My site was recently infected and running malicious scripts. Once I discovered this, I cleared everything, the hosting company re-initialized the VPS and I setup my site anew. This is how I run my PHP scripts currently. I have two objectives:

  1. To keep my scripts from being directly accessible from the web
  2. To never run a script that I do not approve of

First I moved the PHP files out of the public folder except for one gateway file

scripts/
   prepend.php
   a.php
   b.php
   ...
public_html/
   gateway.php
   html, css, js, image files

I use .htaccess to redirect all requests for my PHP scripts to gateway.php. This is the file that actually executes the PHP script requested. To reach a.php, the browser requests /php/a...

RewriteEngine On
RewriteBase /    
RewriteRule ^php/(.*)   gateway.php?file=$1.php   [END,QSA,NC]

In php.ini, I set auto_prepend_file to point to scripts/prepend.php. This file is run before all PHP scripts, so I can use it to do some configuration. In part, it has a list of allowed PHP scripts.

prepend.php

//the only scripts that will be allowed to run
Const SAFE_SCRIPTS = '"path/to/gateway.php","/path/to/a.php","/path/to/b.php"';
Const SCRIPTS_PATH = __DIR__;   

//ensure that the requested script is allowed. The check here is to prevent
//a maliciously uploaded PHP file from executing.
$file = $_SERVER['SCRIPT_FILENAME'];//typically /path/to/gateway.php
if(strpos(SAFE_SCRIPTS,"\"$file\"")===false) die("$file is not allowed");

After prepend.php, gateway.php runs; it's in charge of making sure the requested PHP file is on the allowed list and for forwarding the request to the right file

//get the file requested. Comes from the htaccess rule
$file = SCRIPTS_PATH.'/'.$_GET['file'];

//if file is not in the allowed list, abort. The check here is to prevent
//gateway from loading any file it wasn't designed to load
if(strpos(SAFE_SCRIPTS,"\"$file\"")===false) die("$file is not allowed");

//load the file that actually handles the http request
require $file;

Looking for feedback on what's the weakest link and/or how to improve.

Improve this question
\$\endgroup\$
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast Jul 18 '16 at 18:06
  • \$\begingroup\$ @Mast thanks. Actually I made the edit when I realized from Dave's answer that I may get more helpful answers if the code in my question better reflects what I actually do (see my comment under Dave's answer). So I saw the edit as a clarification of the question, not an implementation of a solution. In fact you'll notice that my change was not in line with what Dave suggested, but just made to better reflect my actual code. \$\endgroup\$ – BeetleJuice Jul 18 '16 at 18:14
  • \$\begingroup\$ @Mast so what do you think? Should I re-apply the edit? add an addendum at the end? leave things as they are? \$\endgroup\$ – BeetleJuice Jul 18 '16 at 18:34
  • \$\begingroup\$ On Code Review, we encourage you to post real code, not idealized or simplified versions of it. We review the code you posted, not the code that you had in your mind. If you choose to post an altered version of the code, it's your own responsibility to figure out how to apply the answers you get to your production environment. \$\endgroup\$ – 200_success Jul 19 '16 at 5:26
  • \$\begingroup\$ Thank you. I agree. That's why I edited the OP after getting Dave's answer: to reflect the real code. I think @Mast got the impression that I was altering the question to implement a solution. So should I reapply the edit? add an addendum? or leave things as they are? \$\endgroup\$ – BeetleJuice Jul 19 '16 at 5:31
2
\$\begingroup\$

Remove the additional information you are providing in your die statements. Just simply die quietly without revealing any information. It's similar to putting a zero byte index.html file in directories that you don't want people browsing.

Edit: Going away quietly without providing any information gives less information about your site to someone who may be trying to attack it. Any clues that you give them, like file not found for example, lets them know that they need to change their attack vector. The less information you can give them the better.

Improve this answer
\$\endgroup\$
  • \$\begingroup\$ Thank you. I included it like that for illustration. In the code I return a 404 file not found if the requested file is not in the whitelist. \$\endgroup\$ – BeetleJuice Jul 18 '16 at 17:51
  • \$\begingroup\$ I wouldn't even do that but that's your call. The only other thing you might consider is mapping what is provided in the URL to a different real file name. For example if a is passed it actually maps to real_file_a or something along those lines. Not really needed but it can help stop people from guessing what might work since that mapped name is never revealed. \$\endgroup\$ – Dave Jul 18 '16 at 17:54
  • 1
    \$\begingroup\$ Dave could you please elaborate a little more (in your answer) why you would do this? we like our reviews to be a little bit longer. Thanks! \$\endgroup\$ – Malachi♦ Jul 18 '16 at 19:17
  • \$\begingroup\$ @Dave Thanks for the edit. I get that 404 not found tells the attacker that he needs to use a different path but consider: If all webservers stopped returning404 and just returned empty responses with a 200 OK code, it would have significant negative implications for the site owners and legitimate users. For instance users who misspell URLs will think the site is broken. Admins will be unable to find dead links in server logs. Web spiders will archive dead-end URLs. Browsers will cache bad links... Does it improve security enough to make up for all that in your opinion? \$\endgroup\$ – BeetleJuice Jul 27 '16 at 3:48
  • \$\begingroup\$ Fair point. How you handle things is entirely up to you of course. There are some situations and cases where a 404 is more appropriate and correct. Where I tend to not even return a 404 is when someone is poking around, or trying to poke around, in a "restricted" area of a site. \$\endgroup\$ – Dave Jul 27 '16 at 11:31
0
\$\begingroup\$

Maybe I'm being a bit on the paranoid side here, but strpos will just return if a string is inside of another string.

That means that this code:

if(strpos(SAFE_SCRIPTS,"\"$file\"")===false) die("$file is not allowed");

Will actually return true if $file is something like:

"/path/to/a.php/../../../../../something"

So I think it's probably best if you get only the directory path, evaluating it with maybe realpath, concatenate the filename to the resulting directory and compare that with your whitelist.

Improve this answer
\$\endgroup\$
  • \$\begingroup\$ I don't understand. Note that the search includes the quotes at both ends, I surround the filename with quotes and look for "$file" in the whitelist. So "/path/to/a.php/../something" cannot match "/path/to/a.php". If I were looking for just $file, I could see the vulnerability. \$\endgroup\$ – BeetleJuice Jul 27 '16 at 2:58
  • \$\begingroup\$ to see what I mean: 3v4l.org/DLSo6 \$\endgroup\$ – BeetleJuice Jul 27 '16 at 3:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.