fix(scripting): block dangerous System types in the script sandbox (Core.Scripting-001)

ForbiddenTypeAnalyzer used only a namespace-prefix deny-list. System.Environment,
System.AppDomain, System.GC and System.Activator live directly in the System
namespace, which must stay allowed for primitives (Math, String, ...), so they
were never caught — an operator-authored predicate could call
System.Environment.Exit(0) and terminate the in-process OPC UA server.

Add a type-granular deny-list (ForbiddenFullTypeNames) checked by
fully-qualified type name after the namespace-prefix check; legitimate System
types are unaffected.

Regression tests assert scripts referencing Environment/AppDomain/GC/Activator
are rejected at analysis time. Core.Scripting suite: 68/68 pass.

Resolves code-review finding Core.Scripting-001 (Critical).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 05:54:08 -04:00
parent 973730d0eb
commit cfb9ff1032
4 changed files with 162 additions and 29 deletions

View File

@@ -92,23 +92,83 @@ public sealed class ScriptSandboxTests
"""));
}
[Fact]
public void Rejects_Environment_Exit_at_compile()
{
// System.Environment lives in System.Private.CoreLib (allow-listed for
// primitives) so a namespace-prefix deny-list cannot block it. Environment.Exit
// terminates the whole in-process OPC UA server — every connected client and
// every driver — so it MUST be rejected member-granularly. (Core.Scripting-001.)
Should.Throw<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
System.Environment.Exit(0);
return 0;
"""));
}
[Fact]
public void Rejects_Environment_FailFast_at_compile()
{
// Environment.FailFast crashes the host process immediately — same outage as
// Exit. (Core.Scripting-001.)
Should.Throw<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
System.Environment.FailFast("boom");
return 0;
"""));
}
[Fact]
public void Rejects_AppDomain_at_compile()
{
// AppDomain.CurrentDomain exposes process-wide control (assembly load events,
// unhandled-exception hooks). Not script surface. (Core.Scripting-001.)
Should.Throw<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
var n = System.AppDomain.CurrentDomain.FriendlyName;
return 0;
"""));
}
[Fact]
public void Rejects_GC_Collect_at_compile()
{
// GC.Collect / GC.AddMemoryPressure let a script perturb the whole process's
// memory subsystem. Not script surface. (Core.Scripting-001.)
Should.Throw<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
System.GC.Collect();
return 0;
"""));
}
[Fact]
public void Rejects_Activator_CreateInstance_at_compile()
{
// Activator.CreateInstance is a reflection-equivalent escape — it can construct
// a forbidden type by name without ever naming it syntactically. (Core.Scripting-001.)
Should.Throw<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
var o = System.Activator.CreateInstance(typeof(object));
return 0;
"""));
}
[Fact]
public void Rejects_Environment_GetEnvironmentVariable_at_compile()
{
// Environment lives in System.Private.CoreLib (allow-listed for primitives) —
// BUT calling .GetEnvironmentVariable exposes process state we don't want in
// scripts. In an allow-list sandbox this passes because mscorlib is allowed;
// relying on ScriptSandbox alone isn't enough for the Environment class. We
// document here that the CURRENT sandbox allows Environment — acceptable because
// Environment doesn't leak outside the process boundary, doesn't side-effect
// persistent state, and Phase 7 plan decision #6 targets File/Net/Process/
// reflection specifically.
//
// This test LOCKS that compromise: operators should not be surprised if a
// script reads an env var. If we later decide to tighten, this test flips.
var evaluator = ScriptEvaluator<FakeScriptContext, string?>.Compile(
"""return System.Environment.GetEnvironmentVariable("PATH");""");
evaluator.ShouldNotBeNull();
// The whole System.Environment type is forbidden (Core.Scripting-001) — even the
// read-only GetEnvironmentVariable member. Once Exit / FailFast made the type
// dangerous, the cleanest member-granular rule is to deny the type outright; the
// read path has no legitimate use in a SCADA predicate either.
Should.Throw<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, string?>.Compile(
"""return System.Environment.GetEnvironmentVariable("PATH");"""));
}
[Fact]

View File

@@ -29,12 +29,13 @@ public sealed class TimedScriptEvaluatorTests
[Fact]
public async Task Script_longer_than_timeout_throws_ScriptTimeoutException()
{
// Scripts can't easily do Thread.Sleep in the sandbox (System.Threading.Thread
// is denied). But a tight CPU loop exceeds any short timeout.
// Scripts can't reach the sandbox-denied process surface (System.Threading.Thread
// and System.Environment are both denied — Core.Scripting-001). A tight CPU loop
// that never returns exceeds any short timeout without touching a forbidden type.
var inner = ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
var end = Environment.TickCount64 + 5000;
while (Environment.TickCount64 < end) { }
long acc = 0;
while (true) { acc++; if (acc < 0) break; }
return 1;
""");
var timed = new TimedScriptEvaluator<FakeScriptContext, int>(
@@ -54,8 +55,8 @@ public sealed class TimedScriptEvaluatorTests
// paths aren't misclassified as timeouts.
var inner = ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
var end = Environment.TickCount64 + 10000;
while (Environment.TickCount64 < end) { }
long acc = 0;
while (true) { acc++; if (acc < 0) break; }
return 1;
""");
var timed = new TimedScriptEvaluator<FakeScriptContext, int>(
@@ -119,8 +120,8 @@ public sealed class TimedScriptEvaluatorTests
{
var inner = ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
var end = Environment.TickCount64 + 5000;
while (Environment.TickCount64 < end) { }
long acc = 0;
while (true) { acc++; if (acc < 0) break; }
return 1;
""");
var timed = new TimedScriptEvaluator<FakeScriptContext, int>(