diff --git a/docs/requirements/Component-ScriptAnalysis.md b/docs/requirements/Component-ScriptAnalysis.md index 2b25704e..b399a4e6 100644 --- a/docs/requirements/Component-ScriptAnalysis.md +++ b/docs/requirements/Component-ScriptAnalysis.md @@ -38,6 +38,16 @@ The following namespace/type prefixes are forbidden in all scripts: | `System.Net` | Raw network access — forbidden entirely (scripts must use `ExternalSystem.Call`) | | `System.Runtime.InteropServices` | Native interop — forbidden entirely | | `Microsoft.Win32` | Win32 API access — forbidden entirely | +| `System.Environment` | Whole type — forbidden entirely; exposes process control (`Exit`/`FailFast`), the host environment, and secrets via environment variables (e.g. `SCADABRIDGE_API_KEY`). Scripts that need a line break use `"\n"` instead of `Environment.NewLine`. | +| `System.GC` | Whole type — forbidden entirely; GC control (`Collect`, `KeepAlive`, memory-pressure knobs) has no legitimate script use. | +| `Microsoft.Data` | Concrete ADO.NET provider namespace (`Microsoft.Data.SqlClient`, …) — forbidden; closes the arbitrary-host `new SqlConnection("Server=attacker")` channel | +| `System.Data.SqlClient` | Concrete ADO.NET SQL Server provider — forbidden (see provider-namespace posture below) | +| `System.Data.Odbc` | Concrete ADO.NET ODBC provider — forbidden | +| `System.Data.OleDb` | Concrete ADO.NET OLE DB provider — forbidden | + +##### `System.Data` provider-namespace posture + +Only the **concrete ADO.NET provider namespaces** are forbidden — `Microsoft.Data` (which covers `Microsoft.Data.SqlClient`), `System.Data.SqlClient`, `System.Data.Odbc`, and `System.Data.OleDb`. The abstract `System.Data.Common` types (`DbConnection`, `DbCommand`, …) and `System.Data` broadly are **deliberately NOT forbidden**: scripts reach a database only through the sanctioned `Database` helper, whose surface exposes those abstract `System.Data.Common` types. Forbidding the concrete providers blocks a script from constructing its own connection to an arbitrary host (the `new SqlConnection(...)` channel) while leaving the sanctioned `Database` path intact. #### Allowed exceptions within forbidden scopes @@ -47,13 +57,15 @@ The following types are explicitly allowed despite falling within a forbidden na - `System.Threading.CancellationToken` — cooperative cancellation - `System.Threading.CancellationTokenSource` — cooperative cancellation -The scoping rationale: `System.Diagnostics.Process` is the dangerous type (spawns processes); `Stopwatch`, `Debug`, and `Activity` are harmless diagnostic utilities. Forbidding the whole `System.Diagnostics` namespace, as some earlier call sites did, was overly broad. +The scoping rationale: `System.Diagnostics.Process` is the dangerous type (spawns processes); `Stopwatch`, `Debug`, and `Activity` are harmless diagnostic utilities. Forbidding the whole `System.Diagnostics` namespace, as some earlier call sites did, was overly broad. In contrast, `System.Environment` and `System.GC` are forbidden as **whole types** because — unlike `System.Diagnostics` — they have no harmless members a script legitimately needs. #### Reflection-gateway members The following member names are blocked regardless of the receiver type, to prevent reflection-based bypasses such as `typeof(x).Assembly.GetType("System.IO.File")`: -`GetType`, `GetTypeInfo`, `Assembly`, `Module`, `CreateInstance`, `InvokeMember`, `GetMethod`, `GetMethods`, `GetConstructor`, `GetConstructors`, `GetField`, `GetFields`, `GetProperty`, `GetProperties`, `GetMember`, `GetMembers`, `GetRuntimeMethod`, `GetRuntimeMethods`, `MethodHandle`, `TypeHandle`. +`GetType`, `GetTypeInfo`, `Assembly`, `Module`, `CreateInstance`, `InvokeMember`, `GetMethod`, `GetMethods`, `GetConstructor`, `GetConstructors`, `GetField`, `GetFields`, `GetProperty`, `GetProperties`, `GetMember`, `GetMembers`, `GetRuntimeMethod`, `GetRuntimeMethods`, `MethodHandle`, `TypeHandle`, `GetTypes`, `EntryPoint`, `DeclaredMethods`, `DeclaredMembers`, `DeclaredConstructors`, `DynamicInvoke`. + +`Invoke` is **deliberately excluded** from this list: the syntactic pass rejects a gateway member regardless of receiver, and `delegate.Invoke()` (`Func<>`/`Action<>` invocation) is a legitimate, common script pattern. The reflection late-bind `MethodInfo.Invoke` is already caught semantically via the `System.Reflection` scope, so excluding `Invoke` here avoids a false-positive blast radius without weakening the reflection deny-list. #### Forbidden identifiers @@ -77,6 +89,7 @@ The identifiers `dynamic` and `Activator` are forbidden at any scope, as they pr - For each identifier in the syntax tree, resolves the underlying symbol to its fully qualified containing namespace and type name. - Flags any symbol whose containing namespace or type matches a forbidden scope in `ScriptTrustPolicy.ForbiddenScopes`, taking `AllowedExceptions` into account. - Correctly handles aliases (`using X = System.IO.File`), `using static`, and `global::` prefixes — the resolved symbol is checked, not the spelling. +- The validation compilation applies the **same default namespace imports** (`ScriptTrustPolicy.DefaultImports` — `using System;`, `System.Collections.Generic`, `System.Linq`, `System.Threading.Tasks`) that `RoslynScriptCompiler` compiles the real script with. Without them the semantic pass under-resolves **bare** identifiers the running script sees fully bound — e.g. `Environment.Exit(0)` or `GC.Collect()`, whose `Environment`/`GC` resolve to the forbidden `System.Environment`/`System.GC` types only when `using System;` is in scope. Matching the runtime import set closes that blind spot; it can only make more identifiers resolve to their true (forbidden) namespace, never produce a false allow. - Because the full reference set is loaded, this pass also catches a forbidden type accessed inside an otherwise-allowed namespace (e.g., bare `Process` after `using System.Diagnostics;`). ##### `AnalysisReferences` vs `DefaultReferences` diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustPolicy.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustPolicy.cs index 94305474..2a1eb8bb 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustPolicy.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustPolicy.cs @@ -42,6 +42,23 @@ public static class ScriptTrustPolicy "System.Net", "System.Runtime.InteropServices", "Microsoft.Win32", + // Whole type — no legitimate script use. Exposes process control + // (Exit/FailFast), the host environment, and secrets via environment + // variables (SCADABRIDGE_API_KEY). Callers needing a line break use "\n". + "System.Environment", + // Whole type — GC control (Collect, KeepAlive, memory pressure knobs) has + // no legitimate script use. + "System.GC", + // Concrete ADO.NET provider namespaces. Scripts reach a DbConnection ONLY + // via the Database helper, whose abstract System.Data.Common types stay + // allowed; forbidding the concrete providers closes the arbitrary-host + // `new SqlConnection("Server=attacker")` channel. Deliberately NOT + // forbidding System.Data or System.Data.Common broadly — only these + // concrete provider namespaces. + "Microsoft.Data", + "System.Data.SqlClient", + "System.Data.Odbc", + "System.Data.OleDb", ]; /// @@ -85,6 +102,18 @@ public static class ScriptTrustPolicy "GetRuntimeMethods", "MethodHandle", "TypeHandle", + "GetTypes", + "EntryPoint", + "DeclaredMethods", + "DeclaredMembers", + "DeclaredConstructors", + "DynamicInvoke", + // DELIBERATELY EXCLUDED: "Invoke". The syntactic pass rejects a gateway + // member regardless of receiver, and delegate.Invoke() is a legitimate, + // common script pattern (Func<>/Action<> invocation). MethodInfo.Invoke + // (the reflection late-bind) is already caught semantically via the + // System.Reflection scope, so excluding "Invoke" here avoids a + // false-positive blast radius without weakening the reflection deny-list. }; /// diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs index ca165341..e1114440 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs @@ -115,11 +115,21 @@ public static class ScriptTrustValidator if (extraReferences != null) references.AddRange(extraReferences); + // Apply the SAME default namespace imports the site runtime compiles the + // real script with (ScriptTrustPolicy.DefaultImports — "using System;" + // etc.). Without them the semantic pass under-resolves BARE identifiers + // that the running script sees fully bound — e.g. `Environment.Exit(0)` or + // `GC.Collect()`, whose `Environment`/`GC` resolve to the forbidden + // System.Environment/System.GC types only when `using System;` is in + // scope. Matching the runtime's import set closes that blind spot; it can + // only make MORE identifiers resolve to their true (forbidden) namespace, + // never produce a false allow. var compilation = CSharpCompilation.CreateScriptCompilation( "TrustValidation", tree, references, - new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary) + .WithUsings(ScriptTrustPolicy.DefaultImports)); var model = compilation.GetSemanticModel(tree); diff --git a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs index 34ee3a0d..b422cf40 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs @@ -321,4 +321,29 @@ public class ScriptTrustValidatorTests var code = "var s = \"System.IO.File.ReadAllText\";"; Assert.Empty(ScriptTrustValidator.FindViolations(code)); } + + // ---- PLAN-05 Task 21: hardened deny-list additions ----------------------- + + // Environment (whole type), GC, and the concrete ADO.NET provider namespaces + // (Microsoft.Data / System.Data.SqlClient / .Odbc / .OleDb) are now forbidden. + [Theory] + [InlineData("Environment.Exit(0);")] + [InlineData("Environment.FailFast(\"x\");")] + [InlineData("var s = Environment.GetEnvironmentVariable(\"SCADABRIDGE_API_KEY\");")] + [InlineData("var c = new Microsoft.Data.SqlClient.SqlConnection(\"Server=attacker\");")] + public void FindViolations_Flags(string code) => Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + + // Syntactic gateway closure (matters in TPA-degraded fallback mode): these + // reflection-gateway members are rejected regardless of receiver. + [Theory] + [InlineData("typeof(string).Assembly.GetTypes()")] + [InlineData("asm.EntryPoint")] + public void SyntacticPass_Flags(string code) => Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + + // Must NOT regress legitimate scripts: Task.Delay stays allowed and + // delegate.Invoke() is deliberately NOT on the reflection-gateway list. + [Theory] + [InlineData("await Task.Delay(1);")] + [InlineData("Func f = x => x; f.Invoke(3);")] + public void FindViolations_Allows(string code) => Assert.Empty(ScriptTrustValidator.FindViolations(code)); }