From a5d50fc1db2be8981dfe87a26873bff38dfeba25 Mon Sep 17 00:00:00 2001 From: calum Date: Thu, 8 Nov 2018 12:31:42 +0000 Subject: [PATCH 1/2] C#: Handle `in` arguments, and add `AssignableAccess::isInArgument()` predicate. --- .../Semmle.Extraction.CSharp/Entities/Expression.cs | 3 +++ csharp/ql/src/semmle/code/csharp/exprs/Access.qll | 8 ++++++++ .../ql/test/library-tests/csharp7.2/InArguments.expected | 1 + csharp/ql/test/library-tests/csharp7.2/InArguments.ql | 5 +++++ .../test/library-tests/csharp7.2/NumericLiterals.expected | 4 ++-- .../library-tests/csharp7.2/PrivateProtected.expected | 4 ++-- .../test/library-tests/csharp7.2/ReadonlyStructs.expected | 4 ++-- .../library-tests/csharp7.2/RefReadonlyDelegate.expected | 2 +- .../library-tests/csharp7.2/RefReadonlyReturns.expected | 2 +- .../ql/test/library-tests/csharp7.2/RefStructs.expected | 4 ++-- csharp/ql/test/library-tests/csharp7.2/csharp72.cs | 6 ++++++ 11 files changed, 33 insertions(+), 10 deletions(-) create mode 100644 csharp/ql/test/library-tests/csharp7.2/InArguments.expected create mode 100644 csharp/ql/test/library-tests/csharp7.2/InArguments.ql diff --git a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs index 78c4096bf18a..c050d4d83e95 100644 --- a/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs +++ b/csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs @@ -223,6 +223,9 @@ private void PopulateArgument(ArgumentSyntax arg, int child) case SyntaxKind.None: mode = 0; break; + case SyntaxKind.InKeyword: + mode = 3; + break; default: throw new InternalError(arg, "Unknown argument type"); } diff --git a/csharp/ql/src/semmle/code/csharp/exprs/Access.qll b/csharp/ql/src/semmle/code/csharp/exprs/Access.qll index e68af94ab345..b03a5b64c7a9 100644 --- a/csharp/ql/src/semmle/code/csharp/exprs/Access.qll +++ b/csharp/ql/src/semmle/code/csharp/exprs/Access.qll @@ -174,6 +174,14 @@ class AssignableAccess extends Access, @assignable_access_expr { isOutArgument() or isRefArgument() } + + /** + * Holds if this access passes the assignable being accessed as an `in` + * argument in a method call. + */ + predicate isInArgument() { + expr_argument(this, 3) + } } /** diff --git a/csharp/ql/test/library-tests/csharp7.2/InArguments.expected b/csharp/ql/test/library-tests/csharp7.2/InArguments.expected new file mode 100644 index 000000000000..62e4085b707e --- /dev/null +++ b/csharp/ql/test/library-tests/csharp7.2/InArguments.expected @@ -0,0 +1 @@ +| csharp72.cs:18:12:18:12 | access to local variable s | diff --git a/csharp/ql/test/library-tests/csharp7.2/InArguments.ql b/csharp/ql/test/library-tests/csharp7.2/InArguments.ql new file mode 100644 index 000000000000..432893aa863f --- /dev/null +++ b/csharp/ql/test/library-tests/csharp7.2/InArguments.ql @@ -0,0 +1,5 @@ +import csharp + +from AssignableAccess e +where e.isInArgument() +select e diff --git a/csharp/ql/test/library-tests/csharp7.2/NumericLiterals.expected b/csharp/ql/test/library-tests/csharp7.2/NumericLiterals.expected index 12ea4a1f46a9..dcc6936de3f2 100644 --- a/csharp/ql/test/library-tests/csharp7.2/NumericLiterals.expected +++ b/csharp/ql/test/library-tests/csharp7.2/NumericLiterals.expected @@ -1,2 +1,2 @@ -| csharp72.cs:42:23:42:34 | 85 | -| csharp72.cs:47:31:47:31 | 1 | +| csharp72.cs:48:23:48:34 | 85 | +| csharp72.cs:53:31:53:31 | 1 | diff --git a/csharp/ql/test/library-tests/csharp7.2/PrivateProtected.expected b/csharp/ql/test/library-tests/csharp7.2/PrivateProtected.expected index bf125f15a494..4754b369a38e 100644 --- a/csharp/ql/test/library-tests/csharp7.2/PrivateProtected.expected +++ b/csharp/ql/test/library-tests/csharp7.2/PrivateProtected.expected @@ -1,2 +1,2 @@ -| csharp72.cs:47:27:47:27 | X | -| csharp72.cs:49:28:49:28 | F | +| csharp72.cs:53:27:53:27 | X | +| csharp72.cs:55:28:55:28 | F | diff --git a/csharp/ql/test/library-tests/csharp7.2/ReadonlyStructs.expected b/csharp/ql/test/library-tests/csharp7.2/ReadonlyStructs.expected index e1a0e8de30d1..35b88cd6c013 100644 --- a/csharp/ql/test/library-tests/csharp7.2/ReadonlyStructs.expected +++ b/csharp/ql/test/library-tests/csharp7.2/ReadonlyStructs.expected @@ -1,2 +1,2 @@ -| csharp72.cs:28:17:28:30 | ReadonlyStruct | -| csharp72.cs:36:21:36:37 | ReadonlyRefStruct | +| csharp72.cs:34:17:34:30 | ReadonlyStruct | +| csharp72.cs:42:21:42:37 | ReadonlyRefStruct | diff --git a/csharp/ql/test/library-tests/csharp7.2/RefReadonlyDelegate.expected b/csharp/ql/test/library-tests/csharp7.2/RefReadonlyDelegate.expected index af776d5c0a0c..ceb92009b446 100644 --- a/csharp/ql/test/library-tests/csharp7.2/RefReadonlyDelegate.expected +++ b/csharp/ql/test/library-tests/csharp7.2/RefReadonlyDelegate.expected @@ -1 +1 @@ -| csharp72.cs:25:31:25:33 | Del | +| csharp72.cs:31:31:31:33 | Del | diff --git a/csharp/ql/test/library-tests/csharp7.2/RefReadonlyReturns.expected b/csharp/ql/test/library-tests/csharp7.2/RefReadonlyReturns.expected index a65a390c4e68..86bbf1c956cf 100644 --- a/csharp/ql/test/library-tests/csharp7.2/RefReadonlyReturns.expected +++ b/csharp/ql/test/library-tests/csharp7.2/RefReadonlyReturns.expected @@ -1 +1 @@ -| csharp72.cs:20:22:20:22 | F | +| csharp72.cs:26:22:26:22 | F | diff --git a/csharp/ql/test/library-tests/csharp7.2/RefStructs.expected b/csharp/ql/test/library-tests/csharp7.2/RefStructs.expected index b2e1ae5bb4b2..92dd1d5020de 100644 --- a/csharp/ql/test/library-tests/csharp7.2/RefStructs.expected +++ b/csharp/ql/test/library-tests/csharp7.2/RefStructs.expected @@ -1,2 +1,2 @@ -| csharp72.cs:32:12:32:20 | RefStruct | -| csharp72.cs:36:21:36:37 | ReadonlyRefStruct | +| csharp72.cs:38:12:38:20 | RefStruct | +| csharp72.cs:42:21:42:37 | ReadonlyRefStruct | diff --git a/csharp/ql/test/library-tests/csharp7.2/csharp72.cs b/csharp/ql/test/library-tests/csharp7.2/csharp72.cs index b285b2e12421..6844e005decb 100644 --- a/csharp/ql/test/library-tests/csharp7.2/csharp72.cs +++ b/csharp/ql/test/library-tests/csharp7.2/csharp72.cs @@ -11,6 +11,12 @@ struct S void F(in S s) { } + + void CallF() + { + var s = new S(); + F(in s); + } } class RefReadonlyReturns From 9f04ace4aebf55c7129ba53938a945a37d1020ea Mon Sep 17 00:00:00 2001 From: calum Date: Thu, 8 Nov 2018 12:37:22 +0000 Subject: [PATCH 2/2] C#: Update change notes. --- change-notes/1.19/analysis-csharp.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/change-notes/1.19/analysis-csharp.md b/change-notes/1.19/analysis-csharp.md index 148a42d65fd2..86dc2d786fab 100644 --- a/change-notes/1.19/analysis-csharp.md +++ b/change-notes/1.19/analysis-csharp.md @@ -3,7 +3,7 @@ ## General improvements * Control flow graph improvements: - * The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead. +* The control flow graph construction now takes simple Boolean conditions on local scope variables into account. For example, in `if (b) x = 0; if (b) x = 1;`, the control flow graph will reflect that taking the `true` (resp. `false`) branch in the first condition implies taking the same branch in the second condition. In effect, the first assignment to `x` will now be identified as being dead. * Code that is only reachable from a constant failing assertion, such as `Debug.Assert(false)`, is considered to be unreachable. ## New queries @@ -20,7 +20,11 @@ | Cross-site scripting (`cs/web/xss`) | More results | This query now finds cross-site scripting vulnerabilities in ASP.NET Core applications. | | *@name of query (Query ID)*| *Impact on results* | *How/why the query has changed* | +## Changes to code extraction + +* Arguments passed using `in` are now extracted. ## Changes to QL libraries * `getArgument()` on `AccessorCall` has been improved so it now takes tuple assignments into account. For example, the argument for the implicit `value` parameter in the setter of property `P` is `0` in `(P, x) = (0, 1)`. Additionally, the argument for the `value` parameter in compound assignments is now only the expanded value, for example, in `P += 7` the argument is `P + 7` and not `7`. +* The predicate `isInArgument()` has been added to the `AssignableAccess` class. This holds for expressions that are passed as arguments using `in`.