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

review(Core.Scripting): block Unsafe.As sandbox bypass (Security)

Browse Source 
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.
This commit is contained in:
Joseph Doherty
2026-06-19 11:06:56 -04:00
parent 65e6af6001
commit 38c48a009c
3 changed files with 174 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Core.Scripting/findings.md
+108 -2
View File
@@ -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<TFrom, TTo>(ref TFrom source)` and `Unsafe.As<T>(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<string>(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<T>(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.
src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs
+24 -8
View File
@@ -82,11 +82,11 @@ public static class ForbiddenTypeAnalyzer
/// <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.
/// namespace. Used when a dangerous type shares its namespace with legitimate types
/// (e.g. <c>System.Threading</c> hosts both <c>CancellationToken</c> and
/// <c>Thread</c>) so a namespace-prefix rule would over-block. 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">
@@ -105,6 +105,14 @@ public static class ForbiddenTypeAnalyzer
/// namespace is <c>System.Threading</c> (shared with allowed types like
/// <c>CancellationToken</c>), so a namespace-prefix rule cannot reach it
/// without blocking unrelated types. (Core.Scripting-010.)</item>
/// <item><c>System.Threading.ThreadPool</c> / <c>System.Threading.Timer</c> —
/// background-work vectors that outlive the per-evaluation timeout; same
/// threat as <c>Task.Run</c> that Core.Scripting-003 closed. (Core.Scripting-012.)</item>
/// <item><c>System.Runtime.CompilerServices.Unsafe</c> — exposes raw type-
/// reinterpretation (<c>As&lt;TFrom,TTo&gt;</c>, <c>As&lt;T&gt;(object)</c>)
/// that bypasses the CLR type system without requiring an <c>unsafe</c> context at
/// the call site; enables type-confusion and managed heap corruption.
/// (Core.Scripting-017.)</item>
/// </list>
/// </remarks>
public static readonly IReadOnlyList<string> 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<TFrom, TTo>, Unsafe.As<T>(object)) that
// bypasses the CLR type system without requiring an 'unsafe' compilation context
// at the call site. In a script, calling Unsafe.As<string>(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",
];
/// <summary>
@@ -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;
}
}
tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs
+42
View File
@@ -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<TFrom, TTo> / Unsafe.As<T>(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).
/// <summary>Verifies that Unsafe.As type-reinterpretation is rejected at compile time (Core.Scripting-017).</summary>
[Fact]
public void Rejects_Unsafe_As_at_compile()
{
// System.Runtime.CompilerServices.Unsafe.As<T>(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<ScriptSandboxViolationException>(() =>
ScriptEvaluator<FakeScriptContext, int>.Compile(
"""
var x = ctx.GetTag("X").Value;
var s = System.Runtime.CompilerServices.Unsafe.As<string>(x);
return 0;
"""));
}
/// <summary>Verifies that MethodImplAttribute (a benign CompilerServices type) is still usable (Core.Scripting-017).</summary>
[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<FakeScriptContext, int>.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);
}
}