diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index 9573755..444e226 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 19 | +| Open findings | 18 | ## Summary @@ -55,7 +55,7 @@ pages and the auth bridge are untested. |--|--| | Severity | Critical | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:171-424` | **Description** @@ -85,7 +85,22 @@ an editor hint. **Resolution** -_Unresolved._ +Resolved 2026-05-16. A Roslyn semantic trust-model gate was added. `RunInSandboxAsync` +now calls `EnforceTrustModel` after compilation and before `script.RunAsync`; if the +script references any forbidden API the run is rejected (`SandboxErrorKind.CompileError`) +with the offending markers, and the same gate is applied to nested shared scripts in +`callSharedFunc`. `FindForbiddenApiUsages` was reworked so it resolves every identifier +(not just the leftmost) against the semantic model and checks types **and** members — +so a fully-qualified call such as `System.IO.File.WriteAllText(...)` is now caught, not +only `using`-directive or bare-type forms. This is a static semantic gate consistent +with the documented trust model; it is not a process sandbox — reflection-based +indirection remains out of its reach, and full isolation would require running scripts +in a separate constrained process (a larger change deliberately not taken here). +Regression tests `RunInSandbox_FullyQualifiedForbiddenApi_IsBlockedBeforeExecution`, +`RunInSandbox_ForbiddenUsingDirective_IsBlockedBeforeExecution` and +`Diagnose_FullyQualifiedForbiddenCall_RaisesSCADA002` fail against the pre-fix code and +pass after; `RunInSandbox_CleanScript_StillRuns` guards against over-blocking. Fixed by +the commit whose message references `CentralUI-001`. ### CentralUI-002 — Site-scoped Deployment permissions are issued but never enforced diff --git a/code-reviews/README.md b/code-reviews/README.md index fc57e07..aa23022 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -34,17 +34,17 @@ resolved and re-triaged. | Severity | Open findings | |----------|---------------| -| Critical | 4 | +| Critical | 3 | | High | 46 | | Medium | 100 | | Low | 89 | -| **Total** | **239** | +| **Total** | **238** | ## Module Status | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| -| [CentralUI](CentralUI/findings.md) | 2026-05-16 | `9c60592` | 1/3/10/5 | 19 | 19 | +| [CentralUI](CentralUI/findings.md) | 2026-05-16 | `9c60592` | 0/3/10/5 | 18 | 19 | | [CLI](CLI/findings.md) | 2026-05-16 | `9c60592` | 0/1/6/6 | 13 | 13 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-05-16 | `9c60592` | 0/1/4/3 | 8 | 8 | | [Commons](Commons/findings.md) | 2026-05-16 | `9c60592` | 0/0/4/8 | 12 | 12 | @@ -71,11 +71,10 @@ Resolved findings drop off this list but remain recorded in their module's `findings.md` (see [REVIEW-PROCESS.md](REVIEW-PROCESS.md) §4–§5). Full detail — description, location, recommendation — lives in the module's `findings.md`. -### Critical (4) +### Critical (3) | ID | Module | Title | |----|--------|-------| -| CentralUI-001 | [CentralUI](CentralUI/findings.md) | Test Run sandbox executes arbitrary C# with no trust-model enforcement | | ExternalSystemGateway-001 | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | No S&F delivery handler registered; cached calls and writes can never be delivered | | NotificationService-001 | [NotificationService](NotificationService/findings.md) | Buffered notifications are never retried (no S&F delivery handler) | | StoreAndForward-001 | [StoreAndForward](StoreAndForward/findings.md) | Replication to standby is never triggered by the active node | diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs index 28bdeac..4176c26 100644 --- a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs @@ -220,6 +220,20 @@ public class ScriptAnalysisService SandboxErrorKind.CompileError, 0, markers); } + // Trust-model gate (CentralUI-001): the documented forbidden-API set is + // enforced HERE, before execution — not merely surfaced as an editor hint. + // Without this, a Design-role user could run arbitrary file/process/ + // reflection/network code in the central host process. + var trustViolations = EnforceTrustModel(script.GetCompilation()); + if (trustViolations.Count > 0) + { + return new SandboxRunResult(false, null, null, "", + "Script blocked by the trust model — it references forbidden APIs " + + "(System.IO, System.Diagnostics, System.Reflection, System.Net, threading). " + + "See the highlighted diagnostics.", + SandboxErrorKind.CompileError, 0, trustViolations); + } + var parameters = ConvertJsonParameters(request.Parameters); using var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(timeoutSeconds)); @@ -311,6 +325,13 @@ public class ScriptAnalysisService throw new ScriptSandboxException( $"Scripts.CallShared(\"{name}\") compile failed: {string.Join("; ", nestedErrors.Select(d => d.GetMessage()))}"); + // Trust-model gate (CentralUI-001) — a nested shared script runs + // arbitrary code too, so it must clear the same forbidden-API gate. + if (EnforceTrustModel(built.GetCompilation()).Count > 0) + throw new ScriptSandboxException( + $"Scripts.CallShared(\"{name}\") is blocked by the script trust model — " + + "the shared script references forbidden APIs."); + lock (compileCacheLock) { if (!compileCache.TryGetValue(name, out compiled)) @@ -1086,15 +1107,25 @@ public class ScriptAnalysisService return new(AttributeContextKind.None, null); } + /// + /// Finds every reference to a forbidden API — the documented script trust model, + /// see . Identifiers are resolved against + /// the semantic model, so a forbidden type or member is caught however it is + /// written: bare (File), fully qualified + /// (System.IO.File.WriteAllText), or via an alias — while a user identifier + /// that merely shares a name with a forbidden type (var File = …) does not + /// false-positive. Used both for editor diagnostics and as the pre-execution + /// trust-model gate (see ). + /// private static IEnumerable FindForbiddenApiUsages(SyntaxTree tree, SemanticModel model) { var root = tree.GetRoot(); - // Banned using directives — pure namespace string match is fine here. + // Banned using directives. foreach (var u in root.DescendantNodes().OfType()) { var name = u.Name?.ToString() ?? ""; - if (ForbiddenNamespacePrefixes.Any(p => name == p || name.StartsWith(p + "."))) + if (IsForbiddenName(name)) { var span = u.GetLocation().GetLineSpan().Span; yield return new DiagnosticMarker( @@ -1108,20 +1139,14 @@ public class ScriptAnalysisService } } - // Banned type usages — resolved via the semantic model so a user - // identifier named "File" or "Thread" does NOT trigger the diagnostic - // unless it actually resolves to a forbidden type. + // Banned type / member references, resolved via the semantic model. Every + // identifier is checked — including the right-hand side of a member access — + // so a fully-qualified forbidden call (System.IO.File.WriteAllText) cannot + // slip past by avoiding a `using` directive or a bare type name. foreach (var ident in root.DescendantNodes().OfType()) { - // Skip the identifier on the right side of a member access — only - // the leftmost (the type or qualifier) is what we want to check. - if (ident.Parent is MemberAccessExpressionSyntax m && m.Name == ident) continue; - - var symbol = model.GetSymbolInfo(ident).Symbol; - if (symbol is not INamedTypeSymbol type) continue; - - var ns = type.ContainingNamespace?.ToDisplayString() ?? ""; - if (!ForbiddenNamespacePrefixes.Any(p => ns == p || ns.StartsWith(p + "."))) continue; + var forbidden = ForbiddenNameFor(model.GetSymbolInfo(ident).Symbol); + if (forbidden == null) continue; var span = ident.GetLocation().GetLineSpan().Span; yield return new DiagnosticMarker( @@ -1130,11 +1155,75 @@ public class ScriptAnalysisService StartColumn: span.Start.Character + 1, EndLineNumber: span.End.Line + 1, EndColumn: span.End.Character + 1, - Message: $"Type '{type.Name}' from forbidden namespace '{ns}' is not allowed in scripts.", + Message: $"'{ident.Identifier.ValueText}' resolves to forbidden API '{forbidden}', " + + "which is not allowed in scripts (script trust model).", Code: "SCADA002"); } } + /// + /// The forbidden namespace/type a symbol implicates, or null if it is allowed. + /// Checks the symbol's namespace and — for a type or member — the type's full + /// name, so an entry like System.Threading.Thread bans that exact type + /// while System.Threading (e.g. CancellationToken) stays allowed. + /// + private static string? ForbiddenNameFor(ISymbol? symbol) + { + if (symbol == null) return null; + foreach (var name in QualifiedNamesOf(symbol)) + if (IsForbiddenName(name)) + return name; + return null; + } + + /// Fully-qualified names a symbol reference implicates for trust-model checking. + private static IEnumerable QualifiedNamesOf(ISymbol symbol) + { + switch (symbol) + { + case INamespaceSymbol { IsGlobalNamespace: false } ns: + yield return ns.ToDisplayString(); + break; + case ITypeSymbol type: + if (type.ContainingNamespace is { IsGlobalNamespace: false } tn) + yield return tn.ToDisplayString(); + yield return FullTypeName(type); + break; + default: + if (symbol.ContainingType is { } ct) + { + if (ct.ContainingNamespace is { IsGlobalNamespace: false } cn) + yield return cn.ToDisplayString(); + yield return FullTypeName(ct); + } + break; + } + } + + private static string FullTypeName(ITypeSymbol type) => + type.ContainingNamespace is { IsGlobalNamespace: false } ns + ? ns.ToDisplayString() + "." + type.Name + : type.Name; + + private static bool IsForbiddenName(string qualifiedName) => + ForbiddenNamespacePrefixes.Any(p => + qualifiedName == p || qualifiedName.StartsWith(p + ".", StringComparison.Ordinal)); + + /// + /// Pre-execution trust-model gate (CentralUI-001). Returns the forbidden-API + /// markers (SCADA001/SCADA002) for a compiled script; an empty list means the + /// script is clear to run. This is a static semantic gate, not a process + /// sandbox — reflection-based indirection is still out of its reach; full + /// isolation would require running scripts in a separate constrained process. + /// + private static IReadOnlyList EnforceTrustModel(Compilation compilation) + { + var tree = compilation.SyntaxTrees.FirstOrDefault(); + if (tree == null) return Array.Empty(); + var model = compilation.GetSemanticModel(tree); + return FindForbiddenApiUsages(tree, model).ToList(); + } + private static CompletionItem ToCompletionItem(ISymbol symbol) { var kind = symbol.Kind switch diff --git a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs index 83ad758..3f4f51f 100644 --- a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs +++ b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -405,4 +405,64 @@ public class ScriptAnalysisServiceTests Assert.Contains("name", resp.Markdown); Assert.Contains("String", resp.Markdown); } + + // ── CentralUI-001: trust-model gate before sandbox execution ────────── + + [Fact] + public void Diagnose_FullyQualifiedForbiddenCall_RaisesSCADA002() + { + // A forbidden API reached by fully-qualified name (no `using`, no bare + // type identifier) must still be flagged — the pre-fix semantic check + // only inspected the leftmost identifier and missed this shape. + var resp = _svc.Diagnose(new DiagnoseRequest( + "var d = System.IO.Directory.GetCurrentDirectory(); return d;")); + Assert.Contains(resp.Markers, m => m.Code == "SCADA002"); + } + + [Fact] + public async Task RunInSandbox_FullyQualifiedForbiddenApi_IsBlockedBeforeExecution() + { + // Regression test for CentralUI-001. RunInSandboxAsync used to execute any + // script that compiled, with no trust-model enforcement — so fully-qualified + // forbidden API code ran in the central host process. The fix gates execution + // on the forbidden-API analysis. + var result = await _svc.RunInSandboxAsync( + new SandboxRunRequest( + "var d = System.IO.Directory.GetCurrentDirectory(); return d;", + Parameters: null, + TimeoutSeconds: null), + CancellationToken.None); + + Assert.False(result.Success); + Assert.Equal(SandboxErrorKind.CompileError, result.ErrorKind); + Assert.Contains("trust model", result.Error); + Assert.NotNull(result.Markers); + Assert.Contains(result.Markers!, m => m.Code is "SCADA001" or "SCADA002"); + } + + [Fact] + public async Task RunInSandbox_ForbiddenUsingDirective_IsBlockedBeforeExecution() + { + var result = await _svc.RunInSandboxAsync( + new SandboxRunRequest( + "using System.Diagnostics; var p = Process.GetCurrentProcess().Id; return p;", + Parameters: null, + TimeoutSeconds: null), + CancellationToken.None); + + Assert.False(result.Success); + Assert.Equal(SandboxErrorKind.CompileError, result.ErrorKind); + } + + [Fact] + public async Task RunInSandbox_CleanScript_StillRuns() + { + // The gate must not block a script that stays within the allowed surface. + var result = await _svc.RunInSandboxAsync( + new SandboxRunRequest("return 21 * 2;", Parameters: null, TimeoutSeconds: null), + CancellationToken.None); + + Assert.True(result.Success); + Assert.Equal("42", result.ReturnValueJson); + } }