From 38c48a009c51a30ce2e4f794f5981fc567a5cdbf Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:06:56 -0400 Subject: [PATCH] review(Core.Scripting): block Unsafe.As sandbox bypass (Security) Re-review at 7286d320. Core.Scripting-017 (Medium, Security): System.Runtime.CompilerServices.Unsafe added to ForbiddenFullTypeNames (Unsafe.As bypasses the type system without an unsafe context; CWE-843 type-confusion into SetVirtualTag) + regression tests (rejects Unsafe.As, still allows benign CompilerServices attributes). -018: refresh stale rejection message. Sandbox holds. --- code-reviews/Core.Scripting/findings.md | 110 +++++++++++++++++- .../ForbiddenTypeAnalyzer.cs | 32 +++-- .../ScriptSandboxTests.cs | 42 +++++++ 3 files changed, 174 insertions(+), 10 deletions(-) diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md index 28e56339..880ca10f 100644 --- a/code-reviews/Core.Scripting/findings.md +++ b/code-reviews/Core.Scripting/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting` | | Reviewer | Claude Code | -| Review date | 2026-05-23 | -| Commit reviewed | `a9be809` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` (re-review; HEAD at time of fix `8ac5a2db`) | | Status | Reviewed | | Open findings | 0 | @@ -754,3 +754,109 @@ inside two cooperating `[MethodImpl(MethodImplOptions.NoInlining)]` helpers intermediate reflection locals die when each helper returns. Test totals after fix: Core.Scripting 104 green (unchanged); VirtualTags 57 green (was 56 — +1 unload test); ScriptedAlarms 67 green (was 66 — +1 unload test). + +## Re-review 2026-06-19 (commit 7286d320) + +All 16 prior findings remain Resolved. This re-review covers the code added between +commits `a9be809` and `7286d320` (primarily: `ScriptLogTopicSink`, `ScriptRootLogger`, +`ScriptLoggerFactory` identity overload, `ScriptLogCompanionSink` ScriptId fallback, +`PassthroughScript` backslash-exclusion hardening, and the Core.Scripting.Abstractions +refactor that moved `ScriptContext` / `ScriptGlobals` to a Roslyn-free assembly). + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No new issues | +| 2 | OtOpcUa conventions | No new issues | +| 3 | Concurrency & thread safety | No new issues | +| 4 | Error handling & resilience | No new issues | +| 5 | Security | Core.Scripting-017 | +| 6 | Performance & resource management | No new issues | +| 7 | Design-document adherence | No new issues | +| 8 | Code organization & conventions | No new issues | +| 9 | Testing coverage | No new issues | +| 10 | Documentation & comments | Core.Scripting-018 (fixed inline) | + +#### Sandbox re-verification at HEAD + +Re-verified all previously-fixed vectors at HEAD: `System.IO.File`, `HttpClient`, +`Process`, `Reflection`, `Environment.Exit`/`FailFast`, `AppDomain`, `GC`, `Activator`, +`Thread`, `ThreadPool`, `Timer`, `AssemblyLoadContext`, `ThreadPool.QueueUserWorkItem`, +`Timer(callback)`, `typeof(forbidden)`, generic type arguments, casts, `default(T)`, +`is`/`as` patterns, array element types, sibling method / sibling class injection — all +still rejected. New finding: `System.Runtime.CompilerServices.Unsafe` was not blocked +(Core.Scripting-017); fixed in this session. + +### Core.Scripting-017 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Security | +| Location | `ForbiddenTypeAnalyzer.cs:110-153` (`ForbiddenFullTypeNames`) | +| Status | Resolved | + +**Description:** `System.Runtime.CompilerServices.Unsafe` (in +`System.Runtime.CompilerServices.Unsafe.dll`, which starts with `System.` and is +therefore included in the BCL references via `EnumerateBclAssemblyPaths`) was not in +`ForbiddenNamespacePrefixes` or `ForbiddenFullTypeNames`. The namespace +`System.Runtime.CompilerServices` is intentionally not namespace-prefix-denied because +it hosts harmless compile-time-only types (`MethodImplAttribute`, `CallerMemberNameAttribute`, +`DefaultInterpolatedStringHandler`, etc.) that operator scripts may legitimately use. + +However, `Unsafe.As(ref TFrom source)` and `Unsafe.As(object o)` do NOT +require an `unsafe` context at the call site — the `allowUnsafe: false` compile option +only blocks `unsafe {}` blocks and pointer types at the call site, not the `Unsafe` +helper class's managed overloads. A script calling +`System.Runtime.CompilerServices.Unsafe.As(someBoxedObject)` reinterprets the +managed reference bits, bypassing CLR type safety and potentially corrupting managed heap +state in downstream virtual-tag consumers that cast the stored value. + +This is not a traditional RCE / "escape to file-system" vector; the script cannot load +assemblies or spawn processes via `Unsafe` alone. The risk is CWE-843 (type confusion): +a malicious or buggy script could write a type-confused value to `ctx.SetVirtualTag()`, +causing a `ClassCastException` or silent wrong-type storage in the virtual-tag engine, +which in the worst case brings down the tag's evaluation loop or corrupts a downstream +subscriber's reading. Severity is Medium (incorrect/risky behaviour with limited blast +radius vs. a full sandbox escape). + +**Recommendation:** Add `System.Runtime.CompilerServices.Unsafe` to +`ForbiddenFullTypeNames` (type-granular, same pattern as `Thread` / `ThreadPool` / +`Timer`). Add a regression test confirming `Unsafe.As(object)` is rejected, and a +guard test confirming that harmless `CompilerServices` attributes (e.g. `MethodImpl`) +remain usable. + +**Resolution:** Resolved 2026-06-19 — added `System.Runtime.CompilerServices.Unsafe` +to `ForbiddenFullTypeNames` with a detailed comment referencing Core.Scripting-017. +Updated the `ForbiddenFullTypeNames` XML doc remarks to enumerate `ThreadPool`, `Timer`, +and `Unsafe` alongside the existing entries, and updated the summary doc sentence. +The stale rejection message listing only the original four types was replaced (see +Core.Scripting-018). Regression tests `Rejects_Unsafe_As_at_compile` and +`Benign_CompilerServices_attribute_still_usable` added to `ScriptSandboxTests`. Test +totals: Core.Scripting 137 green (was 135, +2 new tests). SHA: TBD. + +### Core.Scripting-018 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `ForbiddenTypeAnalyzer.cs` (`CheckSymbol` rejection message) | +| Status | Resolved | + +**Description:** The rejection message emitted for a type-granular deny-list hit +(a match in `ForbiddenFullTypeNames`) read: _"Scripts cannot reach process-control +types (Environment / AppDomain / GC / Activator) even though they live in the allowed +'System' namespace."_ This list was accurate when written for Core.Scripting-001 but +became stale after Core.Scripting-012 added `Thread`, `ThreadPool`, and `Timer` to +`ForbiddenFullTypeNames`. An operator trying to use `System.Threading.Timer` would +receive a message listing only the original four types, with no mention of `Timer` — +confusing guidance that contradicts the source. + +**Recommendation:** Replace the hard-coded type list in the message with a +forward-pointer to `ForbiddenTypeAnalyzer.ForbiddenFullTypeNames`. + +**Resolution:** Resolved 2026-06-19 — replaced the stale hard-coded list +_"(Environment / AppDomain / GC / Activator)"_ with a generic reference to +`ForbiddenTypeAnalyzer.ForbiddenFullTypeNames`, so the error text stays accurate +regardless of future deny-list additions. No separate test change needed — companion +tests assert on exception type, not message content of this path. SHA: TBD. 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 8bc02a6f..71392f37 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs @@ -82,11 +82,11 @@ public static class ForbiddenTypeAnalyzer /// /// Fully-qualified type names scripts are NOT allowed to reference, regardless of - /// namespace. These types live directly in the allow-listed System - /// namespace (in System.Private.CoreLib), so a namespace-prefix rule cannot - /// reach them without also blocking primitives. Matched by exact fully-qualified - /// name against the resolved type symbol — every member of the type - /// (including read-only ones) is therefore rejected. + /// 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 + /// fully-qualified name against the resolved type symbol — every member of + /// the type (including read-only ones) is therefore rejected. /// /// /// @@ -105,6 +105,14 @@ public static class ForbiddenTypeAnalyzer /// namespace is System.Threading (shared with allowed types like /// CancellationToken), so a namespace-prefix rule cannot reach it /// without blocking unrelated types. (Core.Scripting-010.) + /// System.Threading.ThreadPool / System.Threading.Timer — + /// background-work vectors that outlive the per-evaluation timeout; same + /// threat as Task.Run that Core.Scripting-003 closed. (Core.Scripting-012.) + /// 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. + /// (Core.Scripting-017.) /// /// public static readonly IReadOnlyList ForbiddenFullTypeNames = @@ -136,6 +144,15 @@ public static class ForbiddenTypeAnalyzer // namespace are denied by default. "System.Threading.ThreadPool", "System.Threading.Timer", + // Core.Scripting-017 — 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, calling Unsafe.As(someObject) can corrupt + // managed heap references and cause type-confusion in downstream virtual-tag + // consumers. Type-granular (not namespace-prefix) because System.Runtime.CompilerServices + // also contains harmless compile-time-only types (CallerMemberNameAttribute, + // MethodImplAttribute, MethodImplOptions) that scripts may legitimately reference. + "System.Runtime.CompilerServices.Unsafe", ]; /// @@ -296,9 +313,8 @@ public static class ForbiddenTypeAnalyzer 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.")); + $"deny-list. Scripts cannot reach this type per sandbox rules " + + $"(see ForbiddenTypeAnalyzer.ForbiddenFullTypeNames for the authoritative list).")); return; } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs index 7aecd405..2d6be6d9 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs @@ -569,4 +569,46 @@ public sealed class ScriptSandboxTests """)); ex.Message.ShouldContain("Core.Scripting-013"); } + + // --- Core.Scripting-017: System.Runtime.CompilerServices.Unsafe accessible --- + // System.Runtime.CompilerServices is not in ForbiddenNamespacePrefixes (only .InteropServices + // and .Loader are blocked). Unsafe.As / Unsafe.As(object) allow raw type- + // reinterpretation without unsafe context at the call site — bypassing the CLR's type-safety + // checks and enabling type confusion / managed heap corruption. The fix adds + // System.Runtime.CompilerServices.Unsafe to ForbiddenFullTypeNames (type-granular, shared + // namespace with harmless CompilerServices types like CallerMemberName, MethodImpl). + + /// Verifies that Unsafe.As type-reinterpretation is rejected at compile time (Core.Scripting-017). + [Fact] + public void Rejects_Unsafe_As_at_compile() + { + // System.Runtime.CompilerServices.Unsafe.As(object) bypasses CLR type checks + // entirely, enabling type confusion and managed heap corruption without requiring + // 'unsafe' context at the call site. It must be blocked. (Core.Scripting-017.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var x = ctx.GetTag("X").Value; + var s = System.Runtime.CompilerServices.Unsafe.As(x); + return 0; + """)); + } + + /// Verifies that MethodImplAttribute (a benign CompilerServices type) is still usable (Core.Scripting-017). + [Fact] + public async Task Benign_CompilerServices_attribute_still_usable() + { + // System.Runtime.CompilerServices.MethodImplOptions is a harmless enum used + // with [MethodImpl]. Local functions in scripts can bear it. Denying only + // System.Runtime.CompilerServices.Unsafe (type-granular) must not block this. + var evaluator = ScriptEvaluator.Compile( + """ + [System.Runtime.CompilerServices.MethodImpl( + System.Runtime.CompilerServices.MethodImplOptions.AggressiveInlining)] + static int Helper(int x) => x * 2; + return Helper(21); + """); + var result = await evaluator.RunAsync(new FakeScriptContext(), TestContext.Current.CancellationToken); + result.ShouldBe(42); + } }