First, read http://drupal.org/node/93204 and patch submitted by chx in that same thread at http://drupal.org/node/93204#comment-465077

chx's patch looks like it was reverted during a security update in July '07 and the problem was reintroduced. See http://drupal.org/files/sa-2007-018/SA-2007-018-5.1.patch for the security patch. I ran into this problem myself running drupal 5 not too long ago when the security patch was issued. See my initial attempt to address this issue here http://drupal.org/node/162924

Today, I downloaded drupal 6 and ran into the same exact problem when trying to install. So, I decided to do a little research and try to solve this problem.

As chx's patch makes clear, the problem is with the $_SERVER['SCRIPT_NAME'] which chx changed to $_SERVER['PHP_SELF']. However, as the comment in the security patch points out, PHP_SELF opens up a potential xss hole.

There is a solution, however. Please refer to http://blog.phpdoc.info/archives/13-guid.html which addresses this very issue with PHP_SELF. The attached patch addresses the potential xss vulnerability.

CommentFileSizeAuthor
#18 php_self2.patch1.88 KBSteve Dondley
#14 php_self.patch1.71 KBSteve Dondley
fastcgi_fix.patch824 bytesSteve Dondley

Comments

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

this is a patch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
heine’s picture

heine commented
Priority: Critical » Normal

The check_plain approach (never use htmlentities directly), is obvious, but not a solution as the following issue (discovered before the security release of our original patch) indicates. Keep in mind that this was with our initial patch for the problem in Drupal 5; I haven't actually tested this with 6 with this patch applied, but the same mechanics should apply.

Enable the upload module, and upload a file called test.txt with the contents:

alert('xss');

Now visit:

http://example.com/index.php/%3Fq%3Dsystem/files/files/test.txt%23/d?q=node/ANID/edit

You will get multiple alerts, for every call to drupal_add_js, where test.txt is loaded in such code as: <script type="text/javascript" src="[injection]misc/jquery.js"></script>

heine’s picture

heine commented

By the way: how do you run fastcgi? Is this just an issue with suexec?

heine’s picture

heine commented
Status: Needs review » Needs work

Sorry.

Steve Dondley’s picture

Steve Dondley commented

Heine,

I would have to get the details from my server guy who set up fastcgi for me. I only have vague notions of how it all works. If you can tell me where to look on my server to help you answer you question, let me know. I'm running a Debian setup.

That said, other people have had the same problem as me. See the first link in my original post. I also found other discussions on d.o with people having the same problem. So whatever my setup is, it's probably fairly common with a fastcgi environment.

So, IMHO, I recommend we try to find a way to use PHP_SELF safely and help others avoid a lot of frustration trying to figure out why drupal doesn't work on their fastcgi server. Others made suggestions for using PHP_SELF on the page I linked to, http://blog.phpdoc.info/archives/13-guid.html, in my original post. perhaps some of them have merit. If using htmlentities won't do it, do you think it's possible to find something else that will?

Personally, I liked this suggestion for removing PATH_INFO:

$script = substr($_SERVER['PHP_SELF'], 0, (strlen($_SERVER['PHP_SELF']) - @strlen($_SERVER['PATH_INFO'])));

Steve Dondley’s picture

Steve Dondley commented

Heine, I cannot duplicate #3 above. I get a blank screen except for a "No input file specified" message.

heine’s picture

heine commented

There were some changes to the way the session id is defined. To reproduce use Firefox 2 and set a specific cookie domain in settings.php.

tayknight’s picture

tayknight commented

Is there a answer to this issue that is secure. I'm having the exact same issue at Dreamhost.

Steve Dondley’s picture

Steve Dondley commented
Version: 6.x-dev » 6.0
Status: Needs work » Needs review

This patch sanitizes $_SERVER['PHP_SELF'] with php's basename() function per recommendation in last paragarph at http://www.mc2design.com/blog/php_self-safe-alternatives

chx’s picture

chx commented
Status: Needs review » Needs work

What about running a decent preg_replace on the thing to keep a whitelist of characters?

Steve Dondley’s picture

Steve Dondley commented
Status: Needs work » Needs review
StatusFileSize
new php_self.patch1.71 KB

OK, patch in #11 was not uploaded. Fixing that problem. #13 was the wrong patch so deleted to avoid confustion. Now, to the issue at hand:

chx, I'm not sure why preg_replace would be superior to using basename. Using a regex seems less elegant than lopping off path information that should not be present at all. I'm all ears though.

The original problem was that with a URI like http://example.com/index.php/this/is/malicious/, $_SERVER['PHP_SELF'] returns /index.php/this/is/malicious. Hence it causes a security issue. $_SERVER['SCRIPT_NAME'], however, properly returns the desired string, /index.php.

However, $_SERVER['SCRIPT_NAME'] has the unintended consequence of breaking at least some fastcgi setups as noted above.

So, this patch uses '/' . basename($_SERVER['PHP_SELF']) in place of $_SERVER['SCRIPT_NAME']. The values returned by each should be precisely equivalent. If anyone can point out a case where they would not be equivalent, let me know and I'll go back to the drawing board.

For more information about basename(), see http://www.php.net/basename
For more information about this issue in general, see http://www.mc2design.com/blog/php_self-safe-alternatives. In the last paragraph, you'll see the recommendation to use basename(), which was the inspiration for this patch.

chx’s picture

chx commented

I htink i like where this patch goes but definitely Heine needs to follow up.

heine’s picture

heine commented
Status: Needs review » Needs work

When Drupal is installed in a subdirectory, basename($_SERVER['PHP_SELF'])) doesn't get us the subdirectory:

$_SERVER['SCRIPT_NAME '] is /sub/index.php
$_SERVER['PHP_SELF'] is /sub/index.php
basename($_SERVER['PHP_SELF']) is index.php
heine’s picture

heine commented

And BTW, basename(sub/index.php/this/is/malicious) == malicious. The article you refer to talks about the basename of $_SERVER['SCRIPT_FILENAME']

Steve Dondley’s picture

Steve Dondley commented
Status: Needs work » Needs review
StatusFileSize
new php_self2.patch1.88 KB

Argh! Foiled again. (Wipes egg off face.)

OK, let me throw this patch at you, Heine. It uses some simple string manipulation:

$script_name = substr($_SERVER['PHP_SELF'], 0, strpos($_SERVER['PHP_SELF'], 'index.php') + 9);
dpearcefl’s picture

dpearcefl commented

Is this still an issue in current D6?

Status: Needs review » Needs work

The last submitted patch, php_self2.patch, failed testing.

multiplextor’s picture

multiplextor commented
Assigned: Steve Dondley » Unassigned
Status: Needs work » Closed (won't fix)

Closed. The reason: expired