diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ForbiddenApiChecker.cs b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ForbiddenApiChecker.cs index 5d345b48..de112e1c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ForbiddenApiChecker.cs +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ForbiddenApiChecker.cs @@ -1,6 +1,4 @@ -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; +using ZB.MOM.WW.ScadaBridge.ScriptAnalysis; namespace ZB.MOM.WW.ScadaBridge.InboundAPI; @@ -8,89 +6,30 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI; /// InboundAPI-005: Enforces the ScadaBridge script trust model on inbound API method /// scripts before they are compiled into executable handlers. /// -/// The trust model (CLAUDE.md, Akka.NET conventions) forbids scripts from reaching -/// System.IO, System.Diagnostics.Process, System.Threading, -/// System.Reflection, and raw network APIs. Roslyn scripting performs no -/// API allow/deny-listing — restricting default imports is a convenience, not a -/// sandbox — so a script can fully-qualify any referenced type. This static check -/// walks the script syntax tree and rejects any reference to a forbidden namespace, -/// whether reached through a using directive or a fully-qualified name. +/// This class is now a thin shim that delegates to the shared, authoritative +/// implemented in +/// ZB.MOM.WW.ScadaBridge.ScriptAnalysis (M3.4). The unified validator runs +/// both a semantic symbol pass (catching alias / global:: / using static +/// escapes) and the reflection-gateway + dynamic / Activator syntactic +/// hardening that previously lived exclusively in this file. /// /// /// InboundAPI-015: a purely namespace-textual deny-list is bypassable because /// reflection is reachable through members of permitted types that never /// spell a forbidden namespace, e.g. -/// typeof(string).Assembly.GetType("System.IO.File"). The walker therefore -/// also rejects a curated set of reflection-gateway member names (GetType, -/// Assembly, GetMethod, InvokeMember, CreateInstance, …) -/// and the dynamic keyword. This is hardening of a best-effort static check, -/// not a true sandbox — a determined script author may still find -/// a vector the syntax walker cannot see (see the security notes in -/// code-reviews/InboundAPI/findings.md, InboundAPI-015). The check is -/// defence-in-depth; genuine containment needs a runtime boundary (restricted -/// AssemblyLoadContext / curated reference set / out-of-process sandbox). +/// typeof(string).Assembly.GetType("System.IO.File"). The shared validator +/// handles this with both semantic resolution and reflection-gateway member +/// hardening — GetType, Assembly, GetMethod, InvokeMember, +/// CreateInstance, and the dynamic keyword are all rejected. This +/// remains hardening of a best-effort static check, not a true sandbox +/// (see the security notes in code-reviews/InboundAPI/findings.md, +/// InboundAPI-015). The check is defence-in-depth; genuine containment needs a +/// runtime boundary (restricted AssemblyLoadContext / curated reference set / +/// out-of-process sandbox). /// /// public static class ForbiddenApiChecker { - /// - /// Namespace prefixes the trust model forbids. A script segment matches if it is - /// equal to one of these or is a child namespace of it. - /// - private static readonly string[] ForbiddenNamespaces = - { - "System.IO", - "System.Diagnostics", // covers Process - "System.Threading", // Task/Tasks is explicitly re-allowed below - "System.Reflection", - "System.Net", // raw network (Sockets, HttpClient, etc.) - "System.Runtime.InteropServices", - "Microsoft.Win32", - }; - - /// - /// Namespaces that would otherwise be caught by a forbidden prefix but are - /// required for normal async script authoring and carry no host-access risk. - /// - private static readonly string[] AllowedExceptions = - { - "System.Threading.Tasks", - }; - - /// - /// InboundAPI-015: member names that are reflection gateways. Reaching any of - /// these — even off a permitted type such as typeof(string) or a plain - /// string — lets a script escape the namespace deny-list (obtain an - /// arbitrary Type, load an assembly, late-bind a method). They are - /// rejected regardless of the receiver expression. Invoke is deliberately - /// excluded because Action/Func delegate invocation is legitimate; - /// the reflection MethodInfo.Invoke path is already cut off by rejecting - /// the GetMethod/GetConstructor that produces the MethodInfo. - /// - private static readonly HashSet ForbiddenMemberNames = new(StringComparer.Ordinal) - { - "GetType", // object.GetType() / Type.GetType(string) — yields a System.Type - "GetTypeInfo", // -> TypeInfo (reflection) - "Assembly", // Type.Assembly — yields a System.Reflection.Assembly - "Module", // Type.Module / MethodBase.Module - "CreateInstance", // Activator.CreateInstance / Assembly.CreateInstance - "InvokeMember", // Type.InvokeMember — late-bound dispatch - "GetMethod", - "GetMethods", - "GetConstructor", - "GetConstructors", - "GetField", - "GetFields", - "GetProperty", - "GetProperties", - "GetMember", - "GetMembers", - "GetRuntimeMethod", - "GetRuntimeMethods", - "MethodHandle", // RuntimeMethodHandle escape - "TypeHandle", - }; - /// /// Analyses the script source and returns the list of trust-model violations. /// An empty list means the script is acceptable. @@ -102,111 +41,6 @@ public static class ForbiddenApiChecker if (string.IsNullOrWhiteSpace(scriptCode)) return Array.Empty(); - var tree = CSharpSyntaxTree.ParseText( - scriptCode, - new CSharpParseOptions(kind: SourceCodeKind.Script)); - - var walker = new ForbiddenApiWalker(); - walker.Visit(tree.GetRoot()); - return walker.Violations; - } - - private static bool IsForbidden(string dottedName) - { - foreach (var allowed in AllowedExceptions) - { - if (dottedName == allowed || dottedName.StartsWith(allowed + ".", StringComparison.Ordinal)) - return false; - } - - foreach (var forbidden in ForbiddenNamespaces) - { - if (dottedName == forbidden || dottedName.StartsWith(forbidden + ".", StringComparison.Ordinal)) - return true; - } - - return false; - } - - private sealed class ForbiddenApiWalker : CSharpSyntaxWalker - { - private readonly List _violations = new(); - - /// Gets the accumulated list of trust-model violation descriptions. - public IReadOnlyList Violations => _violations; - - /// - public override void VisitUsingDirective(UsingDirectiveSyntax node) - { - if (node.Name is not null && IsForbidden(node.Name.ToString())) - _violations.Add($"forbidden namespace import '{node.Name}'"); - - base.VisitUsingDirective(node); - } - - /// - public override void VisitQualifiedName(QualifiedNameSyntax node) - { - // Check the longest qualified name; do not descend so a single - // System.IO.File reference is reported once, not three times. - var text = node.ToString(); - if (IsForbidden(text)) - { - _violations.Add($"forbidden type reference '{text}'"); - return; - } - - base.VisitQualifiedName(node); - } - - /// - public override void VisitMemberAccessExpression(MemberAccessExpressionSyntax node) - { - // Catches fully-qualified expressions such as System.IO.File.Delete(...). - var text = node.ToString(); - if (IsForbidden(text)) - { - _violations.Add($"forbidden API access '{text}'"); - return; - } - - // InboundAPI-015: reject reflection-gateway members regardless of the - // receiver. typeof(string).Assembly.GetType("System.IO.File") never - // spells a forbidden namespace, but '.Assembly' and '.GetType' do - // appear here as the accessed member name. - var memberName = node.Name.Identifier.ValueText; - if (ForbiddenMemberNames.Contains(memberName)) - { - _violations.Add($"forbidden reflection member access '.{memberName}'"); - // Still descend: the receiver may contain a further violation. - } - - base.VisitMemberAccessExpression(node); - } - - /// - public override void VisitIdentifierName(IdentifierNameSyntax node) - { - // InboundAPI-015: 'dynamic' widens late-bound member access that the - // static walker cannot see through — reject its use outright. The - // 'dynamic' contextual keyword surfaces as an identifier name. - if (node.Identifier.ValueText == "dynamic") - { - _violations.Add("forbidden use of the 'dynamic' keyword"); - return; - } - - // InboundAPI-015: a bare reference to the reflection entry-point types - // (e.g. 'Activator', 'Type') as an identifier. 'Activator' has no - // non-reflection use; flag it. ('Type' as an identifier is too broad - // to flag here — the gateway members above already cut off its use.) - if (node.Identifier.ValueText == "Activator") - { - _violations.Add("forbidden reflection type reference 'Activator'"); - return; - } - - base.VisitIdentifierName(node); - } + return ScriptTrustValidator.FindViolations(scriptCode); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ZB.MOM.WW.ScadaBridge.InboundAPI.csproj b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ZB.MOM.WW.ScadaBridge.InboundAPI.csproj index b6307013..665799b5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ZB.MOM.WW.ScadaBridge.InboundAPI.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.InboundAPI/ZB.MOM.WW.ScadaBridge.InboundAPI.csproj @@ -17,6 +17,7 @@ + diff --git a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ForbiddenApiCheckerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ForbiddenApiCheckerTests.cs index aa9fb536..7cebaf3d 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ForbiddenApiCheckerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/ForbiddenApiCheckerTests.cs @@ -3,9 +3,18 @@ namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests; /// /// InboundAPI-005 / InboundAPI-015: tests for the script-trust-model checker. /// -/// InboundAPI-015 hardens the textual walker against reflection reached through -/// permitted-type members that never spell a forbidden namespace, e.g. -/// typeof(string).Assembly.GetType("System.IO.File"). +/// Since M3.4 delegates to the shared +/// ScriptTrustValidator in ZB.MOM.WW.ScadaBridge.ScriptAnalysis. +/// The unified policy differs from the old local deny-list in one notable way: +/// System.Diagnostics is loosened to System.Diagnostics.Process +/// only — Stopwatch, Debug, and other non-Process Diagnostics +/// types are now ALLOWED. +/// +/// InboundAPI-015 hardening (reflection-gateway / dynamic / Activator) is +/// preserved unchanged via the shared validator. +/// +/// Violation message wording is intentionally not asserted — callers should +/// use .Count / .Any() rather than exact strings. /// public class ForbiddenApiCheckerTests { @@ -26,11 +35,37 @@ public class ForbiddenApiCheckerTests Assert.False(IsRejected(script), script); } + // --- System.Diagnostics loosening: Process is still forbidden; non-Process types + // (Stopwatch, Debug) are now ALLOWED by the unified policy (M3.4). The old + // InboundAPI checker blocked the entire System.Diagnostics namespace. --- + + [Fact] + public void Diagnostics_Process_StillForbidden() + { + // System.Diagnostics.Process is an explicit forbidden scope in ScriptTrustPolicy. + Assert.True(IsRejected("System.Diagnostics.Process.Start(\"/bin/sh\"); return null;")); + } + + [Fact] + public void Diagnostics_Stopwatch_NowAllowed() + { + // System.Diagnostics.Stopwatch is NOT under the forbidden scope + // "System.Diagnostics.Process" — the unified policy only forbids + // System.Diagnostics.Process, not the whole System.Diagnostics namespace. + Assert.False(IsRejected("var sw = new System.Diagnostics.Stopwatch(); sw.Start(); return null;")); + } + + [Fact] + public void Diagnostics_Debug_NowAllowed() + { + // System.Diagnostics.Debug is also not covered by the Process-only scope. + Assert.False(IsRejected("System.Diagnostics.Debug.WriteLine(\"hi\"); return null;")); + } + // --- Baseline: forbidden namespaces (textual) must still be rejected --- [Theory] [InlineData("System.IO.File.Delete(\"/tmp/x\"); return null;")] - [InlineData("System.Diagnostics.Process.Start(\"/bin/sh\"); return null;")] [InlineData("using System.Reflection; return null;")] [InlineData("var s = new System.Net.Sockets.Socket(default, default, default); return null;")] public void ForbiddenNamespace_Rejected(string script) @@ -101,4 +136,17 @@ public class ForbiddenApiCheckerTests // dynamic widens late-bound member access the static walker cannot see through. Assert.True(IsRejected("dynamic d = Parameters; return null;")); } + + // --- FindViolations returns a non-null, non-throwing result for edge inputs --- + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(null!)] + public void EmptyOrWhitespace_ReturnsEmptyList(string? script) + { + var result = ForbiddenApiChecker.FindViolations(script!); + Assert.NotNull(result); + Assert.Empty(result); + } }