From 6c6d7e4fff33d0158e5ebd63a3e9c39066e8ee9e Mon Sep 17 00:00:00 2001 From: calum Date: Wed, 28 Nov 2018 17:42:08 +0000 Subject: [PATCH 1/3] C#: Fix false-positives in cs/index-out-of-bounds. --- .../Collections/ContainerLengthCmpOffByOne.ql | 59 +++++++++++++++---- .../ContainerLengthCmpOffByOne.cs | 57 +++++++++++++++++- 2 files changed, 105 insertions(+), 11 deletions(-) diff --git a/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql b/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql index 0eb4706d194..4aab5d6fdaf 100644 --- a/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql +++ b/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql @@ -14,16 +14,55 @@ import csharp import semmle.code.csharp.controlflow.Guards +import semmle.code.csharp.commons.ComparisonTest -from RelationalOperation cmp, Variable array, Variable index, ElementAccess ea, VariableAccess indexAccess -// Look for `index <= array.Length` or `array.Length >= index` -where (cmp instanceof GEExpr or cmp instanceof LEExpr) - and cmp.getGreaterOperand() = any(PropertyAccess pa | pa.getQualifier() = array.getAnAccess() and pa.getTarget().hasName("Length")) - and cmp.getLesserOperand() = index.getAnAccess() +/** A comparison of an index variable with the length of an array. */ +class IndexGuard extends Expr { + ComparisonTest ct; + VariableAccess indexAccess; + Variable array; + + IndexGuard() { + ct.getExpr() = this and + ct.getFirstArgument() = indexAccess and + ct.getSecondArgument() = any(PropertyAccess lengthAccess | + lengthAccess.getQualifier() = array.getAnAccess() and + lengthAccess.getTarget().hasName("Length") + ) + } + + /** This comparison applies to array `arr` and index `ind`. */ + predicate controls(Variable arr, Variable ind) { + arr = array and + ind.getAnAccess() = indexAccess + } + + /** This comparison guards `expr`. */ + predicate guards(GuardedExpr expr, boolean condition) { + expr.isGuardedBy(this, indexAccess, condition) + } + + /** This comparison is an incorrect `<=` or equivalent. */ + predicate isIncorrect() { + ct.getComparisonKind().isLessThanEquals() + } +} + +from IndexGuard incorrectGuard, Variable array, Variable index, ElementAccess ea, GuardedExpr indexAccess +where + // Look for `index <= array.Length` or `array.Length >= index` + incorrectGuard.controls(array, index) and + incorrectGuard.isIncorrect() and // Look for `array[index]` - and ea.getQualifier() = array.getAnAccess() - and ea.getIndex(0) = indexAccess - and indexAccess = index.getAnAccess() + ea.getQualifier() = array.getAnAccess() and + ea.getIndex(0) = indexAccess and + indexAccess = index.getAnAccess() and // Where the index access is guarded by the comparison - and indexAccess.(GuardedExpr).isGuardedBy(cmp, index.getAnAccess(), true) -select cmp, "Off-by-one index comparison against length leads to possible out of bounds $@.", ea, ea.toString() + incorrectGuard.guards(indexAccess, true) and + // And there are no other guards + not exists(IndexGuard validGuard | + not validGuard.isIncorrect() and + validGuard.controls(array, index) and + validGuard.guards(indexAccess, _) + ) +select incorrectGuard, "Off-by-one index comparison against length leads to possible out of bounds $@.", ea, ea.toString() 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 390f78613c1..b25145a8cbd 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 @@ -2,7 +2,7 @@ using System; class Test { - static void Main(string[] args) + void Test1(string[] args) { // BAD: Loop upper bound is off-by-one for (int i = 0; i <= args.Length; i++) @@ -34,6 +34,11 @@ class Test { Console.WriteLine(args[j]); } + } + + void Test2(string[] args) + { + int j = 0; // GOOD: Correct terminating value if (args.Length > j) @@ -41,4 +46,54 @@ class Test Console.WriteLine(args[j]); } } + + void Test3(string[] args) + { + // GOOD: Guarded by ternary operator. + for (int i = 0; i <= args.Length; i++) + { + string s = i < args.Length ? args[i] : ""; + } + } + + void Test4(string[] args) + { + int j = 0; + + // GOOD: Guarded by ternary operator. + if( j <= args.Length ) + { + string s = j < args.Length ? args[j] : ""; + } + } + + void Test5(string[] args) + { + // GOOD: A valid test of Length. + for (int i = 0; i != args.Length; i++) + { + Console.WriteLine(args[i]); + } + } + + void Test6(string[] args) + { + int j = 0; + + // GOOD: There is another correct test. + if( j <= args.Length ) + { + if (j == args.Length) return; + Console.WriteLine(args[j]); + } + } + + void Test7(string[] args) + { + // GOOD: Guarded by ||. + for (int i = 0; i <= args.Length; i++) + { + bool b = i == args.Length || args[i] == "x"; + } + } } From f2d7b6ebe9267a7d75c7e2f4027b3341c01d38d6 Mon Sep 17 00:00:00 2001 From: calum Date: Wed, 28 Nov 2018 20:21:34 +0000 Subject: [PATCH 2/3] C#: Change notes. --- change-notes/1.20/analysis-csharp.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 change-notes/1.20/analysis-csharp.md diff --git a/change-notes/1.20/analysis-csharp.md b/change-notes/1.20/analysis-csharp.md new file mode 100644 index 00000000000..2fb90f6cdec --- /dev/null +++ b/change-notes/1.20/analysis-csharp.md @@ -0,0 +1,19 @@ +# Improvements to C# analysis + +## General improvements + +## New queries + +| **Query** | **Tags** | **Purpose** | +|-----------------------------|-----------|--------------------------------------------------------------------| + +## Changes to existing queries + +| *@name of query (Query ID)*| *Impact on results* | *How/why the query has changed* | +| Off-by-one comparison against container length (cs/index-out-of-bounds) | Fewer false positives | Results have been removed when there are additional guards on the index. | + +## Changes to code extraction + +## Changes to QL libraries + +## Changes to the autobuilder From 6a1ab51d66334af2ef634922a6376617bdc28f08 Mon Sep 17 00:00:00 2001 From: calum Date: Thu, 29 Nov 2018 11:39:10 +0000 Subject: [PATCH 3/3] C#: Address review comments. --- .../Collections/ContainerLengthCmpOffByOne.ql | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql b/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql index 4aab5d6fdaf..2cb7103ee0c 100644 --- a/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql +++ b/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql @@ -17,34 +17,32 @@ import semmle.code.csharp.controlflow.Guards import semmle.code.csharp.commons.ComparisonTest /** A comparison of an index variable with the length of an array. */ -class IndexGuard extends Expr { - ComparisonTest ct; +class IndexGuard extends ComparisonTest { VariableAccess indexAccess; Variable array; IndexGuard() { - ct.getExpr() = this and - ct.getFirstArgument() = indexAccess and - ct.getSecondArgument() = any(PropertyAccess lengthAccess | + this.getFirstArgument() = indexAccess and + this.getSecondArgument() = any(PropertyAccess lengthAccess | lengthAccess.getQualifier() = array.getAnAccess() and lengthAccess.getTarget().hasName("Length") ) } - /** This comparison applies to array `arr` and index `ind`. */ + /** Holds if this comparison applies to array `arr` and index `ind`. */ predicate controls(Variable arr, Variable ind) { arr = array and ind.getAnAccess() = indexAccess } - /** This comparison guards `expr`. */ + /** Holds if this comparison guards `expr`. */ predicate guards(GuardedExpr expr, boolean condition) { - expr.isGuardedBy(this, indexAccess, condition) + expr.isGuardedBy(this.getExpr(), indexAccess, condition) } - /** This comparison is an incorrect `<=` or equivalent. */ + /** Holds if this comparison is an incorrect `<=` or equivalent. */ predicate isIncorrect() { - ct.getComparisonKind().isLessThanEquals() + this.getComparisonKind().isLessThanEquals() } }