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" } 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); ) } 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 | 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..6d93e7f5ef9f --- /dev/null +++ b/csharp/ql/test/library-tests/tainttracking/collections/CollectionTaintTracking.expected @@ -0,0 +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 +#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 | 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