Security Review / Globals

[jaswrks]

I haven't looked at this yet (at all), but I just remembered that I enabled this feature a long time ago, and it's probably a good idea to just review it again, in case there is anything that could be considered a security issue with respect to the $_POST, $_GET, $_SERVER data being made possible with these replacement codes.

[raamdev]

Great idea. If you could help me with this, that would be great. :) If not, no worries.

Adding this to the Next Release milestone.

[jaswrks]

Definitely. I'll take a look shortly.

[jaswrks]

I just did a review of this functionality. There was no security issue. In fact, it was not working at all. It looks like I may have changed something in this awhile back, because the regex was broken, and a change in the namespace was broken also. I just fixed it up :-) I'll submit a PR in just a moment.

---

You might consider taking this functionality out of the plugin. I suspect it is rarely used anyway, if at all. Anyone looking at this code is going to suspect it as being a vulnerability. It's an extremely tricky trick that we are pulling off there; and while it's totally safe, it would take a PHP guru to be able to prove that to others. Perfectly safe! But it's looks really bad.

public static function _url_e_gprcs_value($m)
            {
                if(isset($m[1], $m[2]) && in_array(($gprcs = strtoupper($m[1])), array('_GET', '_POST', '_REQUEST', '_COOKIE', '_SESSION'), TRUE))
                    if(strlen($element_w_brackets = $m[2]) && preg_match('/^(?:(?:\[(["\'])[a-z0-9 \._\-]+?\\1\])|(?:\[[0-9]+\]))+$/i', $element_w_brackets))
                        eval('$value = urlencode(trim(stripslashes((string)@$'.$gprcs.$element_w_brackets.')));');

                return !empty($value) ? $value : ''; // Default to empty string.
            }

---

Proof that it's safe...

1. The regex validation that occurs here is very important for security.
2. Secondly, the value is forced to a string, and we only collect the value, even though it appears that we are magically evaluating it. Tricky tricky. It's one of those things that's just too tricky.

Proof that it's safe...

<?php
error_reporting(-1);
ini_set('display_errors', TRUE);

$gprcs              = '_GET';
$element_w_brackets = '["hello"]';
$_GET['hello']      = 'mt_rand()';
echo eval('return urlencode(trim(stripslashes((string)@$'.$gprcs.$element_w_brackets.')));');

// mt_rand%28%29

[jaswrks]

If you agree, I can submit a second PR and remove this.

[raamdev]

Perfectly safe! But it's looks really bad.

I see what you mean. Thanks for fixing this up!

You might consider taking this functionality out of the plugin. I suspect it is rarely used anyway, if at all.

I think I might just make it an "advanced option" that a site owner can enable if they want that added functionality. I'd include some additional notes in there about how the code has been thoroughly reviewed for security.

---

I'll merge your PR so that at least the fix is in the trunk and then open a new issue to look into disabling this functionality by default.

[raamdev]

Closed by PR #21.

[raamdev]

Next release changelog:

• Bug Fix: Fixed a bug that was preventing the PHP Globals replacement codes from working properly. Props @jaswsinc. See #18.

[jaswrks]

I think I might just make it an "advanced option" that a site owner can enable if they want that added functionality. I'd include some additional notes in there about how the code has been thoroughly reviewed for security.

OK, cool.