Merge commit from fork

Check for permissions when evaluating object filters

Author: julianbrost

lib/base/CMakeLists.txt

@@ -64,6 +64,7 @@ set(base_SOURCES
   ringbuffer.cpp ringbuffer.hpp
   scriptframe.cpp scriptframe.hpp
   scriptglobal.cpp scriptglobal.hpp
+  scriptpermission.cpp scriptpermission.hpp
   scriptutils.cpp scriptutils.hpp
   serializer.cpp serializer.hpp
   shared.hpp

lib/base/scriptframe.cpp

@@ -44,14 +44,24 @@ INITIALIZE_ONCE_WITH_PRIORITY([]() {
 	l_StatsNS->Freeze();
 }, InitializePriority::FreezeNamespaces);
 
+/**
+ * Construct a @c ScriptFrame that has `Self` assigned to the global namespace.
+ *
+ * Prefer the other constructor if possible since if misused this may leak global variables
+ * without permissions or senstive variables like TicketSalt in a sandboxed context.
+ *
+ * @todo Remove this constructor and call the other with the global namespace in places where it's actually necessary.
+ */
 ScriptFrame::ScriptFrame(bool allocLocals)
-	: Locals(allocLocals ? new Dictionary() : nullptr), Self(ScriptGlobal::GetGlobals()), Sandboxed(false), Depth(0)
+	: Locals(allocLocals ? new Dictionary() : nullptr), PermChecker(new ScriptPermissionChecker),
+	  Self(ScriptGlobal::GetGlobals()), Sandboxed(false), Depth(0), Globals(nullptr)
 {
 	InitializeFrame();
 }
 
 ScriptFrame::ScriptFrame(bool allocLocals, Value self)
-	: Locals(allocLocals ? new Dictionary() : nullptr), Self(std::move(self)), Sandboxed(false), Depth(0)
+	: Locals(allocLocals ? new Dictionary() : nullptr), PermChecker(new ScriptPermissionChecker), Self(std::move(self)),
+	  Sandboxed(false), Depth(0), Globals(nullptr)
 {
 	InitializeFrame();
 }
@@ -63,6 +73,8 @@ void ScriptFrame::InitializeFrame()
 	if (frames && !frames->empty()) {
 		ScriptFrame *frame = frames->top();
 
+		// See the documentation of `ScriptFrame::Globals` for why these two are inherited and Globals isn't.
+		PermChecker = frame->PermChecker;
 		Sandboxed = frame->Sandboxed;
 	}
 
@@ -79,6 +91,41 @@ ScriptFrame::~ScriptFrame()
 #endif /* I2_DEBUG */
 }
 
+/**
+ * Returns a sanitized copy of the global variables namespace when sandboxed.
+ *
+ * This filters out the TicketSalt variable specifically and any variable for which the
+ * PermChecker does not return 'true'.
+ *
+ * However it specifically keeps the Types, System, and Icinga sub-namespaces, because they're
+ * accessed through globals in ScopeExpression and the user should have access to all Values
+ * contained in these namespaces.
+ *
+ * @return a sanitized copy of the global namespace if sandboxed, a pointer to the global namespace otherwise.
+ */
+Namespace::Ptr ScriptFrame::GetGlobals()
+{
+	if (Sandboxed) {
+		if (!Globals) {
+			Globals = new Namespace;
+			auto globals = ScriptGlobal::GetGlobals();
+			ObjectLock lock{globals};
+			for (auto& [key, val] : globals) {
+				if (key == "TicketSalt") {
+					continue;
+				}
+
+				if (key == "Types" || key == "System" || key == "Icinga" || PermChecker->CanAccessGlobalVariable(key)) {
+					Globals->Set(key, val.Val, val.Const);
+				}
+			}
+		}
+		return Globals;
+	}
+
+	return ScriptGlobal::GetGlobals();
+}
+
 void ScriptFrame::IncreaseStackDepth()
 {
 	if (Depth + 1 > 300)

lib/base/scriptframe.hpp

@@ -5,18 +5,30 @@
 
 #include "base/i2-base.hpp"
 #include "base/dictionary.hpp"
-#include "base/array.hpp"
+#include "base/namespace.hpp"
+#include "base/scriptpermission.hpp"
 #include <boost/thread/tss.hpp>
 #include <stack>
 
 namespace icinga
 {
 
+/**
+ * A frame describing the context a section of script code is executed in.
+ *
+ * This is implemented by each new object that is constructed getting pushed on a thread_local
+ * global stack that is accessible from anywhere during script evaluation.
+ *
+ * Most properties in this frame, like local variables do not carry over to successive frames,
+ * except the `PermChecker` and `Sandboxed` members, which get propagated to enforce access
+ * control and availability of unsafe functions.
+ */
 struct ScriptFrame
 {
 	Dictionary::Ptr Locals;
+	ScriptPermissionChecker::Ptr PermChecker; /* inherited by next frame */
 	Value Self;
-	bool Sandboxed;
+	bool Sandboxed; /* inherited by next frame */
 	int Depth;
 
 	ScriptFrame(bool allocLocals);
@@ -28,7 +40,20 @@ struct ScriptFrame
 
 	static ScriptFrame *GetCurrentFrame();
 
+	Namespace::Ptr GetGlobals();
+
 private:
+	/**
+	 * Caches a sanitized version of the global namespace for the current `ScriptFrame`.
+	 *
+	 * This is a value that is dependent on a ScriptFrame's `Sandboxed` and `CheckPerms`
+	 * members. These are both independent of each other and while they are inherited by
+	 * subsequent frames themselves, their values can be changed for new frames easily.
+	 * Therefore Globals can hold a different value for each ScriptFrame and is not
+	 * inherited.
+	 */
+	Namespace::Ptr Globals;
+
 	static boost::thread_specific_ptr<std::stack<ScriptFrame *> > m_ScriptFrames;
 
 	static void PushFrame(ScriptFrame *frame);

lib/base/scriptpermission.cpp

@@ -0,0 +1,15 @@
+/* Icinga 2 | (c) 2025 Icinga GmbH | GPLv2+ */
+
+#include "base/scriptpermission.hpp"
+
+using namespace icinga;
+
+bool ScriptPermissionChecker::CanAccessGlobalVariable(const String&)
+{
+	return true;
+}
+
+bool ScriptPermissionChecker::CanAccessConfigObject(const ConfigObject::Ptr&)
+{
+	return true;
+}

lib/base/scriptpermission.hpp

@@ -0,0 +1,28 @@
+/* Icinga 2 | (c) 2025 Icinga GmbH | GPLv2+ */
+
+#pragma once
+
+#include "base/string.hpp"
+#include "base/shared-object.hpp"
+#include "base/configobject.hpp"
+
+namespace icinga {
+
+class ScriptPermissionChecker : public SharedObject
+{
+public:
+	DECLARE_PTR_TYPEDEFS(ScriptPermissionChecker);
+
+	ScriptPermissionChecker() = default;
+	ScriptPermissionChecker(const ScriptPermissionChecker&) = delete;
+	ScriptPermissionChecker(ScriptPermissionChecker&&) = delete;
+	ScriptPermissionChecker& operator=(const ScriptPermissionChecker&) = delete;
+	ScriptPermissionChecker& operator=(ScriptPermissionChecker&&) = delete;
+
+	~ScriptPermissionChecker() override = default;
+
+	virtual bool CanAccessGlobalVariable(const String& varName);
+	virtual bool CanAccessConfigObject(const ConfigObject::Ptr& obj);
+};
+
+} // namespace icinga

lib/base/scriptutils.cpp

@@ -35,18 +35,18 @@ REGISTER_FUNCTION(System, exit, &Application::Exit, "status");
 REGISTER_SAFE_FUNCTION(System, typeof, &ScriptUtils::TypeOf, "value");
 REGISTER_SAFE_FUNCTION(System, keys, &ScriptUtils::Keys, "value");
 REGISTER_SAFE_FUNCTION(System, random, &Utility::Random, "");
-REGISTER_SAFE_FUNCTION(System, get_template, &ScriptUtils::GetTemplate, "type:name");
-REGISTER_SAFE_FUNCTION(System, get_templates, &ScriptUtils::GetTemplates, "type");
+REGISTER_FUNCTION(System, get_template, &ScriptUtils::GetTemplate, "type:name");
+REGISTER_FUNCTION(System, get_templates, &ScriptUtils::GetTemplates, "type");
 REGISTER_SAFE_FUNCTION(System, get_object, &ScriptUtils::GetObject, "type:name");
-REGISTER_SAFE_FUNCTION(System, get_objects, &ScriptUtils::GetObjects, "type");
+REGISTER_FUNCTION(System, get_objects, &ScriptUtils::GetObjects, "type");
 REGISTER_FUNCTION(System, assert, &ScriptUtils::Assert, "value");
 REGISTER_SAFE_FUNCTION(System, string, &ScriptUtils::CastString, "value");
 REGISTER_SAFE_FUNCTION(System, number, &ScriptUtils::CastNumber, "value");
 REGISTER_SAFE_FUNCTION(System, bool, &ScriptUtils::CastBool, "value");
 REGISTER_SAFE_FUNCTION(System, get_time, &Utility::GetTime, "");
 REGISTER_SAFE_FUNCTION(System, basename, &Utility::BaseName, "path");
 REGISTER_SAFE_FUNCTION(System, dirname, &Utility::DirName, "path");
-REGISTER_SAFE_FUNCTION(System, getenv, &ScriptUtils::GetEnv, "value");
+REGISTER_FUNCTION(System, getenv, &ScriptUtils::GetEnv, "value");
 REGISTER_SAFE_FUNCTION(System, msi_get_component_path, &ScriptUtils::MsiGetComponentPathShim, "component");
 REGISTER_SAFE_FUNCTION(System, escape_shell_cmd, &Utility::EscapeShellCmd, "cmd");
 REGISTER_SAFE_FUNCTION(System, escape_shell_arg, &Utility::EscapeShellArg, "arg");
@@ -473,7 +473,15 @@ ConfigObject::Ptr ScriptUtils::GetObject(const Value& vtype, const String& name)
 	if (!ctype)
 		return nullptr;
 
-	return ctype->GetObject(name);
+	auto cfgObj = ctype->GetObject(name);
+	if (cfgObj) {
+		auto* frame = ScriptFrame::GetCurrentFrame();
+		if (frame->PermChecker->CanAccessConfigObject(cfgObj)) {
+			return cfgObj;
+		}
+	}
+
+	return nullptr;
 }
 
 Array::Ptr ScriptUtils::GetObjects(const Type::Ptr& type)

lib/config/expression.cpp

@@ -118,8 +118,10 @@ ExpressionResult VariableExpression::DoEvaluate(ScriptFrame& frame, DebugHint *d
 		return value;
 	else if (VMOps::FindVarImport(frame, m_Imports, m_Variable, &value, m_DebugInfo))
 		return value;
-	else
-		return ScriptGlobal::Get(m_Variable);
+	else if (frame.GetGlobals()->Get(m_Variable, &value))
+		return value;
+
+	BOOST_THROW_EXCEPTION(ScriptError{"Tried to access undefined script variable '" + m_Variable + "'"});
 }
 
 bool VariableExpression::GetReference(ScriptFrame& frame, bool init_dict, Value *parent, String *index, DebugHint **dhint) const
@@ -138,8 +140,8 @@ bool VariableExpression::GetReference(ScriptFrame& frame, bool init_dict, Value
 			*dhint = new DebugHint((*dhint)->GetChild(m_Variable));
 	} else if (VMOps::FindVarImportRef(frame, m_Imports, m_Variable, parent, m_DebugInfo)) {
 		return true;
-	} else if (ScriptGlobal::Exists(m_Variable)) {
-		*parent = ScriptGlobal::GetGlobals();
+	} else if (frame.GetGlobals()->Contains(m_Variable)) {
+		*parent = frame.GetGlobals();
 
 		if (dhint)
 			*dhint = nullptr;
@@ -546,7 +548,7 @@ ExpressionResult GetScopeExpression::DoEvaluate(ScriptFrame& frame, DebugHint *d
 	else if (m_ScopeSpec == ScopeThis)
 		return frame.Self;
 	else if (m_ScopeSpec == ScopeGlobal)
-		return ScriptGlobal::GetGlobals();
+		return frame.GetGlobals();
 	else
 		VERIFY(!"Invalid scope.");
 }