Textpattern CMS

#1 2012-01-10 20:48:33

Gocom

Developer Emeritus

From: Helsinki, Finland

Registered: 2006-07-14

Posts: 4,533

Website

Superglobals, register_global and Textpattern's un-registering

Textpattern does include that neat snippet for preventing superglobal registering from kicking in. That snippet that is at the beginning of index.php files. But, if I were to actually turn (deprecated as of 5.3) register_global directive on, that doesn’t do what it’s supposed.

I see couple issues:

• It unsets any global variable no matter what it is. For example passing ?_SERVER to URL does something it’s not supposed to do.
• It doesn’t unset every registered global. Used $_REQUEST doesn’t contain $_SERVER and runtime modification do not carry over. Well, runtime modifications shouldn’t really matter as long as index.php is the first mouth in the food chain. Altho, as of 5.3.0, REQUEST follows it’s own order directive, not variable_order which effects register globals.
• Added to that some of the array type globals in Textpattern are not defined. For example txpcfg is a global that is never actually set as empty array. If one of the $txpcfg options is left unset, well, you see where I’m going.
• What about $GLOBALS, the variables are there too, right?

I would suggest either:

• Removing the feature. If it doesn’t work, it’s kind of pointless.
• But because of b/c it might be good idea to patch it. Emulating register_globals off securely might be harder than expected, considering that session_start() and likes. Also as far as I know, not all “superglobals” are protected from global registering. When one those (non underscore starting) is detected in EGPCS Textpattern needs… to kill itself, I suppose. Textpattern does officially support PHP versions starting from 4.3.0, right?

Speaking of patches, here is one for suggestion #2 which implement some additional protection against register_globals hazards.

/textpattern/index.php

--- development/4.x/textpattern/index.php	2011-06-18 11:03:32.000000000 +0300
+++ development/4.x/textpattern/index.patch.php	2012-01-10 22:30:55.000000000 +0200
@@ -13,9 +13,38 @@
 $LastChangedRevision: 3577 $

 */
-	if (@ini_get('register_globals'))
-		foreach ( $_REQUEST as $name => $value )
-			unset($$name);
+	if (@ini_get('register_globals')) {
+		
+		if (isset($_REQUEST['GLOBALS']) || isset($_FILES['GLOBALS'])) {
+			die('GLOBALS overwrite attempt detected. Please consider turning register_globals off.');
+		}
+		
+		foreach (
+			array_merge(
+				isset($_SESSION) ? (array) $_SESSION : array(),
+				(array) $_ENV,
+				(array) $_GET,
+				(array) $_POST,
+				(array) $_COOKIE,
+				(array) $_FILES,
+				(array) $_SERVER
+			) as $name => $value
+		) {
+			
+			if (!in_array($name, array(
+				'GLOBALS',
+				'_SERVER',
+				'_GET',
+				'_POST',
+				'_FILES',
+				'_COOKIE',
+				'_SESSION',
+				'_REQUEST',
+				'_ENV',
+			)))
+				unset($GLOBALS[$name], $$name);
+		}
+	}

 	if (!defined('txpath'))
 	{

And /index.php

--- development/4.x/index.php	2011-06-13 14:06:46.000000000 +0300
+++ development/4.x/index.patch.php	2012-01-10 22:30:10.000000000 +0200
@@ -8,9 +8,39 @@
 	error_reporting(E_ALL);
 	@ini_set("display_errors","1");

-	if (@ini_get('register_globals'))
-		foreach ( $_REQUEST as $name => $value )
-			unset($$name);
+	if (@ini_get('register_globals')) {
+		
+		if (isset($_REQUEST['GLOBALS']) || isset($_FILES['GLOBALS'])) {
+			die('GLOBALS overwrite attempt detected. Please consider turning register_globals off.');
+		}
+		
+		foreach (
+			array_merge(
+				isset($_SESSION) ? (array) $_SESSION : array(),
+				(array) $_ENV,
+				(array) $_GET,
+				(array) $_POST,
+				(array) $_COOKIE,
+				(array) $_FILES,
+				(array) $_SERVER
+			) as $name => $value
+		) {
+			
+			if (!in_array($name, array(
+				'GLOBALS',
+				'_SERVER',
+				'_GET',
+				'_POST',
+				'_FILES',
+				'_COOKIE',
+				'_SESSION',
+				'_REQUEST',
+				'_ENV',
+			)))
+				unset($GLOBALS[$name], $$name);
+		}
+	}
+
 	define("txpinterface", "public");

 	if (!defined('txpath'))

The implementation works better than what is currently used, but I haven’t done any super-special investigation. I would suspect that plugins may nullify the benefits if they use some of the non-superglobal globals directly as those are not protected from overwriting, and sessions are not really protected either. Session are not initialized until session_start(), and well, session_start()…

To be honest, if there wasn’t b/c and some of those shared hosts, I would recommend just removing the whole deal from Textpattern. At least I wouldn’t leave as it’s now as it has no benefit.

Last edited by Gocom (2012-01-10 20:53:37)

---

Rah | GitHub

#2 2012-01-12 05:01:39

wet

Developer Emeritus

From: Schoerfling, Austria

Registered: 2005-06-06

Posts: 3,324

Website Mastodon

Re: Superglobals, register_global and Textpattern's un-registering

Gocom wrote:

For example txpcfg is a global that is never actually set as empty array. If one of the $txpcfg options is left unset, well, you see where I’m going.

Frankly, no. E_NOTAMINDREADER.

#3 2012-01-12 05:08:26

Gocom

Developer Emeritus

From: Helsinki, Finland

Registered: 2006-07-14

Posts: 4,533

Website

Re: Superglobals, register_global and Textpattern's un-registering

wet wrote:

Frankly, no. E_NOTAMINDREADER.

I.e. the value from _FILES is used instead of the value variable that was supposed to be unset.

Last edited by Gocom (2012-01-12 05:11:23)

---

Rah | GitHub