From d8215a35c0268367775c371fe00b1ca0e0cca271 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 18 Jul 2025 13:01:05 +0200 Subject: [PATCH 1/8] C#: Add example of failing taint flow for collections in sinks. --- .../collections/CollectionTaintTracking.cs | 13 +++++++++++++ .../collections/CollectionTaintTracking.expected | 7 +++++++ .../collections/CollectionTaintTracking.ql | 12 ++++++++++++ .../library-tests/tainttracking/collections/options | 2 ++ 4 files changed, 34 insertions(+) create mode 100644 csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs create mode 100644 csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected create mode 100644 csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql create mode 100644 csharp/ql/test/library-tests/tainttracking/collections/options diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs new file mode 100644 index 000000000000..d4177a576616 --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.cs @@ -0,0 +1,13 @@ +public class CollectionTaintTracking +{ + public void ImplicitCollectionReadAtSink() + { + var tainted = Source(1); + var arr = new object[] { tainted }; + Sink(arr); // $ hasTaintFlow=1 + } + + static T Source(object source) => throw null; + + public static void Sink(T t) { } +} diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected new file mode 100644 index 000000000000..57e00d1fd096 --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected @@ -0,0 +1,7 @@ +models +edges +nodes +subpaths +testFailures +| CollectionTaintTracking.cs:10:20:10:38 | // ... | Missing result: hasTaintFlow=1 | +#select diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql new file mode 100644 index 000000000000..0af8971a13b2 --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.ql @@ -0,0 +1,12 @@ +/** + * @kind path-problem + */ + +import csharp +import utils.test.InlineFlowTest +import TaintFlowTest +import PathGraph + +from PathNode source, PathNode sink +where flowPath(source, sink) +select sink, source, sink, "$@", source, source.toString() diff --git a/csharp/ql/test/library-tests/tainttracking/collections/options b/csharp/ql/test/library-tests/tainttracking/collections/options new file mode 100644 index 000000000000..75c39b4541ba --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/options @@ -0,0 +1,2 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj From 81751ea5916fde3388fbb5b93a1465ec44a93efa Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 18 Jul 2025 13:40:56 +0200 Subject: [PATCH 2/8] C#: Allow implicit reads from collections in argument nodes (sinks and additional flow steps) for default taint tracking configurations. --- .../code/csharp/dataflow/internal/TaintTrackingPrivate.qll | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll index b7681994e2c3..908877c359bb 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll @@ -7,6 +7,7 @@ private import semmle.code.csharp.dataflow.internal.DataFlowPrivate private import semmle.code.csharp.dataflow.internal.ControlFlowReachability private import semmle.code.csharp.dispatch.Dispatch private import semmle.code.csharp.commons.ComparisonTest +private import semmle.code.csharp.commons.Collections as Collections // import `TaintedMember` definitions from other files to avoid potential reevaluation private import semmle.code.csharp.frameworks.JsonNET private import semmle.code.csharp.frameworks.WCF @@ -29,7 +30,11 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { * of `c` at sinks and inputs to additional taint steps. */ bindingset[node] -predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { none() } +predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { + node instanceof ArgumentNode and + Collections::isCollectionType(node.getType()) and + c.isElement() +} private class LocalTaintExprStepConfiguration extends ControlFlowReachabilityConfiguration { LocalTaintExprStepConfiguration() { this = "LocalTaintExprStepConfiguration" } From abd0b2e2f9f54590bc4aabeed7a8ed15bc130d4c Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 18 Jul 2025 13:48:12 +0200 Subject: [PATCH 3/8] C#: Update test expected output. --- .../collections/CollectionTaintTracking.expected | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected index 57e00d1fd096..6d93e7f5ef9f 100644 --- a/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected @@ -1,7 +1,18 @@ models edges +| CollectionTaintTracking.cs:5:13:5:19 | access to local variable tainted : Object | CollectionTaintTracking.cs:6:34:6:40 | access to local variable tainted : Object | provenance | | +| CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | CollectionTaintTracking.cs:5:13:5:19 | access to local variable tainted : Object | provenance | | +| CollectionTaintTracking.cs:6:13:6:15 | access to local variable arr : null [element] : Object | CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | provenance | | +| CollectionTaintTracking.cs:6:32:6:42 | { ..., ... } : null [element] : Object | CollectionTaintTracking.cs:6:13:6:15 | access to local variable arr : null [element] : Object | provenance | | +| CollectionTaintTracking.cs:6:34:6:40 | access to local variable tainted : Object | CollectionTaintTracking.cs:6:32:6:42 | { ..., ... } : null [element] : Object | provenance | | nodes +| CollectionTaintTracking.cs:5:13:5:19 | access to local variable tainted : Object | semmle.label | access to local variable tainted : Object | +| CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | semmle.label | call to method Source : Object | +| CollectionTaintTracking.cs:6:13:6:15 | access to local variable arr : null [element] : Object | semmle.label | access to local variable arr : null [element] : Object | +| CollectionTaintTracking.cs:6:32:6:42 | { ..., ... } : null [element] : Object | semmle.label | { ..., ... } : null [element] : Object | +| CollectionTaintTracking.cs:6:34:6:40 | access to local variable tainted : Object | semmle.label | access to local variable tainted : Object | +| CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | semmle.label | access to local variable arr | subpaths testFailures -| CollectionTaintTracking.cs:10:20:10:38 | // ... | Missing result: hasTaintFlow=1 | #select +| CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | CollectionTaintTracking.cs:7:14:7:16 | access to local variable arr | $@ | CollectionTaintTracking.cs:5:23:5:39 | call to method Source : Object | call to method Source : Object | From 1d25a20c9cd81ac73d53723342ff6bb84b9de712 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 18 Jul 2025 15:30:07 +0200 Subject: [PATCH 4/8] C#: Update the external flow test and expected test output. --- .../library-tests/dataflow/external-models/ExternalFlow.cs | 2 +- .../dataflow/external-models/ExternalFlow.expected | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs index 705efd35e38a..d7552376c0f0 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs @@ -116,7 +116,7 @@ void M17() { var a = new object[] { new object() }; var b = Reverse(a); - Sink(b); // No flow + Sink(b); // Flow Sink(b[0]); // Flow } diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected index 7254208be186..3099a3fec7e6 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected @@ -104,6 +104,7 @@ edges | ExternalFlow.cs:117:17:117:17 | access to local variable a : null [element] : Object | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | provenance | | | ExternalFlow.cs:117:34:117:49 | { ..., ... } : null [element] : Object | ExternalFlow.cs:117:17:117:17 | access to local variable a : null [element] : Object | provenance | | | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:117:34:117:49 | { ..., ... } : null [element] : Object | provenance | | +| ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | ExternalFlow.cs:119:18:119:18 | access to local variable b | provenance | | | ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | provenance | | | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | provenance | | | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | provenance | MaD:7 | @@ -240,6 +241,7 @@ nodes | ExternalFlow.cs:118:17:118:17 | access to local variable b : null [element] : Object | semmle.label | access to local variable b : null [element] : Object | | ExternalFlow.cs:118:21:118:30 | call to method Reverse : null [element] : Object | semmle.label | call to method Reverse : null [element] : Object | | ExternalFlow.cs:118:29:118:29 | access to local variable a : null [element] : Object | semmle.label | access to local variable a : null [element] : Object | +| ExternalFlow.cs:119:18:119:18 | access to local variable b | semmle.label | access to local variable b | | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | semmle.label | access to local variable b : null [element] : Object | | ExternalFlow.cs:120:18:120:21 | access to array element | semmle.label | access to array element | | ExternalFlow.cs:205:17:205:18 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object | @@ -315,6 +317,7 @@ invalidModelRow | ExternalFlow.cs:102:22:102:22 | access to parameter d | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:102:22:102:22 | access to parameter d | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:104:18:104:25 | access to field Field | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | ExternalFlow.cs:104:18:104:25 | access to field Field | $@ | ExternalFlow.cs:98:24:98:35 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:112:18:112:25 | access to property MyProp | ExternalFlow.cs:111:24:111:35 | object creation of type Object : Object | ExternalFlow.cs:112:18:112:25 | access to property MyProp | $@ | ExternalFlow.cs:111:24:111:35 | object creation of type Object : Object | object creation of type Object : Object | +| ExternalFlow.cs:119:18:119:18 | access to local variable b | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:119:18:119:18 | access to local variable b | $@ | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:120:18:120:21 | access to array element | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | ExternalFlow.cs:120:18:120:21 | access to array element | $@ | ExternalFlow.cs:117:36:117:47 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:206:18:206:48 | call to method MixedFlowArgs | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:18:206:48 | call to method MixedFlowArgs | $@ | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:212:18:212:62 | call to method GeneratedFlowWithGeneratedNeutral | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | ExternalFlow.cs:212:18:212:62 | call to method GeneratedFlowWithGeneratedNeutral | $@ | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | object creation of type Object : Object | From 7431ee8df9d39c2e1203b3d002e44d9f92fd8dc2 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 18 Jul 2025 15:31:33 +0200 Subject: [PATCH 5/8] C#: Update the barrier in HashWithoutSalt to avoid an FP. It worked by accident before as we didn't allow implicit element reads at sinks. --- .../CWE-759/HashWithoutSalt.ql | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql b/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql index f18798c8b086..f175723c0997 100644 --- a/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql +++ b/csharp/ql/src/experimental/Security Features/CWE-759/HashWithoutSalt.ql @@ -10,6 +10,7 @@ */ import csharp +import semmle.code.csharp.frameworks.system.Collections import HashWithoutSalt::PathGraph /** The C# class `Windows.Security.Cryptography.Core.HashAlgorithmProvider`. */ @@ -93,12 +94,17 @@ predicate hasAnotherHashCall(MethodCall mc) { /** Holds if a password hash without salt is further processed in another method call. */ predicate hasFurtherProcessing(MethodCall mc) { - mc.getTarget().fromLibrary() and - ( - mc.getTarget().hasFullyQualifiedName("System", "Array", "Copy") or // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen); - mc.getTarget().hasFullyQualifiedName("System", "String", "Concat") or // string.Concat(passwordHash, saltkey) - mc.getTarget().hasFullyQualifiedName("System", "Buffer", "BlockCopy") or // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20) - mc.getTarget().hasFullyQualifiedName("System", "String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password) + exists(Method m | m = mc.getTarget() and m.fromLibrary() | + m.hasFullyQualifiedName("System", "Array", "Copy") // Array.Copy(passwordHash, 0, password.Length), 0, key, 0, keyLen); + or + m.hasFullyQualifiedName("System", "String", "Concat") // string.Concat(passwordHash, saltkey) + or + m.hasFullyQualifiedName("System", "Buffer", "BlockCopy") // Buffer.BlockCopy(passwordHash, 0, allBytes, 0, 20) + or + m.hasFullyQualifiedName("System", "String", "Format") // String.Format("{0}:{1}:{2}", username, salt, password) + or + m.getName() = "CopyTo" and + m.getDeclaringType().getABaseType*() instanceof SystemCollectionsICollectionInterface // passBytes.CopyTo(rawSalted, 0); ) } From 4b0c725367414cd61cf28b0f3304bcc37e4f4ef9 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 18 Aug 2025 11:56:53 +0200 Subject: [PATCH 6/8] C#: Add change note. --- .../ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md diff --git a/csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md b/csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md new file mode 100644 index 000000000000..d66e982e6aeb --- /dev/null +++ b/csharp/ql/lib/change-notes/2025-08-18-implicit-reads-at-sinks.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The default taint tracking configuration now allows implicit reads from collections at sinks and in additional flow steps. This increases flow coverage for many taint tracking queries and helps reduce false negatives. From b42c366250be3a2d929e8a78dfc003a8bbf4a942 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 20 Aug 2025 08:50:23 +0200 Subject: [PATCH 7/8] C#: Address review comments. --- .../code/csharp/dataflow/internal/TaintTrackingPrivate.qll | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll index 908877c359bb..dbfda21c09cd 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll @@ -31,8 +31,7 @@ predicate defaultTaintSanitizer(DataFlow::Node node) { */ bindingset[node] predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { - node instanceof ArgumentNode and - Collections::isCollectionType(node.getType()) and + exists(node) and c.isElement() } From ebfbc711046dbdcfa3d275ef71044d2c4185bd0a Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 21 Aug 2025 08:07:17 +0200 Subject: [PATCH 8/8] C#: Address more review comments. --- .../code/csharp/dataflow/internal/TaintTrackingPrivate.qll | 1 - 1 file changed, 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll index dbfda21c09cd..3146720efe86 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll @@ -7,7 +7,6 @@ private import semmle.code.csharp.dataflow.internal.DataFlowPrivate private import semmle.code.csharp.dataflow.internal.ControlFlowReachability private import semmle.code.csharp.dispatch.Dispatch private import semmle.code.csharp.commons.ComparisonTest -private import semmle.code.csharp.commons.Collections as Collections // import `TaintedMember` definitions from other files to avoid potential reevaluation private import semmle.code.csharp.frameworks.JsonNET private import semmle.code.csharp.frameworks.WCF