From 28aa9b2b3cdac47eb7b039f3af860269fb85b1bf Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 10 Jan 2024 12:54:33 +0000 Subject: [PATCH 1/8] C#: Emulate that some methods don't have a body (so generated summaries will be applied) --- .../dataflow/external-models/ExternalFlow.ql | 12 ++++++++---- .../dataflow/external-models/steps.ql | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql index 5aca9148ff1..bc292493f1e 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql @@ -21,11 +21,15 @@ module TaintConfig implements DataFlow::ConfigSig { module Taint = TaintTracking::Global; /** - * Simulate that methods with summaries are not included in the source code. - * This is relevant for dataflow analysis using summaries tagged as generated. + * Emulate that methods with summaries do not have a body. + * This is relevant for dataflow analysis using summaries with a generated like + * provenance as generated summaries are only applied, if a + * callable does not have a body. */ -private class MyMethod extends Method { - override predicate fromSource() { none() } +private class MixedFlowArgs extends Method { + MixedFlowArgs() { this.hasFullyQualifiedName("My.Qltest", "G", "MixedFlowArgs") } + + override predicate hasBody() { none() } } from Taint::PathNode source, Taint::PathNode sink diff --git a/csharp/ql/test/library-tests/dataflow/external-models/steps.ql b/csharp/ql/test/library-tests/dataflow/external-models/steps.ql index 313f1dc9b7a..59d5c02258f 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/steps.ql +++ b/csharp/ql/test/library-tests/dataflow/external-models/steps.ql @@ -6,6 +6,22 @@ import semmle.code.csharp.dataflow.FlowSummary import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlowDispatch import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl +/** + * Emulate that methods with summaries do not have a body. + * This is relevant for dataflow analysis using summaries with a generated like + * provenance as generated summaries are only applied, if a + * callable does not have a body. + */ +private class StepArgQualGenerated extends Method { + StepArgQualGenerated() { + exists(string name | + this.hasFullyQualifiedName("My.Qltest", "C", name) and name.matches("StepArgQualGenerated%") + ) + } + + override predicate hasBody() { none() } +} + query predicate summaryThroughStep( DataFlow::Node node1, DataFlow::Node node2, boolean preservesValue ) { From 370a32da8bf86f6507c5421cdfd378cad3ee991b Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 8 Jan 2024 12:58:14 +0000 Subject: [PATCH 2/8] Test summary models and neutral models, manual and generated --- .../dataflow/external-models/ExternalFlow.cs | 13 +++++++++ .../external-models/ExternalFlow.expected | 28 +++++++++++++------ .../external-models/ExternalFlow.ext.yml | 9 ++++++ .../dataflow/external-models/ExternalFlow.ql | 7 +++-- .../dataflow/external-models/Steps.cs | 12 +++++++- .../dataflow/external-models/steps.expected | 10 ++++--- .../dataflow/external-models/steps.ext.yml | 10 +++++++ .../dataflow/external-models/C.java | 9 ++++++ .../dataflow/external-models/steps.expected | 2 ++ .../dataflow/external-models/steps.ext.yml | 8 ++++++ 10 files changed, 93 insertions(+), 15 deletions(-) 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 bc49805fc04..9d320b32581 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs @@ -206,12 +206,25 @@ namespace My.Qltest Sink(MixedFlowArgs(null, o2)); } + void M4() + { + var o1 = new object(); + Sink(GeneratedFlowWithGeneratedNeutral(o1)); + + var o2 = new object(); + Sink(GeneratedFlowWithManualNeutral(o2)); // no flow because the modelled method exists has a manual neutral summary model + } + object GeneratedFlow(object o) => throw null; object GeneratedFlowArgs(object o1, object o2) => throw null; object MixedFlowArgs(object o1, object o2) => throw null; + object GeneratedFlowWithGeneratedNeutral(object o) => throw null; + + object GeneratedFlowWithManualNeutral(object o) => throw null; + static void Sink(object o) { } } 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 34849ad7014..72ed5dfa3cf 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected @@ -63,9 +63,13 @@ edges | ExternalFlow.cs:120:18:120:18 | access to local variable b : null [element] : Object | ExternalFlow.cs:120:18:120:21 | access to array element | | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object | | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | -| ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | ExternalFlow.cs:232:21:232:21 | access to local variable h : HC | -| ExternalFlow.cs:232:21:232:21 | access to local variable h : HC | ExternalFlow.cs:232:21:232:39 | call to method ExtensionMethod : HC | -| ExternalFlow.cs:232:21:232:39 | call to method ExtensionMethod : HC | ExternalFlow.cs:233:18:233:18 | access to local variable o | +| ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | ExternalFlow.cs:212:52:212:53 | access to local variable o1 : Object | +| ExternalFlow.cs:212:52:212:53 | access to local variable o1 : Object | ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | +| ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | ExternalFlow.cs:215:49:215:50 | access to local variable o2 : Object | +| ExternalFlow.cs:215:49:215:50 | access to local variable o2 : Object | ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | +| ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | ExternalFlow.cs:245:21:245:21 | access to local variable h : HC | +| ExternalFlow.cs:245:21:245:21 | access to local variable h : HC | ExternalFlow.cs:245:21:245:39 | call to method ExtensionMethod : HC | +| ExternalFlow.cs:245:21:245:39 | call to method ExtensionMethod : HC | ExternalFlow.cs:246:18:246:18 | access to local variable o | nodes | ExternalFlow.cs:9:27:9:38 | object creation of type Object : Object | semmle.label | object creation of type Object : Object | | ExternalFlow.cs:10:18:10:33 | call to method StepArgRes | semmle.label | call to method StepArgRes | @@ -148,10 +152,16 @@ nodes | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object | | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | semmle.label | call to method MixedFlowArgs | | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object | -| ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | semmle.label | object creation of type HC : HC | -| ExternalFlow.cs:232:21:232:21 | access to local variable h : HC | semmle.label | access to local variable h : HC | -| ExternalFlow.cs:232:21:232:39 | call to method ExtensionMethod : HC | semmle.label | call to method ExtensionMethod : HC | -| ExternalFlow.cs:233:18:233:18 | access to local variable o | semmle.label | access to local variable o | +| ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object | +| ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | semmle.label | call to method GeneratedFlowWithGeneratedNeutral | +| ExternalFlow.cs:212:52:212:53 | access to local variable o1 : Object | semmle.label | access to local variable o1 : Object | +| ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object | +| ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | semmle.label | call to method GeneratedFlowWithManualNeutral | +| ExternalFlow.cs:215:49:215:50 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object | +| ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | semmle.label | object creation of type HC : HC | +| ExternalFlow.cs:245:21:245:21 | access to local variable h : HC | semmle.label | access to local variable h : HC | +| ExternalFlow.cs:245:21:245:39 | call to method ExtensionMethod : HC | semmle.label | call to method ExtensionMethod : HC | +| ExternalFlow.cs:246:18:246:18 | access to local variable o | semmle.label | access to local variable o | subpaths #select | ExternalFlow.cs:10:18:10:33 | call to method StepArgRes | ExternalFlow.cs:9:27:9:38 | object creation of type Object : Object | ExternalFlow.cs:10:18:10:33 | call to method StepArgRes | $@ | ExternalFlow.cs:9:27:9:38 | object creation of type Object : Object | object creation of type Object : Object | @@ -175,4 +185,6 @@ subpaths | 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: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:40 | call to method MixedFlowArgs | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | $@ | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | object creation of type Object : Object | -| ExternalFlow.cs:233:18:233:18 | access to local variable o | ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | ExternalFlow.cs:233:18:233:18 | access to local variable o | $@ | ExternalFlow.cs:231:21:231:28 | object creation of type HC : HC | object creation of type HC : HC | +| ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | $@ | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | object creation of type Object : Object | +| ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | $@ | ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | object creation of type Object : Object | +| ExternalFlow.cs:246:18:246:18 | access to local variable o | ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | ExternalFlow.cs:246:18:246:18 | access to local variable o | $@ | ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | object creation of type HC : HC | diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ext.yml b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ext.yml index ff578df868e..fe22fefa319 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ext.yml +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ext.yml @@ -29,4 +29,13 @@ extensions: - ["My.Qltest", "G", false, "GeneratedFlowArgs", "(System.Object,System.Object)", "", "Argument[1]", "ReturnValue", "value", "df-generated"] - ["My.Qltest", "G", false, "MixedFlowArgs", "(System.Object,System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"] - ["My.Qltest", "G", false, "MixedFlowArgs", "(System.Object,System.Object)", "", "Argument[1]", "ReturnValue", "value", "manual"] + - ["My.Qltest", "G", false, "GeneratedFlowWithGeneratedNeutral", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"] + - ["My.Qltest", "G", false, "GeneratedFlowWithManualNeutral", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"] - ["My.Qltest", "HE", false, "ExtensionMethod", "(My.Qltest.HI)", "", "Argument[0]", "ReturnValue", "value", "manual"] + - addsTo: + pack: codeql/csharp-all + extensible: neutralModel + # "namespace", "type", "name", "signature", "kind", "provenance" + data: + - ["My.Qltest", "G", "GeneratedFlowWithGeneratedNeutral", "(System.Object)", "summary", "df-generated"] + - ["My.Qltest", "G", "GeneratedFlowWithManualNeutral", "(System.Object)", "summary", "manual"] diff --git a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql index bc292493f1e..63fbcb935e4 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql @@ -26,8 +26,11 @@ module Taint = TaintTracking::Global; * provenance as generated summaries are only applied, if a * callable does not have a body. */ -private class MixedFlowArgs extends Method { - MixedFlowArgs() { this.hasFullyQualifiedName("My.Qltest", "G", "MixedFlowArgs") } +private class MethodsWithGeneratedModels extends Method { + MethodsWithGeneratedModels() { + this.hasFullyQualifiedName("My.Qltest", "G", + ["MixedFlowArgs", "GeneratedFlowWithGeneratedNeutral", "GeneratedFlowWithManualNeutral"]) + } override predicate hasBody() { none() } } diff --git a/csharp/ql/test/library-tests/dataflow/external-models/Steps.cs b/csharp/ql/test/library-tests/dataflow/external-models/Steps.cs index ca3546a20f9..259e7dd078a 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/Steps.cs +++ b/csharp/ql/test/library-tests/dataflow/external-models/Steps.cs @@ -42,6 +42,12 @@ namespace My.Qltest gen.StepGeneric2(false); new Sub().StepOverride("string"); + + object arg4 = new object(); + this.StepArgQualGenerated(arg4); + + object arg5 = new object(); + this.StepArgQualGeneratedIgnored(arg5); } object StepArgRes(object x) { return null; } @@ -50,6 +56,10 @@ namespace My.Qltest void StepArgQual(object x) { } + void StepArgQualGenerated(object x) { } + + void StepArgQualGeneratedIgnored(object x) { } + object StepQualRes() { return null; } void StepQualArg(object @out) { } @@ -87,4 +97,4 @@ namespace My.Qltest public override string StepOverride(string i) => throw null; } } -} \ No newline at end of file +} diff --git a/csharp/ql/test/library-tests/dataflow/external-models/steps.expected b/csharp/ql/test/library-tests/dataflow/external-models/steps.expected index f73872ed9be..4364ab91c2e 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/steps.expected +++ b/csharp/ql/test/library-tests/dataflow/external-models/steps.expected @@ -11,11 +11,13 @@ summaryThroughStep | Steps.cs:41:29:41:29 | 0 | Steps.cs:41:13:41:30 | call to method StepGeneric | true | | Steps.cs:42:30:42:34 | false | Steps.cs:42:13:42:35 | call to method StepGeneric2 | true | | Steps.cs:44:36:44:43 | "string" | Steps.cs:44:13:44:44 | call to method StepOverride | true | +| Steps.cs:47:39:47:42 | access to local variable arg4 | Steps.cs:47:13:47:16 | [post] this access | false | +| Steps.cs:50:46:50:49 | access to local variable arg5 | Steps.cs:50:13:50:16 | [post] this access | false | summaryGetterStep -| Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:57:13:57:17 | field Field | -| Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:63:13:63:20 | property Property | +| Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:67:13:67:17 | field Field | +| Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:73:13:73:20 | property Property | | Steps.cs:36:13:36:16 | this access | Steps.cs:36:13:36:36 | call to method StepElementGetter | file://:0:0:0:0 | element | summarySetterStep -| Steps.cs:30:34:30:34 | 0 | Steps.cs:30:13:30:16 | [post] this access | Steps.cs:57:13:57:17 | field Field | -| Steps.cs:34:37:34:37 | 0 | Steps.cs:34:13:34:16 | [post] this access | Steps.cs:63:13:63:20 | property Property | +| Steps.cs:30:34:30:34 | 0 | Steps.cs:30:13:30:16 | [post] this access | Steps.cs:67:13:67:17 | field Field | +| Steps.cs:34:37:34:37 | 0 | Steps.cs:34:13:34:16 | [post] this access | Steps.cs:73:13:73:20 | property Property | | Steps.cs:38:36:38:36 | 0 | Steps.cs:38:13:38:16 | [post] this access | file://:0:0:0:0 | element | diff --git a/csharp/ql/test/library-tests/dataflow/external-models/steps.ext.yml b/csharp/ql/test/library-tests/dataflow/external-models/steps.ext.yml index c6cf52c4ae8..5f422e2e532 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/steps.ext.yml +++ b/csharp/ql/test/library-tests/dataflow/external-models/steps.ext.yml @@ -18,3 +18,13 @@ extensions: - ["My.Qltest", "C+Generic", false, "StepGeneric", "(T)", "", "Argument[0]", "ReturnValue", "value", "manual"] - ["My.Qltest", "C+Generic", false, "StepGeneric2", "(S)", "", "Argument[0]", "ReturnValue", "value", "manual"] - ["My.Qltest", "C+Base", true, "StepOverride", "(T)", "", "Argument[0]", "ReturnValue", "value", "manual"] + - ["My.Qltest", "C", false, "StepArgQualGenerated", "(System.Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"] + - ["My.Qltest", "C", false, "StepArgQualGeneratedIgnored", "(System.Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"] + - addsTo: + pack: codeql/csharp-all + extensible: neutralModel + # "namespace", "type", "name", "signature", "kind", "provenance" + data: + - ["My.Qltest", "C", "StepArgQualGenerated", "(System.Object)", "summary", "df-generated"] + - ["My.Qltest", "C", "StepArgQualGeneratedIgnored", "(System.Object)", "summary", "manual"] + diff --git a/java/ql/test/library-tests/dataflow/external-models/C.java b/java/ql/test/library-tests/dataflow/external-models/C.java index b2f35172488..b1c7f2dc85c 100644 --- a/java/ql/test/library-tests/dataflow/external-models/C.java +++ b/java/ql/test/library-tests/dataflow/external-models/C.java @@ -32,6 +32,11 @@ public class C { // The summary for the first parameter is ignored, because it is generated and // because there is hand written summary for the second parameter. stepArgResGeneratedIgnored(arg1, arg2); + + stepArgQualGenerated(arg1); + // The summary for the first parameter is ignored, because it is generated and + // because there is hand written neutral summary model for this callable. + stepArgQualGeneratedIgnored(arg1); } Object stepArgRes(Object x) { return null; } @@ -47,4 +52,8 @@ public class C { Object stepArgResGenerated(Object x) { return null; } Object stepArgResGeneratedIgnored(Object x, Object y) { return null; } + + Object stepArgQualGenerated(Object x) { return null; } + + Object stepArgQualGeneratedIgnored(Object x) { return null; } } diff --git a/java/ql/test/library-tests/dataflow/external-models/steps.expected b/java/ql/test/library-tests/dataflow/external-models/steps.expected index 9f19ff65f9f..7c4218e2a3c 100644 --- a/java/ql/test/library-tests/dataflow/external-models/steps.expected +++ b/java/ql/test/library-tests/dataflow/external-models/steps.expected @@ -10,3 +10,5 @@ invalidModelRow | C.java:24:5:24:23 | this <.method> | C.java:24:17:24:22 | argOut [post update] | | C.java:29:25:29:28 | arg1 | C.java:29:5:29:29 | stepArgResGenerated(...) | | C.java:34:38:34:41 | arg2 | C.java:34:5:34:42 | stepArgResGeneratedIgnored(...) | +| C.java:36:26:36:29 | arg1 | C.java:36:5:36:30 | this <.method> [post update] | +| C.java:39:33:39:36 | arg1 | C.java:39:5:39:37 | this <.method> [post update] | diff --git a/java/ql/test/library-tests/dataflow/external-models/steps.ext.yml b/java/ql/test/library-tests/dataflow/external-models/steps.ext.yml index c6a1fb69d6d..831079dd40b 100644 --- a/java/ql/test/library-tests/dataflow/external-models/steps.ext.yml +++ b/java/ql/test/library-tests/dataflow/external-models/steps.ext.yml @@ -11,3 +11,11 @@ extensions: - ["my.qltest", "C", False, "stepArgResGenerated", "(Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"] - ["my.qltest", "C", False, "stepArgResGeneratedIgnored", "(Object,Object)", "", "Argument[0]", "ReturnValue", "taint", "df-generated"] - ["my.qltest", "C", False, "stepArgResGeneratedIgnored", "(Object,Object)", "", "Argument[1]", "ReturnValue", "taint", "manual"] + - ["my.qltest", "C", False, "stepArgQualGenerated", "(Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"] + - ["my.qltest", "C", False, "stepArgQualGeneratedIgnored", "(Object)", "", "Argument[0]", "Argument[this]", "taint", "df-generated"] + - addsTo: + pack: codeql/java-all + extensible: neutralModel + data: + - ["my.qltest", "C", "stepArgQualGenerated", "(Object)", "summary", "df-generated"] + - ["my.qltest", "C", "stepArgQualGeneratedIgnored", "(Object)", "summary", "manual"] From 52563b01b72bdb86719b1a30e255e44c960d47e2 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 8 Jan 2024 12:17:56 +0000 Subject: [PATCH 3/8] Factor logic out into `interpretNeutral` --- .../code/csharp/dataflow/internal/ExternalFlow.qll | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll index fcd8a91b5e5..167fa78f8a0 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll @@ -529,6 +529,13 @@ private predicate interpretSummary( ) } +private predicate interpretNeutral(UnboundCallable c, string kind, string provenance) { + exists(string namespace, string type, string name, string signature | + neutralModel(namespace, type, name, signature, kind, provenance) and + c = interpretElement(namespace, type, false, name, signature, "") + ) +} + // adapter class for converting Mad summaries to `SummarizedCallable`s private class SummarizedCallableAdapter extends SummarizedCallable { SummarizedCallableAdapter() { interpretSummary(this, _, _, _, _) } @@ -568,12 +575,7 @@ private class NeutralCallableAdapter extends NeutralCallable { string kind; string provenance_; - NeutralCallableAdapter() { - exists(string namespace, string type, string name, string signature | - neutralModel(namespace, type, name, signature, kind, provenance_) and - this = interpretElement(namespace, type, false, name, signature, "") - ) - } + NeutralCallableAdapter() { interpretNeutral(this, kind, provenance_) } override string getKind() { result = kind } From 7824e60acd308b9c95f3ed0d39e186c98127ad03 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Sat, 6 Jan 2024 21:43:43 +0000 Subject: [PATCH 4/8] Manual neutral summaries should block generated summaries --- .../lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll | 4 ++++ java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll index 167fa78f8a0..5c985759b78 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll @@ -551,6 +551,10 @@ private class SummarizedCallableAdapter extends SummarizedCallable { exists(Provenance provenance | interpretSummary(this, input, output, kind, provenance) and provenance.isGenerated() + ) and + not exists(Provenance provenance | + interpretNeutral(this, "summary", provenance) and + provenance.isManual() ) } diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index d2ce3d0a7d6..1666413db76 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -528,6 +528,10 @@ private class SummarizedCallableAdapter extends SummarizedCallable { exists(Provenance provenance | summaryElement(this, input, output, kind, provenance) and provenance.isGenerated() + ) and + not exists(Provenance provenance | + neutralElement(this, "summary", provenance) and + provenance.isManual() ) } From 3767348dec375242b1654657993419a211eedba9 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 8 Jan 2024 12:59:55 +0000 Subject: [PATCH 5/8] Update test expectations --- .../dataflow/external-models/ExternalFlow.expected | 6 ------ .../library-tests/dataflow/external-models/steps.expected | 1 - .../library-tests/dataflow/external-models/steps.expected | 1 - 3 files changed, 8 deletions(-) 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 72ed5dfa3cf..ad549681753 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected @@ -65,8 +65,6 @@ edges | ExternalFlow.cs:206:38:206:39 | access to local variable o2 : Object | ExternalFlow.cs:206:18:206:40 | call to method MixedFlowArgs | | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | ExternalFlow.cs:212:52:212:53 | access to local variable o1 : Object | | ExternalFlow.cs:212:52:212:53 | access to local variable o1 : Object | ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | -| ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | ExternalFlow.cs:215:49:215:50 | access to local variable o2 : Object | -| ExternalFlow.cs:215:49:215:50 | access to local variable o2 : Object | ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | | ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | ExternalFlow.cs:245:21:245:21 | access to local variable h : HC | | ExternalFlow.cs:245:21:245:21 | access to local variable h : HC | ExternalFlow.cs:245:21:245:39 | call to method ExtensionMethod : HC | | ExternalFlow.cs:245:21:245:39 | call to method ExtensionMethod : HC | ExternalFlow.cs:246:18:246:18 | access to local variable o | @@ -155,9 +153,6 @@ nodes | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object | | ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | semmle.label | call to method GeneratedFlowWithGeneratedNeutral | | ExternalFlow.cs:212:52:212:53 | access to local variable o1 : Object | semmle.label | access to local variable o1 : Object | -| ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | semmle.label | object creation of type Object : Object | -| ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | semmle.label | call to method GeneratedFlowWithManualNeutral | -| ExternalFlow.cs:215:49:215:50 | access to local variable o2 : Object | semmle.label | access to local variable o2 : Object | | ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | semmle.label | object creation of type HC : HC | | ExternalFlow.cs:245:21:245:21 | access to local variable h : HC | semmle.label | access to local variable h : HC | | ExternalFlow.cs:245:21:245:39 | call to method ExtensionMethod : HC | semmle.label | call to method ExtensionMethod : HC | @@ -186,5 +181,4 @@ subpaths | 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:40 | call to method MixedFlowArgs | ExternalFlow.cs:205:22:205:33 | object creation of type Object : Object | ExternalFlow.cs:206:18:206:40 | 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:54 | call to method GeneratedFlowWithGeneratedNeutral | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | ExternalFlow.cs:212:18:212:54 | call to method GeneratedFlowWithGeneratedNeutral | $@ | ExternalFlow.cs:211:22:211:33 | object creation of type Object : Object | object creation of type Object : Object | -| ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | ExternalFlow.cs:215:18:215:51 | call to method GeneratedFlowWithManualNeutral | $@ | ExternalFlow.cs:214:22:214:33 | object creation of type Object : Object | object creation of type Object : Object | | ExternalFlow.cs:246:18:246:18 | access to local variable o | ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | ExternalFlow.cs:246:18:246:18 | access to local variable o | $@ | ExternalFlow.cs:244:21:244:28 | object creation of type HC : HC | object creation of type HC : HC | diff --git a/csharp/ql/test/library-tests/dataflow/external-models/steps.expected b/csharp/ql/test/library-tests/dataflow/external-models/steps.expected index 4364ab91c2e..7c228c7817e 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/steps.expected +++ b/csharp/ql/test/library-tests/dataflow/external-models/steps.expected @@ -12,7 +12,6 @@ summaryThroughStep | Steps.cs:42:30:42:34 | false | Steps.cs:42:13:42:35 | call to method StepGeneric2 | true | | Steps.cs:44:36:44:43 | "string" | Steps.cs:44:13:44:44 | call to method StepOverride | true | | Steps.cs:47:39:47:42 | access to local variable arg4 | Steps.cs:47:13:47:16 | [post] this access | false | -| Steps.cs:50:46:50:49 | access to local variable arg5 | Steps.cs:50:13:50:16 | [post] this access | false | summaryGetterStep | Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:67:13:67:17 | field Field | | Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:73:13:73:20 | property Property | diff --git a/java/ql/test/library-tests/dataflow/external-models/steps.expected b/java/ql/test/library-tests/dataflow/external-models/steps.expected index 7c4218e2a3c..c9b2fd4d01e 100644 --- a/java/ql/test/library-tests/dataflow/external-models/steps.expected +++ b/java/ql/test/library-tests/dataflow/external-models/steps.expected @@ -11,4 +11,3 @@ invalidModelRow | C.java:29:25:29:28 | arg1 | C.java:29:5:29:29 | stepArgResGenerated(...) | | C.java:34:38:34:41 | arg2 | C.java:34:5:34:42 | stepArgResGeneratedIgnored(...) | | C.java:36:26:36:29 | arg1 | C.java:36:5:36:30 | this <.method> [post update] | -| C.java:39:33:39:36 | arg1 | C.java:39:5:39:37 | this <.method> [post update] | From def957e8145cbaacd8b2a6e57bd1486094de417d Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Thu, 11 Jan 2024 13:56:27 +0000 Subject: [PATCH 6/8] Accept review suggestion fixing a comment Co-authored-by: Michael Nebel --- .../test/library-tests/dataflow/external-models/ExternalFlow.cs | 2 +- 1 file changed, 1 insertion(+), 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 9d320b32581..1013a31fa50 100644 --- a/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs +++ b/csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs @@ -212,7 +212,7 @@ namespace My.Qltest Sink(GeneratedFlowWithGeneratedNeutral(o1)); var o2 = new object(); - Sink(GeneratedFlowWithManualNeutral(o2)); // no flow because the modelled method exists has a manual neutral summary model + Sink(GeneratedFlowWithManualNeutral(o2)); // no flow because the modelled method has a manual neutral summary model } object GeneratedFlow(object o) => throw null; From 3c369f88bbff6e6245fbb5a99edc0a8aa13a3def Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 11 Jan 2024 14:00:17 +0000 Subject: [PATCH 7/8] Add change notes --- ...2024-01-11-manual-neutral-model-blocks-generated-models.md | 4 ++++ ...2024-01-11-manual-neutral-model-blocks-generated-models.md | 4 ++++ 2 files changed, 8 insertions(+) create mode 100644 csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md create mode 100644 java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md diff --git a/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md b/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md new file mode 100644 index 00000000000..79327dada59 --- /dev/null +++ b/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* A manual neutral model for a callable now blocks and generated summary models for that callable from having any effect. diff --git a/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md b/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md new file mode 100644 index 00000000000..79327dada59 --- /dev/null +++ b/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* A manual neutral model for a callable now blocks and generated summary models for that callable from having any effect. From 5e9ddd8c63c5120204508ea9829a5db98860a96c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Thu, 11 Jan 2024 15:15:21 +0000 Subject: [PATCH 8/8] Apply suggestions from code review on change notes Co-authored-by: Michael Nebel --- .../2024-01-11-manual-neutral-model-blocks-generated-models.md | 2 +- .../2024-01-11-manual-neutral-model-blocks-generated-models.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md b/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md index 79327dada59..bdc5c1b0f2d 100644 --- a/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md +++ b/csharp/ql/lib/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* A manual neutral model for a callable now blocks and generated summary models for that callable from having any effect. +* A manual neutral summary model for a callable now blocks all generated summary models for that callable from having any effect. diff --git a/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md b/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md index 79327dada59..bdc5c1b0f2d 100644 --- a/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md +++ b/java/ql/src/change-notes/2024-01-11-manual-neutral-model-blocks-generated-models.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* A manual neutral model for a callable now blocks and generated summary models for that callable from having any effect. +* A manual neutral summary model for a callable now blocks all generated summary models for that callable from having any effect.