From 2e9160317f23847f01cbc29325b39480d64eb7b5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 06:45:03 -0400 Subject: [PATCH] feat(scripting): deny System.Threading, Type reflection entry points and RuntimeHelpers in the sandbox (E2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R05:10 — the namespace deny-list covered System.Threading.Tasks but not System.Threading, so SemaphoreSlim / Monitor / SpinWait / Mutex / EventWaitHandle could park a script's pool thread past its budget (or touch a system-global kernel object); System.Type.GetType(string) / GetMethod* / InvokeMember and System.Runtime.CompilerServices.RuntimeHelpers resolved and reach a forbidden type without naming it syntactically. ForbiddenTypeAnalyzer now denies the whole System.Threading namespace with a type-granular allow-list (CancellationToken, Interlocked, Volatile — the sync-free primitives), adds a member deny-list for System.Type's reflection entry points (GetType/GetMethod(s)/InvokeMember/GetProperty(ies)/GetField(s)/ GetMember(s)/GetConstructor(s)/GetEvent(s)/GetDefaultMembers; instance object.GetType() stays allowed) and puts RuntimeHelpers on the type deny-list. Thread/ThreadPool/Timer leave the type list — the prefix rule reaches them. R05:11 — the allocation budget landed with E1's ScriptRunGuard (Scripting:MaxAllocBytesPerRun, default 64 MB): sampled around each run, breach quarantines. It is observation after the fact; a pre-emptive OOM guard still needs an out-of-process runner (follow-up issue in the lane report). The Monaco editor shares the analyzer, so DiagnoseTests pins every new form as an OTSCRIPT_FORBIDDEN marker and every allow-listed form as clean. Refs #734 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../ForbiddenTypeAnalyzer.cs | 191 ++++++++++++------ .../ScriptSandboxHardeningTests.cs | 148 ++++++++++++++ .../ScriptAnalysis/DiagnoseTests.cs | 20 ++ 3 files changed, 300 insertions(+), 59 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxHardeningTests.cs diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs index 005734b6..5f7d8140 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs @@ -17,13 +17,15 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting; /// /// /// -/// Deny-list is the authoritative Phase 7 plan set: +/// Deny-list is the authoritative Phase 7 plan set, hardened since: /// System.IO, System.Net, System.Diagnostics, -/// System.Reflection, System.Threading.Thread, -/// System.Threading.Tasks (scripts are synchronous predicates — no -/// legitimate need to start background tasks; a Task.Run fan-out outlives -/// the evaluation timeout entirely), System.Runtime.InteropServices, -/// Microsoft.Win32. +/// System.Reflection, System.Threading.Tasks and the whole of +/// System.Threading (scripts are synchronous predicates — no legitimate need +/// to start background work, and no blocking primitive may park the evaluation's pool +/// thread past its budget; CancellationToken / Interlocked / +/// Volatile are allow-listed back in), System.Runtime.InteropServices, +/// System.Runtime.Loader, Microsoft.Win32; plus the member-granular +/// (System.Type's reflection entry points). /// /// /// Deny-list prefix match. System.Net catches System.Net.Http, @@ -63,13 +65,19 @@ public static class ForbiddenTypeAnalyzer "System.Net", "System.Diagnostics", // catches Process, ProcessStartInfo, EventLog, Trace/Debug file sinks "System.Reflection", - // System.Threading.Thread is NOT in this list: Thread's containing namespace is - // "System.Threading" (not "System.Threading.Thread"), so a prefix check on - // "System.Threading.Thread" never matches. Thread is denied type-granularly via - // ForbiddenFullTypeNames instead so the check actually fires. "System.Threading.Tasks", // Task.Run / Parallel — scripts are synchronous predicates // and have no legitimate need to start background work; // a Task fan-out outlives the evaluation timeout entirely. + // Listed BEFORE the parent prefix so its rejection names it. + "System.Threading", // Thread / ThreadPool / Timer (background work that outlives + // the timeout) AND every blocking primitive — SemaphoreSlim, + // Monitor, SpinWait, ManualResetEvent(Slim), Mutex, + // EventWaitHandle — which park the script's pool thread past + // its budget (the same orphan ScriptRunGuard exists to bound) + // and, for the named kernel objects, touch system-global + // state. The few sync-free primitives a script may + // legitimately name are allow-listed in + // AllowedTypesInForbiddenNamespaces. "System.Runtime.InteropServices", "System.Runtime.Loader", // AssemblyLoadContext + AssemblyDependencyResolver — // arbitrary DLL load into the host process. @@ -79,11 +87,55 @@ public static class ForbiddenTypeAnalyzer "Microsoft.Win32", // registry ]; + /// + /// Types that stay reachable although their namespace is in + /// . Matched by exact fully-qualified name against the + /// resolved type symbol (generic constructions unwrapped). The whole of System.Threading is + /// denied; these three are the sync-free primitives with no blocking surface — a + /// is read-only state, and + /// are single-instruction memory operations. + /// + public static readonly IReadOnlyList AllowedTypesInForbiddenNamespaces = + [ + "System.Threading.CancellationToken", + "System.Threading.Interlocked", + "System.Threading.Volatile", + ]; + + /// + /// Members scripts are NOT allowed to call on otherwise-allowed types, as + /// (declaring type, member name). System.Type must stay reachable + /// (typeof, Name, IsValueType are ordinary metadata reads) but its reflection + /// entry points obtain a type or member by NAME — Type.GetType("System.IO.File") reaches a + /// forbidden type without ever naming it syntactically, and GetMethod* / GetProperty + /// / InvokeMember / … hand back a System.Reflection handle (which pass 2 catches when + /// declared, but not when consumed inline). Instance object.GetType() is declared on + /// System.Object and is not matched. + /// + public static readonly IReadOnlyList<(string Type, string Member)> ForbiddenMembers = + [ + ("System.Type", "GetType"), + ("System.Type", "GetMethod"), + ("System.Type", "GetMethods"), + ("System.Type", "InvokeMember"), + ("System.Type", "GetProperty"), + ("System.Type", "GetProperties"), + ("System.Type", "GetField"), + ("System.Type", "GetFields"), + ("System.Type", "GetMember"), + ("System.Type", "GetMembers"), + ("System.Type", "GetConstructor"), + ("System.Type", "GetConstructors"), + ("System.Type", "GetEvent"), + ("System.Type", "GetEvents"), + ("System.Type", "GetDefaultMembers"), + ]; + /// /// Fully-qualified type names scripts are NOT allowed to reference, regardless of /// namespace. Used when a dangerous type shares its namespace with legitimate types - /// (e.g. System.Threading hosts both CancellationToken and - /// Thread) so a namespace-prefix rule would over-block. Matched by exact + /// (e.g. System.Runtime.CompilerServices hosts both MethodImplOptions and + /// RuntimeHelpers) so a namespace-prefix rule would over-block. Matched by exact /// fully-qualified name against the resolved type symbol — every member of /// the type (including read-only ones) is therefore rejected. /// @@ -99,19 +151,22 @@ public static class ForbiddenTypeAnalyzer /// System.Activator — CreateInstance is a /// reflection-equivalent escape that constructs a forbidden type by name /// without ever naming it syntactically. - /// System.Threading.Thread — raw thread creation bypasses the - /// per-evaluation timeout; denied type-granularly because its containing - /// namespace is System.Threading (shared with allowed types like - /// CancellationToken), so a namespace-prefix rule cannot reach it - /// without blocking unrelated types. - /// System.Threading.ThreadPool / System.Threading.Timer — - /// background-work vectors that outlive the per-evaluation timeout; same - /// threat as Task.Run. /// System.Runtime.CompilerServices.Unsafe — exposes raw type- /// reinterpretation (As<TFrom,TTo>, As<T>(object)) /// that bypasses the CLR type system without requiring an unsafe context at /// the call site; enables type-confusion and managed heap corruption. + /// System.Runtime.CompilerServices.RuntimeHelpers — + /// GetUninitializedObject constructs an instance of any type (a forbidden + /// one included) with no constructor and no syntactic type name beyond a + /// Type value; PrepareMethod / RunClassConstructor reach + /// runtime internals. /// + /// + /// System.Threading.Thread / ThreadPool / Timer used to be listed + /// here; since the whole System.Threading namespace is denied (with a type-granular + /// allow-list, see ) the prefix rule reaches + /// them first. + /// /// public static readonly IReadOnlyList ForbiddenFullTypeNames = [ @@ -119,26 +174,6 @@ public static class ForbiddenTypeAnalyzer "System.AppDomain", "System.GC", "System.Activator", - // System.Threading.Thread lives in the System.Threading namespace (shared with - // CancellationToken, SemaphoreSlim, etc.), so a namespace-prefix deny-list cannot - // target it without blocking those legitimate types. Denied type-granularly here. - "System.Threading.Thread", - // Broadening the references list to the BCL trusted-platform-assemblies set - // re-exposed two background-work vectors the original deny-list missed. Both live - // in System.Threading (shared with allowed sync primitives like CancellationToken / - // SemaphoreSlim), so they must be denied type-granularly: - // - // System.Threading.ThreadPool — QueueUserWorkItem / UnsafeQueueUserWorkItem - // re-introduce the background-fanout threat - // closed against System.Threading.Tasks. - // System.Threading.Timer — Timer(callback, …) schedules unbounded work - // that outlives the per-evaluation timeout. - // - // System.Runtime.Loader.AssemblyLoadContext is also covered, but at the namespace- - // prefix level above (System.Runtime.Loader) so future BCL additions to that - // namespace are denied by default. - "System.Threading.ThreadPool", - "System.Threading.Timer", // System.Runtime.CompilerServices.Unsafe exposes raw type-reinterpretation // (Unsafe.As, Unsafe.As(object)) that bypasses the CLR type system // without requiring an 'unsafe' compilation context at the call site. In a script, @@ -148,6 +183,11 @@ public static class ForbiddenTypeAnalyzer // compile-time-only types (CallerMemberNameAttribute, MethodImplAttribute, // MethodImplOptions) that scripts may legitimately reference. "System.Runtime.CompilerServices.Unsafe", + // RuntimeHelpers.GetUninitializedObject(Type) builds an instance of ANY type — a + // forbidden one obtained through Type.GetType (itself denied via ForbiddenMembers) + // included — without a constructor or a syntactic type name. Same namespace caveat + // as Unsafe above, so type-granular. + "System.Runtime.CompilerServices.RuntimeHelpers", ]; /// @@ -277,28 +317,61 @@ public static class ForbiddenTypeAnalyzer if (rejections.Any(r => r.Span == span && r.TypeName == typeName)) return; - var ns = typeSymbol.ContainingNamespace?.ToDisplayString() ?? string.Empty; - foreach (var forbidden in ForbiddenNamespacePrefixes) - { - if (ns == forbidden || ns.StartsWith(forbidden + ".", StringComparison.Ordinal)) - { - rejections.Add(new ForbiddenTypeRejection( - Span: span, - TypeName: typeName, - Namespace: ns, - Message: $"Type '{typeName}' is in the forbidden namespace '{ns}'. " + - $"Scripts cannot reach {forbidden}* per Phase 7 sandbox rules.")); - return; - } - } - - // Type-granular deny-list — dangerous types that live in the allow-listed - // System namespace and so cannot be caught by ForbiddenNamespacePrefixes. - // Matched on the full type name; OriginalDefinition unwraps any generic - // construction before naming. + // Full name of the (unwrapped) type — shared by the allow-list, the type deny-list and the + // member deny-list below. OriginalDefinition unwraps any generic construction before naming. var fullTypeName = typeSymbol.OriginalDefinition.ToDisplayString( SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle( SymbolDisplayGlobalNamespaceStyle.Omitted)); + + var ns = typeSymbol.ContainingNamespace?.ToDisplayString() ?? string.Empty; + + // Member deny-list first: the containing type (System.Type) is allowed, so neither list + // below would ever look at the member name. + if (symbol is IMethodSymbol method) + { + var declaring = method.ContainingType.OriginalDefinition.ToDisplayString( + SymbolDisplayFormat.FullyQualifiedFormat.WithGlobalNamespaceStyle( + SymbolDisplayGlobalNamespaceStyle.Omitted)); + foreach (var (type, member) in ForbiddenMembers) + { + if (declaring == type && method.Name == member) + { + rejections.Add(new ForbiddenTypeRejection( + Span: span, + TypeName: $"{type}.{member}", + Namespace: ns, + Message: $"Member '{type}.{member}' is on the sandbox forbidden-member deny-list: " + + "it obtains a type or member by name, which bypasses the type allow-list " + + "(see ForbiddenTypeAnalyzer.ForbiddenMembers for the authoritative list).")); + return; + } + } + } + + // A type on the allow-list stays reachable even though its namespace is denied; it is still + // subject to the type-granular deny-list below (nothing is on both today). + var allowListed = AllowedTypesInForbiddenNamespaces.Contains(fullTypeName, StringComparer.Ordinal); + + if (!allowListed) + { + foreach (var forbidden in ForbiddenNamespacePrefixes) + { + if (ns == forbidden || ns.StartsWith(forbidden + ".", StringComparison.Ordinal)) + { + rejections.Add(new ForbiddenTypeRejection( + Span: span, + TypeName: typeName, + Namespace: ns, + Message: $"Type '{typeName}' is in the forbidden namespace '{ns}'. " + + $"Scripts cannot reach {forbidden}* per Phase 7 sandbox rules.")); + return; + } + } + } + + // Type-granular deny-list — dangerous types that live in an allow-listed namespace + // (System, System.Runtime.CompilerServices) and so cannot be caught by + // ForbiddenNamespacePrefixes. foreach (var forbiddenType in ForbiddenFullTypeNames) { if (fullTypeName == forbiddenType) diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxHardeningTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxHardeningTests.cs new file mode 100644 index 00000000..5c932832 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxHardeningTests.cs @@ -0,0 +1,148 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Scripting; + +namespace ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests; + +/// +/// Sandbox hardening (review R05:10): System.Threading is denied as a namespace with a +/// type-granular allow-list (CancellationToken, Interlocked, Volatile) so a +/// script can no longer park its pool thread on a blocking primitive or touch a system-global kernel +/// object; System.Type's reflection entry points (GetType(string), GetMethod*, +/// InvokeMember, …) and System.Runtime.CompilerServices.RuntimeHelpers are denied so a +/// forbidden type cannot be reached by name without ever being named syntactically. +/// +[Trait("Category", "Unit")] +public sealed class ScriptSandboxHardeningTests +{ + private static ScriptSandboxViolationException Rejects(string source) => + Should.Throw(() => + ScriptEvaluator.Compile(source)); + + // ── System.Threading blocking / synchronisation primitives ─────────────────────────────── + + [Fact] + public void Rejects_SemaphoreSlim_Wait() + { + var ex = Rejects("new System.Threading.SemaphoreSlim(0).Wait(); return 0;"); + ex.Rejections.ShouldContain(r => r.TypeName.Contains("SemaphoreSlim") && r.Namespace == "System.Threading"); + } + + [Fact] + public void Rejects_SpinWait_SpinUntil() + => Rejects("System.Threading.SpinWait.SpinUntil(() => false); return 0;"); + + [Fact] + public void Rejects_Monitor() + => Rejects("var o = new object(); System.Threading.Monitor.Enter(o); return 0;"); + + [Fact] + public void Rejects_named_Mutex_system_global_kernel_object() + => Rejects("using var m = new System.Threading.Mutex(false, \"Global\\\\otopcua\"); return 0;"); + + [Fact] + public void Rejects_ManualResetEventSlim() + => Rejects("var e = new System.Threading.ManualResetEventSlim(false); e.Wait(); return 0;"); + + [Fact] + public void Rejects_EventWaitHandle() + => Rejects("var e = new System.Threading.EventWaitHandle(false, System.Threading.EventResetMode.ManualReset); return 0;"); + + [Fact] + public void Rejects_Thread_ThreadPool_and_Timer_still() + { + Rejects("var t = new System.Threading.Thread(() => { }); return 0;"); + Rejects("System.Threading.ThreadPool.QueueUserWorkItem(_ => { }); return 0;"); + Rejects("var t = new System.Threading.Timer(_ => { }); return 0;"); + } + + [Fact] + public void Rejects_Thread_Sleep() + => Rejects("System.Threading.Thread.Sleep(10); return 0;"); + + // ── the allow-list inside System.Threading ─────────────────────────────────────────────── + + [Fact] + public async Task Allows_CancellationToken() + { + using var ev = ScriptEvaluator.Compile( + "var t = System.Threading.CancellationToken.None; return t.IsCancellationRequested ? 1 : 0;"); + (await ev.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken)).ShouldBe(0); + } + + [Fact] + public async Task Allows_Interlocked() + { + using var ev = ScriptEvaluator.Compile( + "var x = 41; System.Threading.Interlocked.Increment(ref x); return x;"); + (await ev.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken)).ShouldBe(42); + } + + [Fact] + public async Task Allows_Volatile() + { + using var ev = ScriptEvaluator.Compile( + "var x = 42; return System.Threading.Volatile.Read(ref x);"); + (await ev.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken)).ShouldBe(42); + } + + // ── System.Type reflection entry points ────────────────────────────────────────────────── + + [Fact] + public void Rejects_Type_GetType_by_name() + { + var ex = Rejects("var t = System.Type.GetType(\"System.IO.File\"); return t is null ? 0 : 1;"); + ex.Rejections.ShouldContain(r => r.Message.Contains("GetType")); + } + + [Fact] + public void Rejects_Type_GetMethod() + => Rejects("var m = typeof(string).GetMethod(\"Trim\"); return m is null ? 0 : 1;"); + + [Fact] + public void Rejects_Type_GetMethods() + => Rejects("return typeof(string).GetMethods().Length;"); + + [Fact] + public void Rejects_Type_InvokeMember() + => Rejects("""return (int)typeof(string).InvokeMember("Length", System.Reflection.BindingFlags.GetProperty, null, "abc", null)!;"""); + + [Fact] + public void Rejects_Type_GetProperty_GetField_GetMember_GetConstructor_GetEvent() + { + Rejects("return typeof(string).GetProperty(\"Length\") is null ? 0 : 1;"); + Rejects("return typeof(string).GetField(\"Empty\") is null ? 0 : 1;"); + Rejects("return typeof(string).GetMember(\"Length\").Length;"); + Rejects("return typeof(string).GetConstructors().Length;"); + Rejects("return typeof(string).GetEvent(\"x\") is null ? 0 : 1;"); + } + + [Fact] + public async Task Allows_instance_GetType_and_Type_metadata_reads() + { + // object.GetType() is declared on System.Object, and Type.Name / IsValueType are plain metadata + // reads — neither is a reflection entry point, and both are common in authored scripts. + using var ev = ScriptEvaluator.Compile( + "object o = 42; var t = o.GetType(); return t.IsValueType && typeof(int).Name == \"Int32\" ? 1 : 0;"); + (await ev.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken)).ShouldBe(1); + } + + // ── RuntimeHelpers ─────────────────────────────────────────────────────────────────────── + + [Fact] + public void Rejects_RuntimeHelpers() + { + var ex = Rejects( + "var o = System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject(typeof(string)); return o is null ? 0 : 1;"); + ex.Rejections.ShouldContain(r => r.TypeName.Contains("RuntimeHelpers")); + } + + [Fact] + public async Task Allows_compile_time_only_CompilerServices_attributes() + { + // The namespace stays open for the harmless compile-time types (MethodImplOptions etc.). + using var ev = ScriptEvaluator.Compile( + "var o = System.Runtime.CompilerServices.MethodImplOptions.NoInlining; return (int)o;"); + (await ev.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken)).ShouldBe(8); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/DiagnoseTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/DiagnoseTests.cs index 21460637..94784a45 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/DiagnoseTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/ScriptAnalysis/DiagnoseTests.cs @@ -31,6 +31,26 @@ public sealed class DiagnoseTests => Diag("""var c = new System.Net.Http.HttpClient(); return 0;""") .ShouldContain(x => x.Severity == 8); + // Sandbox hardening parity (R05:10): the editor runs the SAME ForbiddenTypeAnalyzer the publish gate + // runs, so every newly-denied form must surface as an OTSCRIPT_FORBIDDEN marker here — editor accepts + // ⇔ publish accepts. One case per deny mechanism (namespace, member, type), plus the allow-list. + [Theory] + [InlineData("""new System.Threading.SemaphoreSlim(0).Wait(); return 0;""")] // System.Threading namespace + [InlineData("""System.Threading.SpinWait.SpinUntil(() => false); return 0;""")] + [InlineData("""var t = System.Type.GetType("System.IO.File"); return t is null ? 0 : 1;""")] // forbidden member + [InlineData("""return typeof(string).GetMethods().Length;""")] + [InlineData("""var o = System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObject(typeof(string)); return 0;""")] // forbidden type + public void Hardened_sandbox_forms_are_flagged_in_the_editor(string code) + => Diag(code).ShouldContain(x => x.Code == "OTSCRIPT_FORBIDDEN" && x.Severity == 8); + + [Theory] + [InlineData("""var t = System.Threading.CancellationToken.None; return t.IsCancellationRequested ? 1 : 0;""")] + [InlineData("""var x = 41; System.Threading.Interlocked.Increment(ref x); return x;""")] + [InlineData("""var x = 42; return System.Threading.Volatile.Read(ref x);""")] + [InlineData("""object o = 42; return o.GetType().IsValueType ? 1 : 0;""")] + public void Allow_listed_forms_are_not_flagged_in_the_editor(string code) + => Diag(code).ShouldBeEmpty(); + [Fact] public void Dynamic_tag_path_is_flagged() => Diag("""var p = "A"; return ctx.GetTag(p).Value;""") .ShouldContain(x => x.Message.Contains("literal"));