diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md index c4a4c4c..8d2dd0e 100644 --- a/code-reviews/Core.Scripting/findings.md +++ b/code-reviews/Core.Scripting/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-23 | | Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 3 | ## Checklist coverage @@ -375,7 +375,7 @@ Warning event. | Severity | High | | Category | Security | | Location | `ForbiddenTypeAnalyzer.cs:60-76`, `ScriptSandbox.cs:96-126` | -| Status | Open | +| Status | Resolved | **Description:** The Core.Scripting-008 rewrite broadened the BCL references list from a narrow allow-list (`System.Private.CoreLib` + `System.Linq` only) to the @@ -443,7 +443,26 @@ mirroring the Core.Scripting-010 vector style. Update "Sandbox escape" compliance-check row to enumerate the additions, per the Core.Scripting-009 doc-sync convention. -**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ +**Resolution:** Resolved 2026-05-23 — added `System.Runtime.Loader` to +`ForbiddenNamespacePrefixes` (the namespace-prefix form preferred over +type-granular per the recommendation; future BCL additions to that namespace +are denied by default). Added `System.Threading.ThreadPool` and +`System.Threading.Timer` to `ForbiddenFullTypeNames` — both live in +`System.Threading` shared with allowed sync primitives so they must be +type-granular. Regression tests added to `ScriptSandboxTests`: +`Rejects_ThreadPool_QueueUserWorkItem_at_compile`, +`Rejects_Timer_new_at_compile`, `Rejects_AssemblyLoadContext_at_compile`. +`docs/v2/implementation/phase-7-scripting-and-alarming.md` decision #6 + +the Sandbox-escape compliance-check row both updated per the +Core.Scripting-009 doc-sync convention. The two lower-impact suggestions +from the recommendation (`System.Console`, `CultureInfo.DefaultThreadCurrentCulture`) +were intentionally not addressed: `Console.SetOut` requires constructing +a `System.IO.TextWriter` which is already blocked, leaving only +`Console.WriteLine` log-spam (annoyance, not a security threat); and +`CultureInfo.DefaultThreadCurrentCulture` is a cross-script side-effect +worth knowing about but doesn't escape the sandbox. Recording both as +accepted minor risks. Test totals after fix: Core.Scripting 107 green +(was 104 — +3 new rejection tests). ### Core.Scripting-013 diff --git a/code-reviews/README.md b/code-reviews/README.md index c385db6..5c4d1ff 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -20,7 +20,7 @@ Each module's `findings.md` is the source of truth; this file is generated from | [Core.Abstractions](Core.Abstractions/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 | | [Core.AlarmHistorian](Core.AlarmHistorian/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 | | [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 1 | 13 | -| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 4 | 16 | +| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 3 | 16 | | [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Driver.AbCip](Driver.AbCip/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 | | [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 | @@ -48,7 +48,6 @@ Findings with status `Open` or `In Progress`, ordered by severity. | ID | Severity | Category | Location | Description | |---|---|---|---|---| -| Core.Scripting-012 | High | Security | `ForbiddenTypeAnalyzer.cs:60-76`, `ScriptSandbox.cs:96-126` | The Core.Scripting-008 rewrite broadened the BCL references list from a narrow allow-list (`System.Private.CoreLib` + `System.Linq` only) to the full `TRUSTED_PLATFORM_ASSEMBLIES` set filtered to `System.*` + `netstandard` + `Microsoft.Win… | | Core.Scripting-013 | Medium | Security | `ScriptEvaluator.cs:202-225` (`BuildWrapperSource`) | The synthesized wrapper pastes the user's source verbatim between `{` and `}` braces inside a static method body, with a `#line 1` directive and no escaping. The legacy `CSharpScript.CreateDelegate` path was robust to this because Roslyn's… | | Core.Scripting-014 | Medium | Concurrency & thread safety | `CompiledScriptCache.cs:91-103` (`Clear`) | `Clear()` snapshots `_cache.Keys.ToArray()` then iterates, calling `TryRemove(key, out var lazy)` on each — the key-only overload, not the value-scoped one used in `GetOrCompile`'s catch block. Between the snapshot and a given `TryRemove`,… | | Driver.Galaxy-015 | Medium | Security | `libs/MxGateway.Client.dll`, `libs/MxGateway.Contracts.dll`, `libs/README.md` | Commit `994997b` checks in two binary DLLs (`MxGateway.Client.dll`, 99 840 bytes; `MxGateway.Contracts.dll`, 489 984 bytes) under `src/Drivers/.../Driver.Galaxy/libs/` and references them via ``. These are the onl… | @@ -85,6 +84,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.AlarmHistorian-006 | High | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:103,135-216` | | Core.ScriptedAlarms-001 | High | Resolved | Concurrency & thread safety | `ScriptedAlarmEngine.cs:175`, `ScriptedAlarmEngine.cs:178`, `ScriptedAlarmEngine.cs:73`, `ScriptedAlarmEngine.cs:368` | | Core.Scripting-002 | High | Resolved | Security | `ForbiddenTypeAnalyzer.cs:70` | +| Core.Scripting-012 | High | Resolved | Security | `ForbiddenTypeAnalyzer.cs:60-76`, `ScriptSandbox.cs:96-126` | | Core.VirtualTags-001 | High | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:306` | | Driver.AbCip-001 | High | Resolved | Correctness & logic bugs | `AbCipDriver.cs:111`, `AbCipDriver.cs:163-167` | | Driver.AbCip-002 | High | Resolved | Correctness & logic bugs | `AbCipStatusMapper.cs:65-78` | diff --git a/docs/v2/implementation/phase-7-scripting-and-alarming.md b/docs/v2/implementation/phase-7-scripting-and-alarming.md index 8be57e6..c4e6603 100644 --- a/docs/v2/implementation/phase-7-scripting-and-alarming.md +++ b/docs/v2/implementation/phase-7-scripting-and-alarming.md @@ -29,7 +29,7 @@ Tie-in capability — **historian alarm sink**: | 3 | Evaluation trigger = **change-driven + timer-driven**; operator chooses per-tag | Change-driven is cheap at steady state; timer is the escape hatch for polling derivations that don't have a discrete "input changed" signal. | | 4 | Script shape = **Shape A — one script per virtual tag/alarm**; `return` produces the value (or `bool` for alarm condition) | Minimal surface; no predicate/action split. Alarm side-effects (severity, message) configured out-of-band, not in the script. | | 5 | Alarm fidelity = **full OPC UA Part 9** | Uniform with Galaxy + ALMD on the wire; client-side tooling (HMIs, historians, event pipelines) gets one shape. | -| 6 | Sandbox = **read-only context**; scripts can only read any tag + write to virtual tags | Strict Roslyn `ScriptOptions` allow-list. Authoritative deny-list (`ForbiddenTypeAnalyzer`): namespace-prefix deny `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Tasks` (Task / Parallel fan-out — Core.Scripting-003), `System.Runtime.InteropServices`, `Microsoft.Win32`; type-granular deny `System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`, `System.Threading.Thread` (these live directly in the allow-listed `System` / `System.Threading` namespaces, so a prefix rule cannot reach them without blocking primitives — Core.Scripting-001 / -009). | +| 6 | Sandbox = **read-only context**; scripts can only read any tag + write to virtual tags | Strict Roslyn `ScriptOptions` allow-list. Authoritative deny-list (`ForbiddenTypeAnalyzer`): namespace-prefix deny `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Tasks` (Task / Parallel fan-out — Core.Scripting-003), `System.Runtime.InteropServices`, `System.Runtime.Loader` (AssemblyLoadContext et al. — Core.Scripting-012), `Microsoft.Win32`; type-granular deny `System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`, `System.Threading.Thread`, `System.Threading.ThreadPool` (Core.Scripting-012), `System.Threading.Timer` (Core.Scripting-012) (these live directly in the allow-listed `System` / `System.Threading` namespaces, so a prefix rule cannot reach them without blocking primitives — Core.Scripting-001 / -009 / -012). | | 7 | Dependency declaration = **AST inference**; operator doesn't maintain a separate dependency list | `CSharpSyntaxWalker` extracts `ctx.GetTag("path")` string-literal calls at compile time; dynamic paths rejected at publish. | | 8 | Config storage = **config DB with generation-sealed cache** (same as driver instances) | Virtual tags + alarms publish atomically in the same generation as the driver instance config they may depend on. | | 9 | Script return value shape (`ctx.GetTag`) = **`DataValue { Value, StatusCode, Timestamp }`** | Scripts branch on quality naturally without separate `ctx.GetQuality(...)` calls. | @@ -162,7 +162,7 @@ Tie-in capability — **historian alarm sink**: ## Compliance Checks (run at exit gate) -- [ ] **Sandbox escape**: attempts to reference any deny-listed namespace prefix (`System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Tasks`, `System.Runtime.InteropServices`, `Microsoft.Win32`) or any of the type-granular forbidden types (`System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`, `System.Threading.Thread`) fail at script compile with an actionable error. Vectors include direct calls, `typeof(T)`, generic type arguments, casts, `is`/`as` patterns, `default(T)`, array element types, and explicitly-typed local declarations. +- [ ] **Sandbox escape**: attempts to reference any deny-listed namespace prefix (`System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Tasks`, `System.Runtime.InteropServices`, `System.Runtime.Loader`, `Microsoft.Win32`) or any of the type-granular forbidden types (`System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`, `System.Threading.Thread`, `System.Threading.ThreadPool`, `System.Threading.Timer`) fail at script compile with an actionable error. Vectors include direct calls, `typeof(T)`, generic type arguments, casts, `is`/`as` patterns, `default(T)`, array element types, and explicitly-typed local declarations. - [ ] **Dependency inference**: `ctx.GetTag(myStringVar)` (non-literal path) is rejected at publish with a span-pointed error; `ctx.GetTag("Line1/Speed")` is accepted + appears in the inferred input set. - [ ] **Change cascade**: tag A → virtual tag B → virtual tag C. When A changes, B recomputes, then C recomputes. Single change event triggers the full cascade in topological order within one evaluation pass. - [ ] **Cycle rejection**: publish a config where virtual tag B depends on A and A depends on B. Publish fails pre-commit with a clear cycle message. 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 f69eed2..8c735aa 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/ForbiddenTypeAnalyzer.cs @@ -72,6 +72,11 @@ public static class ForbiddenTypeAnalyzer // a Task fan-out outlives the evaluation timeout entirely // (Core.Scripting-003). "System.Runtime.InteropServices", + "System.Runtime.Loader", // AssemblyLoadContext + AssemblyDependencyResolver — + // arbitrary DLL load into the host process + // (Core.Scripting-012). Namespace-prefix rather than + // type-granular so future BCL additions to this + // namespace are denied by default. "Microsoft.Win32", // registry ]; @@ -113,6 +118,24 @@ public static class ForbiddenTypeAnalyzer // target it without blocking those legitimate types. Denied type-granularly here. // (Core.Scripting-010.) "System.Threading.Thread", + // Core.Scripting-012 — broadening the references list to the BCL trusted-platform- + // assemblies set (Core.Scripting-008 follow-up) 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 + // Core.Scripting-003 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", ]; /// 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 fbfae16..2fc94a7 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 @@ -446,4 +446,52 @@ public sealed class ScriptSandboxTests return 0; """)); } + + // --- Core.Scripting-012: BCL refs broadened by -008/-016 re-exposed three vectors + // the original deny-list missed. Each is denied type-granularly in + // ForbiddenTypeAnalyzer.ForbiddenFullTypeNames; these tests pin the rejection. + + [Fact] + public void Rejects_ThreadPool_QueueUserWorkItem_at_compile() + { + // System.Threading.ThreadPool.QueueUserWorkItem re-introduces the background- + // fanout threat Core.Scripting-003 closed against System.Threading.Tasks. The + // ThreadPool type lives in System.Threading (shared with allowed sync primitives + // like CancellationToken), so it must be denied type-granularly. (Core.Scripting-012.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + System.Threading.ThreadPool.QueueUserWorkItem(_ => { }); + return 0; + """)); + } + + [Fact] + public void Rejects_Timer_new_at_compile() + { + // System.Threading.Timer schedules unbounded callback work that outlives the + // per-evaluation timeout. Same namespace as CancellationToken so type-granular. + // (Core.Scripting-012.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var t = new System.Threading.Timer(_ => { }); + return 0; + """)); + } + + [Fact] + public void Rejects_AssemblyLoadContext_at_compile() + { + // System.Runtime.Loader.AssemblyLoadContext loads arbitrary DLLs into the + // process — defense-in-depth gap that the BCL-wide reference set re-opened. + // System.Reflection already denies the typical invocation path, but the + // sandbox boundary stays type-explicit. (Core.Scripting-012.) + Should.Throw(() => + ScriptEvaluator.Compile( + """ + var alc = System.Runtime.Loader.AssemblyLoadContext.Default; + return 0; + """)); + } }