From 1366638f061bbfe9a28a531a0b5d9c1f383a4dcc Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 12 Dec 2018 13:13:13 +0100 Subject: [PATCH 1/3] C#: Fix whitespaces --- .../Collections/ContainerLengthCmpOffByOne.ql | 6 ++--- .../ql/src/Security Features/CWE-730/ReDoS.ql | 2 +- .../CWE-937/Vulnerabilities.qll | 22 +++++++++---------- .../semmle/code/csharp/controlflow/Guards.qll | 4 ++-- .../library-tests/cil/regressions/Methods.cs | 2 +- .../ContainerLengthCmpOffByOne.cs | 6 ++--- .../Security Features/CWE-937/csproj.config | 4 ++-- .../Security Features/CWE-937/packages.config | 2 +- 8 files changed, 24 insertions(+), 24 deletions(-) diff --git a/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql b/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql index 2cb7103ee0c5..e337aef3b1f7 100644 --- a/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql +++ b/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql @@ -20,10 +20,10 @@ import semmle.code.csharp.commons.ComparisonTest class IndexGuard extends ComparisonTest { VariableAccess indexAccess; Variable array; - + IndexGuard() { this.getFirstArgument() = indexAccess and - this.getSecondArgument() = any(PropertyAccess lengthAccess | + this.getSecondArgument() = any(PropertyAccess lengthAccess | lengthAccess.getQualifier() = array.getAnAccess() and lengthAccess.getTarget().hasName("Length") ) @@ -50,7 +50,7 @@ from IndexGuard incorrectGuard, Variable array, Variable index, ElementAccess ea where // Look for `index <= array.Length` or `array.Length >= index` incorrectGuard.controls(array, index) and - incorrectGuard.isIncorrect() and + incorrectGuard.isIncorrect() and // Look for `array[index]` ea.getQualifier() = array.getAnAccess() and ea.getIndex(0) = indexAccess and diff --git a/csharp/ql/src/Security Features/CWE-730/ReDoS.ql b/csharp/ql/src/Security Features/CWE-730/ReDoS.ql index f1dbe5a068ec..83aab48820ef 100644 --- a/csharp/ql/src/Security Features/CWE-730/ReDoS.ql +++ b/csharp/ql/src/Security Features/CWE-730/ReDoS.ql @@ -19,7 +19,7 @@ from TaintTrackingConfiguration c, DataFlow::PathNode source, DataFlow::PathNode where c.hasFlowPath(source, sink) and // No global timeout set - not exists(RegexGlobalTimeout r) and + not exists(RegexGlobalTimeout r) and ( sink.getNode() instanceof Sink or diff --git a/csharp/ql/src/Security Features/CWE-937/Vulnerabilities.qll b/csharp/ql/src/Security Features/CWE-937/Vulnerabilities.qll index 13e2ea301bbc..b7277850fdf4 100644 --- a/csharp/ql/src/Security Features/CWE-937/Vulnerabilities.qll +++ b/csharp/ql/src/Security Features/CWE-937/Vulnerabilities.qll @@ -1,6 +1,6 @@ /** * Provides a list of NuGet packages with known vulnerabilities. - * + * * To add a new vulnerability follow the existing pattern. * Create a new class that extends the abstract class `Vulnerability`, * supplying the name and the URL, and override one (or both) of @@ -73,9 +73,9 @@ class MicrosoftAdvisory4021279 extends Vulnerability { class CVE_2017_8700 extends Vulnerability { CVE_2017_8700() { this = "CVE-2017-8700" } - + override string getUrl() { result = "https://github.com/aspnet/Announcements/issues/279" } - + override predicate matchesRange(string name, Version affected, Version fixed) { ( name = "Microsoft.AspNetCore.Mvc.Core" @@ -91,9 +91,9 @@ class CVE_2017_8700 extends Vulnerability { class CVE_2018_0765 extends Vulnerability { CVE_2018_0765() { this = "CVE-2018-0765" } - + override string getUrl() { result = "https://github.com/dotnet/announcements/issues/67" } - + override predicate matchesRange(string name, Version affected, Version fixed) { name = "System.Security.Cryptography.Xml" and affected = "0.0.0" and @@ -103,7 +103,7 @@ class CVE_2018_0765 extends Vulnerability { class AspNetCore_Mar18 extends Vulnerability { AspNetCore_Mar18() { this = "ASPNETCore-Mar18" } - + override string getUrl() { result = "https://github.com/aspnet/Announcements/issues/300" } override predicate matchesRange(string name, Version affected, Version fixed) { @@ -125,9 +125,9 @@ class AspNetCore_Mar18 extends Vulnerability { class CVE_2018_8409 extends Vulnerability { CVE_2018_8409() { this = "CVE-2018-8409" } - + override string getUrl() { result = "https://github.com/aspnet/Announcements/issues/316" } - + override predicate matchesRange(string name, Version affected, Version fixed) { name = "System.IO.Pipelines" and affected = "4.5.0" and fixed = "4.5.1" or @@ -138,9 +138,9 @@ class CVE_2018_8409 extends Vulnerability { class CVE_2018_8171 extends Vulnerability { CVE_2018_8171() { this = "CVE-2018-8171" } - + override string getUrl() { result = "https://github.com/aspnet/Announcements/issues/310" } - + override predicate matchesRange(string name, Version affected, Version fixed) { name = "Microsoft.AspNetCore.Identity" and ( affected = "1.0.0" and fixed = "1.0.6" @@ -204,7 +204,7 @@ class CVE_2018_8356 extends Vulnerability { class ASPNETCore_Jul18 extends Vulnerability { ASPNETCore_Jul18() { this = "ASPNETCore-July18" } - + override string getUrl() { result = "https://github.com/aspnet/Announcements/issues/311" } override predicate matchesRange(string name, Version affected, Version fixed) { diff --git a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll index 1a8a097f6cb3..5cf2582f0952 100644 --- a/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll +++ b/csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll @@ -286,7 +286,7 @@ class DereferenceableExpr extends Expr { ie = any(IsTypeExpr ite | ite.getCheckedType() = ite.getExpr().getType()) and branch = false and isNull = true - ) + ) ) or this.hasNullableType() and @@ -1189,7 +1189,7 @@ module Internal { g1 = cond and v1 = v.getDualValue() and ( - // g1 === g2 ? e : ...; + // g1 === g2 ? e : ...; g2 = cond.getCondition() and v2 = TBooleanValue(branch.booleanNot()) or diff --git a/csharp/ql/test/library-tests/cil/regressions/Methods.cs b/csharp/ql/test/library-tests/cil/regressions/Methods.cs index 2d4f07b9855e..b81a5c9a44fa 100644 --- a/csharp/ql/test/library-tests/cil/regressions/Methods.cs +++ b/csharp/ql/test/library-tests/cil/regressions/Methods.cs @@ -3,7 +3,7 @@ * This tests the correct extraction of F, and we should end up with * 2 constructed methods of F. */ - + // semmle-extractor-options: --cil namespace Methods diff --git a/csharp/ql/test/query-tests/Likely Bugs/Collections/ContainerLengthCmpOffByOne/ContainerLengthCmpOffByOne.cs b/csharp/ql/test/query-tests/Likely Bugs/Collections/ContainerLengthCmpOffByOne/ContainerLengthCmpOffByOne.cs index b25145a8cbd8..d68bbfc274ec 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/Collections/ContainerLengthCmpOffByOne/ContainerLengthCmpOffByOne.cs +++ b/csharp/ql/test/query-tests/Likely Bugs/Collections/ContainerLengthCmpOffByOne/ContainerLengthCmpOffByOne.cs @@ -48,7 +48,7 @@ void Test2(string[] args) } void Test3(string[] args) - { + { // GOOD: Guarded by ternary operator. for (int i = 0; i <= args.Length; i++) { @@ -68,7 +68,7 @@ void Test4(string[] args) } void Test5(string[] args) - { + { // GOOD: A valid test of Length. for (int i = 0; i != args.Length; i++) { @@ -94,6 +94,6 @@ void Test7(string[] args) for (int i = 0; i <= args.Length; i++) { bool b = i == args.Length || args[i] == "x"; - } + } } } diff --git a/csharp/ql/test/query-tests/Security Features/CWE-937/csproj.config b/csharp/ql/test/query-tests/Security Features/CWE-937/csproj.config index 721d21559407..690612892147 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-937/csproj.config +++ b/csharp/ql/test/query-tests/Security Features/CWE-937/csproj.config @@ -5,12 +5,12 @@ - + - + diff --git a/csharp/ql/test/query-tests/Security Features/CWE-937/packages.config b/csharp/ql/test/query-tests/Security Features/CWE-937/packages.config index 40e82a77df33..12f63f710432 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-937/packages.config +++ b/csharp/ql/test/query-tests/Security Features/CWE-937/packages.config @@ -4,7 +4,7 @@ - + From 6918dad1dbd53aeb958a227f09160d4e0e430e6f Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 12 Dec 2018 13:14:22 +0100 Subject: [PATCH 2/3] C#: Refactor `localFlowStep()` Using the `forceCachingInSameStage()` trick, we can get rid of the non-cached version of local flow, while still computing it in the same stage. --- .../semmle/code/csharp/dataflow/DataFlow.qll | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/csharp/ql/src/semmle/code/csharp/dataflow/DataFlow.qll b/csharp/ql/src/semmle/code/csharp/dataflow/DataFlow.qll index dfd7885899c5..1e5e6b0353ed 100755 --- a/csharp/ql/src/semmle/code/csharp/dataflow/DataFlow.qll +++ b/csharp/ql/src/semmle/code/csharp/dataflow/DataFlow.qll @@ -90,7 +90,7 @@ module DataFlow { localFlowStep*(source, sink) } - predicate localFlowStep = localFlowStepCached/2; + predicate localFlowStep = Internal::LocalFlow::step/2; /** * A data flow node augmented with a call context and a configuration. Only @@ -690,12 +690,14 @@ module DataFlow { /** * Provides predicates related to local data flow. */ - private module LocalFlow { + module LocalFlow { /** * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local * (intra-procedural) step. */ - predicate localFlowStepNonCached(Node nodeFrom, Node nodeTo) { + cached + predicate step(Node nodeFrom, Node nodeTo) { + forceCachingInSameStage() and localFlowStepExpr(nodeFrom.asExpr(), nodeTo.asExpr()) or // Flow from source to SSA definition @@ -1119,6 +1121,8 @@ module DataFlow { * same stage. */ cached module Cached { + cached predicate forceCachingInSameStage() { any() } + cached newtype TNode = TExprNode(DotNet::Expr e) or @@ -1137,14 +1141,6 @@ module DataFlow { ) } - /** - * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local - * (intra-procedural) step. - */ - cached predicate localFlowStepCached(Node nodeFrom, Node nodeTo) { - LocalFlow::localFlowStepNonCached(nodeFrom, nodeTo) - } - /** * Holds if `pred` can flow to `succ`, by jumping from one callable to * another. From 74167e478a639d209deab026422d791d4b44799c Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 12 Dec 2018 13:16:28 +0100 Subject: [PATCH 3/3] C#: Cache `NamedElement::getLabel()` --- csharp/ql/src/semmle/code/dotnet/Element.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/csharp/ql/src/semmle/code/dotnet/Element.qll b/csharp/ql/src/semmle/code/dotnet/Element.qll index c6d6e7e410c1..739a31ec16bf 100644 --- a/csharp/ql/src/semmle/code/dotnet/Element.qll +++ b/csharp/ql/src/semmle/code/dotnet/Element.qll @@ -80,6 +80,7 @@ class NamedElement extends Element, @dotnet_named_element { } /** Gets a unique string label for this element. */ + cached string getLabel() { none() } /**