fix(scripting): resolve Medium code-review finding (Core.Scripting-003)
Add System.Threading.Tasks to ForbiddenNamespacePrefixes so scripts cannot use Task.Run / Parallel to spawn background work that outlives the per-evaluation timeout. Document the unbounded-memory accepted trade-off and the Task denial rationale in docs/VirtualTags.md (new "Known resource limits" subsection) and cross-reference from docs/ScriptedAlarms.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 9 |
|
||||
| Open findings | 5 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -112,7 +112,7 @@ node form plus over-block guards for allowed generics/`typeof`.
|
||||
| Severity | Medium |
|
||||
| Category | Security |
|
||||
| Location | `TimedScriptEvaluator.cs:9`, `ScriptSandbox.cs:30` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** There is no bound on memory a script may allocate or on the number of
|
||||
threads/tasks a script may spawn. The class docs acknowledge unbounded memory as "a budget
|
||||
@@ -132,7 +132,7 @@ script authoring behind an Admin permission and treat the test-harness preview a
|
||||
control point, or track an explicit v3 issue for out-of-process execution. Record the
|
||||
decision so it is not silently lost.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — added `System.Threading.Tasks` to `ForbiddenNamespacePrefixes` (blocking `Task.Run` / `Parallel` fan-out); documented the unbounded-memory accepted risk and the `Task` denial rationale in `docs/VirtualTags.md` (new "Known resource limits" subsection) and cross-referenced from `docs/ScriptedAlarms.md`.
|
||||
|
||||
### Core.Scripting-004
|
||||
|
||||
@@ -141,7 +141,7 @@ decision so it is not silently lost.
|
||||
| Severity | Medium |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `DependencyExtractor.cs:73` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The walker matches tag-access calls purely by spelling — any
|
||||
`InvocationExpressionSyntax` whose member name is `GetTag` or `SetVirtualTag` is treated as
|
||||
@@ -160,7 +160,7 @@ member-access call to a non-ctx `GetTag` is untested and would be misattributed.
|
||||
(matching the `ScriptGlobals<TContext>.ctx` field name). Add a test for
|
||||
`someOtherObject.GetTag("X")` asserting it is ignored.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — `VisitInvocationExpression` now additionally checks that `member.Expression` is an `IdentifierNameSyntax` with `ValueText == "ctx"` before treating the call as a dependency; test `Ignores_member_access_GetTag_on_non_ctx_receiver` added to `DependencyExtractorTests`.
|
||||
|
||||
### Core.Scripting-005
|
||||
|
||||
@@ -215,7 +215,7 @@ evicted.
|
||||
| Severity | Medium |
|
||||
| Category | Error handling & resilience |
|
||||
| Location | `TimedScriptEvaluator.cs:60` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `RunAsync` wraps the inner run in `Task.Run(...)` and then awaits
|
||||
`WaitAsync(Timeout, ct)`. If the caller-supplied `ct` cancels at roughly the same time the
|
||||
@@ -231,7 +231,7 @@ and throw `OperationCanceledException(ct)` instead of `ScriptTimeoutException` w
|
||||
caller's token is cancelled, so caller cancellation deterministically wins regardless of
|
||||
race ordering.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — in the `catch (TimeoutException)` handler, `ct.IsCancellationRequested` is now checked and `OperationCanceledException(ct)` thrown before `ScriptTimeoutException`, so caller cancellation deterministically wins regardless of race ordering; regression test `Caller_cancellation_wins_even_when_timeout_fires_first` added to `TimedScriptEvaluatorTests`.
|
||||
|
||||
### Core.Scripting-008
|
||||
|
||||
@@ -292,7 +292,7 @@ code.
|
||||
| Severity | Medium |
|
||||
| Category | Testing coverage |
|
||||
| Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxTests.cs:54` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The sandbox-escape test suite covers only the four obvious vectors
|
||||
(File / Http / Process / Reflection) as direct member-access calls. It does not test:
|
||||
@@ -309,7 +309,7 @@ surface.
|
||||
Core.Scripting-002 and every forbidden namespace/member in Core.Scripting-001. Each must
|
||||
assert a `ScriptSandboxViolationException` (or `CompilationErrorException`) at compile.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — added `ScriptSandboxTests` cases for `System.Threading.Thread`, `System.Threading.Tasks.Task.Run`, `System.Runtime.InteropServices.Marshal`, and `Microsoft.Win32.Registry` (the four namespace-deny-list vectors that had no test); the 001/002 vectors (Environment.Exit/FailFast/AppDomain/GC/Activator, typeof, generics, cast, default(T), is/as, array element, declared variable) were already covered by the -001/-002 resolution commits. All 79 tests pass.
|
||||
|
||||
### Core.Scripting-011
|
||||
|
||||
|
||||
@@ -35,7 +35,7 @@ new ScriptedAlarmDefinition(
|
||||
|
||||
## Predicate evaluation
|
||||
|
||||
Alarm predicates reuse the same Roslyn sandbox as virtual tags — `ScriptEvaluator<AlarmPredicateContext, bool>` compiles the source, `TimedScriptEvaluator` wraps it with the configured timeout (default from `TimedScriptEvaluator.DefaultTimeout`), and `DependencyExtractor` statically harvests the tag paths the script reads. The sandbox rules (forbidden types, cancellation, logging sinks) are documented in [VirtualTags.md](VirtualTags.md); ScriptedAlarms does not redefine them.
|
||||
Alarm predicates reuse the same Roslyn sandbox as virtual tags — `ScriptEvaluator<AlarmPredicateContext, bool>` compiles the source, `TimedScriptEvaluator` wraps it with the configured timeout (default from `TimedScriptEvaluator.DefaultTimeout`), and `DependencyExtractor` statically harvests the tag paths the script reads. The sandbox rules (forbidden types, cancellation, logging sinks) are documented in [VirtualTags.md](VirtualTags.md); ScriptedAlarms does not redefine them. The known memory / CPU resource limits are documented there as well.
|
||||
|
||||
`AlarmPredicateContext` (`AlarmPredicateContext.cs`) is the script's `ScriptContext` subclass:
|
||||
|
||||
|
||||
@@ -18,7 +18,13 @@ User scripts are compiled via `Microsoft.CodeAnalysis.CSharp.Scripting` against
|
||||
|
||||
`ScriptSandbox.Build` allow-lists exactly: `System.Private.CoreLib` (primitives + `Math` + `Convert`), `System.Linq`, `Core.Abstractions` (for `DataValueSnapshot` / `DriverDataType`), `Core.Scripting` (for `ScriptContext` + `Deadband`), `Serilog` (for `ILogger`), and the concrete context type's assembly. Pre-imported namespaces: `System`, `System.Linq`, `ZB.MOM.WW.OtOpcUa.Core.Abstractions`, `ZB.MOM.WW.OtOpcUa.Core.Scripting`.
|
||||
|
||||
`ForbiddenTypeAnalyzer.ForbiddenNamespacePrefixes` currently denies `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Thread`, `System.Runtime.InteropServices`, `Microsoft.Win32`. Matching is by prefix against the resolved symbol's containing namespace, so `System.Net` catches `System.Net.Http.HttpClient` and every subnamespace. `System.Environment` is explicitly allowed.
|
||||
`ForbiddenTypeAnalyzer.ForbiddenNamespacePrefixes` currently denies `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Thread`, `System.Threading.Tasks`, `System.Runtime.InteropServices`, `Microsoft.Win32`. Matching is by prefix against the resolved symbol's containing namespace, so `System.Net` catches `System.Net.Http.HttpClient` and every subnamespace. `System.Threading.Tasks` is denied because scripts are synchronous predicates with no legitimate need to start background tasks — a `Task.Run` fan-out would outlive the per-evaluation timeout entirely (Core.Scripting-003). `System.Environment`, `System.AppDomain`, `System.GC`, and `System.Activator` are denied type-granularly via `ForbiddenFullTypeNames` because they live directly in the `System` namespace (which is otherwise allowed for primitives) — `Environment.Exit` / `FailFast` terminate the host process outright (Core.Scripting-001).
|
||||
|
||||
#### Known resource limits (accepted trade-offs)
|
||||
|
||||
The sandbox cannot prevent a script from **allocating unbounded memory**. A script calling `new byte[int.MaxValue]` repeatedly, or accumulating a large LINQ enumeration, can drive the server process to `OutOfMemoryException` before the 250 ms timeout fires. Script authoring is gated behind the Admin permission as the primary control; the test-harness preview (Stream F.4) allows operators to exercise a script before publishing. Out-of-process script execution is a v3 concern.
|
||||
|
||||
Similarly, **`System.Threading.Tasks` is now denied** (Core.Scripting-003), which prevents `Task.Run` / `Parallel` fan-out that would spawn background work outliving the timeout. However, a tight CPU-bound loop still runs on its thread-pool thread after `WaitAsync` returns — see the `TimedScriptEvaluator` remarks for detail. The orphaned thread is reclaimed when the Roslyn runtime eventually returns; in practice the operator fixes the script once the structured timeout warning appears in `scripts-*.log`.
|
||||
|
||||
### Compile cache (`CompiledScriptCache<TContext, TResult>`)
|
||||
|
||||
|
||||
@@ -18,9 +18,12 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// 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.IO</c>, <c>System.Net</c>, <c>System.Diagnostics</c>,
|
||||
/// <c>System.Reflection</c>, <c>System.Threading.Thread</c>,
|
||||
/// <c>System.Runtime.InteropServices</c>.
|
||||
/// <c>System.Threading.Tasks</c> (scripts are synchronous predicates — no
|
||||
/// legitimate need to start background tasks; a <c>Task.Run</c> fan-out outlives
|
||||
/// the evaluation timeout entirely), <c>System.Runtime.InteropServices</c>,
|
||||
/// <c>Microsoft.Win32</c>. (Core.Scripting-003.)
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Deny-list prefix match. <c>System.Net</c> catches <c>System.Net.Http</c>,
|
||||
@@ -60,7 +63,11 @@ public static class ForbiddenTypeAnalyzer
|
||||
"System.Net",
|
||||
"System.Diagnostics", // catches Process, ProcessStartInfo, EventLog, Trace/Debug file sinks
|
||||
"System.Reflection",
|
||||
"System.Threading.Thread", // raw Thread — Tasks stay allowed (different namespace)
|
||||
"System.Threading.Thread", // raw Thread — blocks the thread-pool
|
||||
"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
|
||||
// (Core.Scripting-003).
|
||||
"System.Runtime.InteropServices",
|
||||
"Microsoft.Win32", // registry
|
||||
];
|
||||
|
||||
Reference in New Issue
Block a user