From 1efe182e404dcba05edf860c9af1e2520523baeb Mon Sep 17 00:00:00 2001
From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Thu, 14 Feb 2013 15:44:50 -0500
Subject: [PATCH] Hide mw.makeProtectedEnvFuncs from modules

Allowing a module to call mw.makeProtectedEnvFuncs() lets it bypass the
allowEnvFuncs setting. It can also be used to manipulate the global
tables that other modules' sandboxes will be copied from.

And for paranoia's sake, let's tighten up what setfenv is allowed to
set. This requires changing a unit test, because it is no longer
sane to do something like

 env.setfenv, env.getfenv = mw.makeProtectedEnvFuncs( { [env] = true }, {} )

Nothing real does this, it was only in the unit test.

Change-Id: I8e0d83bb0980ee869af3ac4413afd211717ca92f
---
 engines/LuaCommon/lualib/mw.lua         |  7 +++++++
 tests/engines/LuaCommon/CommonTest.php  |  6 ++++++
 tests/engines/LuaCommon/CommonTests.lua | 14 +++++++-------
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/engines/LuaCommon/lualib/mw.lua b/engines/LuaCommon/lualib/mw.lua
index a766f524..f4e55a4d 100644
--- a/engines/LuaCommon/lualib/mw.lua
+++ b/engines/LuaCommon/lualib/mw.lua
@@ -136,6 +136,9 @@ function mw.executeModule( chunk )
 	local env = mw.clone( _G )
 	makePackageModule( env )
 
+	-- This is unsafe
+	env.mw.makeProtectedEnvFuncs = nil
+
 	if allowEnvFuncs then
 		env.setfenv, env.getfenv = mw.makeProtectedEnvFuncs( {[_G] = true}, {} )
 	else
@@ -188,6 +191,10 @@ function mw.makeProtectedEnvFuncs( protectedEnvironments, protectedFunctions )
 			if protectedFunctions[func] then
 				error( "'setfenv' cannot be called on a protected function", 2 )
 			end
+			local env = old_getfenv( func )
+			if env == nil or protectedEnvironments[ env ] then
+				error( "'setfenv' cannot set the requested environment, it is protected", 2 )
+			end
 			old_setfenv( func, newEnv )
 		else
 			error( "'setfenv' can only be called with a function or integer as the first argument", 2 )
diff --git a/tests/engines/LuaCommon/CommonTest.php b/tests/engines/LuaCommon/CommonTest.php
index 733b0e5b..9fc03867 100644
--- a/tests/engines/LuaCommon/CommonTest.php
+++ b/tests/engines/LuaCommon/CommonTest.php
@@ -45,6 +45,12 @@ class Scribunto_LuaCommonTests extends Scribunto_LuaEngineTestBase {
 
 		// Note this depends on every iteration of the data provider running with a clean parser
 		$this->getEngine()->getParser()->getOptions()->setExpensiveParserFunctionLimit( 10 );
+
+		// Some of the tests need this
+		$interpreter = $this->getEngine()->getInterpreter();
+		$interpreter->callFunction(
+			$interpreter->loadString( 'mw.makeProtectedEnvFuncsForTest = mw.makeProtectedEnvFuncs', 'fortest' )
+		);
 	}
 
 	function getTestModules() {
diff --git a/tests/engines/LuaCommon/CommonTests.lua b/tests/engines/LuaCommon/CommonTests.lua
index 4c5f4192..7580f316 100644
--- a/tests/engines/LuaCommon/CommonTests.lua
+++ b/tests/engines/LuaCommon/CommonTests.lua
@@ -52,7 +52,7 @@ function test.setfenv3()
 	local function jailbreak()
 		setfenv( 2, {} )
 	end
-	local new_setfenv, new_getfenv = mw.makeProtectedEnvFuncs( { [_G] = true }, {} )
+	local new_setfenv, new_getfenv = mw.makeProtectedEnvFuncsForTest( { [_G] = true }, {} )
 	setfenv( jailbreak, {setfenv = new_setfenv} )
 	jailbreak()
 end
@@ -62,17 +62,17 @@ function test.setfenv4()
 	-- environment. It's assumed that any higher-level environment will protect
 	-- itself with its own setfenv wrapper, so this succeeds.
 	local function level3()
+		local protected = {setfenv = setfenv, getfenv = getfenv, mw = mw}
 		local function level2()
-			local env = {setfenv = setfenv}
 			local function level1()
 				setfenv( 3, {} )
 			end
 
+			local env = {}
+			env.setfenv, env.getfenv = mw.makeProtectedEnvFuncsForTest(
+				{[protected] = true}, {} )
 			setfenv( level1, env )()
 		end
-		local protected = {mw = mw}
-		protected.setfenv, protected.getfenv = mw.makeProtectedEnvFuncs(
-			{[protected] = true}, {} )
 		setfenv( level2, protected )()
 	end
 	local unprotected = {setfenv = setfenv, getfenv = getfenv, mw = mw}
@@ -85,7 +85,7 @@ function test.setfenv5()
 	local function allowed()
 		(function() setfenv( 2, {} ) end )()
 	end
-	local new_setfenv, new_getfenv = mw.makeProtectedEnvFuncs( { [_G] = true }, {} )
+	local new_setfenv, new_getfenv = mw.makeProtectedEnvFuncsForTest( { [_G] = true }, {} )
 	setfenv( allowed, {setfenv = new_setfenv} )()
 	return 'ok'
 end
@@ -95,7 +95,7 @@ function test.setfenv6()
 	local function jailbreak()
 		setfenv( target, {} )
 	end
-	local new_setfenv, new_getfenv = mw.makeProtectedEnvFuncs( {}, { [target] = true } )
+	local new_setfenv, new_getfenv = mw.makeProtectedEnvFuncsForTest( {}, { [target] = true } )
 	setfenv( jailbreak, {setfenv = new_setfenv} )()
 end