fix(security): deny Environment/GC and ADO.NET provider namespaces; close reflection-gateway list (GetTypes/EntryPoint/Declared*/DynamicInvoke)

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
2026-07-09 15:38:12 -04:00
parent dbd3ed2fc7
commit d19ab93d39
4 changed files with 80 additions and 3 deletions
@@ -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`
@@ -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",
    ];

    /// <summary>
@@ -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.
    };

    /// <summary>
@@ -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);

@@ -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<int,int> f = x => x; f.Invoke(3);")]
+    public void FindViolations_Allows(string code) => Assert.Empty(ScriptTrustValidator.FindViolations(code));
 }