dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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
Download Patch File Download Diff File Expand all files Collapse all files

13
code-reviews/Core.Scripting/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 11 |
| Open findings | 10 |
## Checklist coverage
@@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank.
| Severity | Critical |
| Category | Security |
| Location | `ForbiddenTypeAnalyzer.cs:45`, `ScriptSandbox.cs:54` |
| Status | Open |
| Status | Resolved |
**Description:** `System.Environment` lives in the allowed `System` namespace (it is in
`System.Private.CoreLib`, which is allow-listed for primitives) and is not on the
@@ -57,7 +57,14 @@ for types in allowed namespaces. Add an explicit forbidden-member deny-list to
`GC.AddMemoryPressure`), and `System.Activator.CreateInstance` (a reflection-equivalent
escape). Reject these in `CheckSymbol` by resolved method symbol, with a test for each.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — added a type-granular `ForbiddenFullTypeNames`
deny-list (`System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`) to
`ForbiddenTypeAnalyzer`; `CheckSymbol` now rejects any resolved type symbol whose
fully-qualified name matches, alongside the existing namespace-prefix check, so dangerous
`System`-namespace process-control types are blocked at compile while legitimate `System`
types (Math, String, …) stay usable. Regression tests added in `ScriptSandboxTests` for
`Environment.Exit` / `Environment.FailFast` / `Environment.GetEnvironmentVariable` /
`AppDomain` / `GC.Collect` / `Activator.CreateInstance`.
### Core.Scripting-002

73
src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs
View File

@@ -20,10 +20,7 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
/// Deny-list is the authoritative Phase 7 plan decision #6 set:
/// <c>System.IO</c>, <c>System.Net</c>, <c>System.Diagnostics.Process</c>,
/// <c>System.Reflection</c>, <c>System.Threading.Thread</c>,
/// <c>System.Runtime.InteropServices</c>. <c>System.Environment</c> (for process
/// env-var read) is explicitly left allowed — it's read-only process state, doesn't
/// persist outside, and the test file pins this compromise so tightening later is
/// a deliberate plan decision.
/// <c>System.Runtime.InteropServices</c>.
/// </para>
/// <para>
/// Deny-list prefix match. <c>System.Net</c> catches <c>System.Net.Http</c>,
@@ -32,6 +29,21 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
/// operator audience authors it through a helper the plan team adds as part of
/// the <see cref="ScriptContext"/> surface, not by unlocking the namespace.
/// </para>
/// <para>
/// A namespace-prefix deny-list is necessary but not sufficient: dangerous types
/// such as <c>System.Environment</c>, <c>System.AppDomain</c>, <c>System.GC</c>,
/// and <c>System.Activator</c> live <em>directly</em> in the <c>System</c>
/// namespace inside <c>System.Private.CoreLib</c> — the same allow-listed assembly
/// that supplies primitives (<c>int</c>, <c>string</c>, <c>Math</c>). They cannot
/// be blocked by namespace because <c>System</c> itself must stay allowed. They
/// are therefore denied <em>type-granularly</em> via
/// <see cref="ForbiddenFullTypeNames"/>. <c>Environment.Exit</c> /
/// <c>Environment.FailFast</c> kill the in-process OPC UA server outright;
/// <c>Activator.CreateInstance</c> is a reflection-equivalent escape; <c>GC</c>
/// and <c>AppDomain</c> expose process-wide control. Legitimate <c>System</c>
/// types (<c>Math</c>, <c>String</c>, <c>Convert</c>, <c>DateTime</c>, …) are not
/// on the list and stay usable. (Core.Scripting-001.)
/// </para>
/// </remarks>
public static class ForbiddenTypeAnalyzer
{
@@ -53,6 +65,36 @@ public static class ForbiddenTypeAnalyzer
"Microsoft.Win32", // registry
];
/// <summary>
/// Fully-qualified type names scripts are NOT allowed to reference, regardless of
/// namespace. These types live directly in the allow-listed <c>System</c>
/// namespace (in <c>System.Private.CoreLib</c>), so a namespace-prefix rule cannot
/// reach them without also blocking primitives. Matched by exact fully-qualified
/// name against the resolved <em>type</em> symbol — every member of the type
/// (including read-only ones) is therefore rejected.
/// </summary>
/// <remarks>
/// <list type="bullet">
/// <item><c>System.Environment</c> — <c>Exit</c> / <c>FailFast</c> terminate
/// the host process; the whole type is denied (the read members have no
/// legitimate SCADA-predicate use either).</item>
/// <item><c>System.AppDomain</c> — process-wide assembly-load /
/// unhandled-exception control.</item>
/// <item><c>System.GC</c> — <c>Collect</c> / <c>AddMemoryPressure</c> perturb
/// the process memory subsystem.</item>
/// <item><c>System.Activator</c> — <c>CreateInstance</c> is a
/// reflection-equivalent escape that constructs a forbidden type by name
/// without ever naming it syntactically.</item>
/// </list>
/// </remarks>
public static readonly IReadOnlyList<string> ForbiddenFullTypeNames =
[
"System.Environment",
"System.AppDomain",
"System.GC",
"System.Activator",
];
/// <summary>
/// Scan the <paramref name="compilation"/> for references to forbidden types.
/// Returns empty list when the script is clean; non-empty list means the script
@@ -121,6 +163,29 @@ public static class ForbiddenTypeAnalyzer
return;
}
}
// Type-granular deny-list — dangerous types that live in the allow-listed
// System namespace and so cannot be caught by ForbiddenNamespacePrefixes
// (Core.Scripting-001). Matched on the full type name; OriginalDefinition
// unwraps any generic construction before naming.
var fullTypeName = typeSymbol.OriginalDefinition.ToDisplayString(
SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle(
SymbolDisplayGlobalNamespaceStyle.Omitted));
foreach (var forbiddenType in ForbiddenFullTypeNames)
{
if (fullTypeName == forbiddenType)
{
rejections.Add(new ForbiddenTypeRejection(
Span: span,
TypeName: typeSymbol.ToDisplayString(),
Namespace: ns,
Message: $"Type '{forbiddenType}' is on the Phase 7 sandbox forbidden-type " +
$"deny-list. Scripts cannot reach process-control types " +
$"(Environment / AppDomain / GC / Activator) even though they " +
$"live in the allowed 'System' namespace."));
return;
}
}
}
}

88
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs
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]

17
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/TimedScriptEvaluatorTests.cs
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>(