diff --git a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs index 06f6ee62..d0335f31 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs @@ -38,6 +38,14 @@ namespace ZB.MOM.WW.ScadaBridge.ScriptAnalysis; /// is a true sandbox — this is best-effort defence-in-depth; genuine containment /// needs a runtime boundary. /// +/// +/// +/// A forbidden type reference inside nameof(...) (e.g. +/// nameof(System.IO.File)) is deliberately flagged: this is +/// conservative/fail-safe — nameof is rare in scripts and a script has +/// no business naming a forbidden type even there, so we prefer fail-safe over +/// adding special-case suppression logic to a security trust boundary. +/// /// public static class ScriptTrustValidator { @@ -287,11 +295,18 @@ public static class ScriptTrustValidator var text = StripWhitespace(node.ToString()); // An allowed-exception name (or a strict prefix of one, e.g. - // "System.Threading" under "System.Threading.Tasks") is OK — stop - // descending so the bare forbidden-namespace qualifier of an allowed - // type is not re-examined and falsely flagged. + // "System.Threading" under "System.Threading.Tasks") must NOT be + // reported — but we STILL DESCEND into its children so a forbidden + // type nested inside (e.g. a generic argument under an allowed + // System.Threading.Tasks.TaskCompletionSource) + // is independently visited and flagged by this pass. Suppress the + // outer report, do not stop the walk. (Pass 2 is self-sufficient and + // does not rely on the semantic pass to catch nested forbidden refs.) if (IsAllowedExceptionPrefix(text)) + { + base.VisitQualifiedName(node); return; + } // Check the longest qualified name; do not descend so a single // System.IO.File reference is reported once, not three times. @@ -323,13 +338,21 @@ public static class ScriptTrustValidator // Still descend: the receiver may contain a further violation. } - // An allowed-exception member-access (or a strict prefix of one) is - // OK — stop descending so the bare forbidden-namespace qualifier of - // an allowed type (e.g. the "System.Threading" inside - // "System.Threading.Tasks.Task.Delay") is not re-examined and - // falsely flagged. + // An allowed-exception member-access (or a strict prefix of one) + // must NOT be reported — the bare forbidden-namespace qualifier of an + // allowed type (e.g. the "System.Threading" inside + // "System.Threading.Tasks.Task.Delay") is a false positive. But we + // STILL DESCEND into its children so a forbidden reference nested + // inside (e.g. a System.IO.File access in an allowed + // System.Threading.Tasks.Task.Run(...) lambda body) is independently + // visited and flagged by this pass. Suppress the outer report, do not + // stop the walk. (Pass 2 is self-sufficient — it does not rely on the + // semantic pass to catch nested forbidden refs.) if (IsAllowedExceptionPrefix(text)) + { + base.VisitMemberAccessExpression(node); return; + } // Catches fully-qualified expressions such as System.IO.File.Delete(...). if (IsForbiddenDottedName(text)) @@ -356,8 +379,8 @@ public static class ScriptTrustValidator } // A bare reference to a reflection entry-point type. 'Activator' has - // no non-reflection use. - if (ScriptTrustPolicy.ForbiddenIdentifiers.Contains(text) && text != "dynamic") + // no non-reflection use. ('dynamic' already returned above.) + if (ScriptTrustPolicy.ForbiddenIdentifiers.Contains(text)) { _violations.Add($"forbidden reflection type reference '{text}'"); return; diff --git a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs index ab7bee6c..431b6714 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ScriptAnalysis.Tests/ScriptTrustValidatorTests.cs @@ -100,6 +100,53 @@ public class ScriptTrustValidatorTests Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); } + [Fact] + public void Rejects_ForbiddenIo_NestedInAllowedTaskRunLambda() + { + // A forbidden System.IO reference buried inside an allowed Task.Run lambda. + // The allowed-exception prefix on the outer member access must NOT shadow + // the nested forbidden reference — Pass 2 must descend into the lambda. + var code = "await System.Threading.Tasks.Task.Run(() => System.IO.File.ReadAllText(\"x\"));"; + Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + } + + [Fact] + public void Rejects_ForbiddenMutex_AsGenericArg_UnderAllowedTasksPrefix() + { + // System.Threading.Mutex (forbidden) appears as a generic argument of an + // allowed System.Threading.Tasks.TaskCompletionSource. The allowed + // outer name must not shadow the forbidden generic arg. + var code = "System.Threading.Tasks.TaskCompletionSource tcs = null;"; + Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + } + + [Fact] + public void Rejects_DirectThreadingMutex_NotThreadSleep() + { + // A direct forbidden System.Threading type (not Thread.Sleep) — pins that + // the broad System.Threading deny-list catches more than the one cased test. + var code = "var m = new System.Threading.Mutex();"; + Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + } + + [Fact] + public void Rejects_ForbiddenFileInfo_AsGenericArg() + { + // System.IO.FileInfo (forbidden) as a generic argument of an allowed + // System.Collections.Generic.List. + var code = "System.Collections.Generic.List x = null;"; + Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + } + + [Fact] + public void Rejects_NameOf_ForbiddenType() + { + // Conservative fail-safe: naming a forbidden type inside nameof(...) is + // deliberately flagged (a script has no business naming it even there). + var code = "var s = nameof(System.IO.File);"; + Assert.NotEmpty(ScriptTrustValidator.FindViolations(code)); + } + // ---- Clean (empty violations) ------------------------------------------- [Fact]