From 81de9d35af13c0140c9dc4e56807ba16a95af227 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 20 Dec 2023 15:18:42 +0100 Subject: [PATCH 1/7] C#/Java: Don't generate models if there exist a manual summary or neutral summary. --- .../CaptureDiscardedSummaryModels.ql | 15 --------------- .../utils/modelgenerator/CaptureNeutralModels.ql | 4 +--- .../utils/modelgenerator/CaptureSummaryModels.ql | 4 +--- .../internal/CaptureModelsSpecific.qll | 6 +++++- .../internal/CaptureModelsSpecific.qll | 7 ++++++- 5 files changed, 13 insertions(+), 23 deletions(-) delete mode 100644 csharp/ql/src/utils/modelgenerator/CaptureDiscardedSummaryModels.ql diff --git a/csharp/ql/src/utils/modelgenerator/CaptureDiscardedSummaryModels.ql b/csharp/ql/src/utils/modelgenerator/CaptureDiscardedSummaryModels.ql deleted file mode 100644 index bbd6e887643f..000000000000 --- a/csharp/ql/src/utils/modelgenerator/CaptureDiscardedSummaryModels.ql +++ /dev/null @@ -1,15 +0,0 @@ -/** - * @name Capture discarded summary models. - * @description Finds summary models that are discarded as handwritten counterparts exist. - * @id cs/utils/modelgenerator/discarded-summary-models - */ - -import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl -import internal.CaptureModels -import internal.CaptureSummaryFlowQuery - -from DataFlowTargetApi api, string flow -where - flow = captureFlow(api) and - api.(FlowSummaryImpl::Public::SummarizedCallable).applyManualModel() -select flow order by flow diff --git a/csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql b/csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql index 5b41c5f26c7f..574d93c8a614 100644 --- a/csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql +++ b/csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql @@ -11,7 +11,5 @@ import internal.CaptureModels import internal.CaptureSummaryFlowQuery from DataFlowTargetApi api, string noflow -where - noflow = captureNoFlow(api) and - not api.(FlowSummaryImpl::Public::SummarizedCallable).applyManualModel() +where noflow = captureNoFlow(api) select noflow order by noflow diff --git a/csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql b/csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql index b8248f5a2b50..ac6e3b3227d5 100644 --- a/csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql +++ b/csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql @@ -11,7 +11,5 @@ import internal.CaptureModels import internal.CaptureSummaryFlowQuery from DataFlowTargetApi api, string flow -where - flow = captureFlow(api) and - not api.(FlowSummaryImpl::Public::SummarizedCallable).applyManualModel() +where flow = captureFlow(api) select flow order by flow diff --git a/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll b/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll index 60748c57b087..618cd1141711 100644 --- a/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll +++ b/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll @@ -7,6 +7,7 @@ private import dotnet private import semmle.code.csharp.commons.Util as Util private import semmle.code.csharp.commons.Collections as Collections private import semmle.code.csharp.dataflow.internal.DataFlowDispatch +private import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl private import semmle.code.csharp.frameworks.system.linq.Expressions import semmle.code.csharp.dataflow.internal.ExternalFlow as ExternalFlow import semmle.code.csharp.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon @@ -37,7 +38,10 @@ private predicate isRelevantForModels(CS::Callable api) { not api instanceof Util::MainMethod and not api instanceof CS::Destructor and not api instanceof CS::AnonymousFunctionExpr and - not api.(CS::Constructor).isParameterless() + not api.(CS::Constructor).isParameterless() and + // Disregard all APIs that have a manual model. + not api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()) and + not api = any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel()) } /** diff --git a/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll b/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll index 7877594519a9..22372ac4e34f 100644 --- a/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll +++ b/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll @@ -5,6 +5,7 @@ private import java as J private import semmle.code.java.dataflow.internal.DataFlowPrivate private import semmle.code.java.dataflow.internal.ContainerFlow as ContainerFlow +private import semmle.code.java.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl private import semmle.code.java.dataflow.internal.ModelExclusions private import semmle.code.java.dataflow.DataFlow as Df private import semmle.code.java.dataflow.SSA as Ssa @@ -37,7 +38,11 @@ private predicate isInfrequentlyUsed(J::CompilationUnit cu) { */ private predicate isRelevantForModels(J::Callable api) { not isUninterestingForModels(api) and - not isInfrequentlyUsed(api.getCompilationUnit()) + not isInfrequentlyUsed(api.getCompilationUnit()) and + // Disregard all APIs that have a manual model. + not api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()).asCallable() and + not api = + any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel()).asCallable() } /** From 8702293878c1b3895cdbbf8dffb14d00cabc7ce4 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 20 Dec 2023 15:49:26 +0100 Subject: [PATCH 2/7] C#: Update expected test output for type based model generator. --- .../typebasedflow/CaptureTypeBasedSummaryModels.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/csharp/ql/test/utils/modelgenerator/typebasedflow/CaptureTypeBasedSummaryModels.expected b/csharp/ql/test/utils/modelgenerator/typebasedflow/CaptureTypeBasedSummaryModels.expected index 45b4980ea43c..28fe5d4d93f3 100644 --- a/csharp/ql/test/utils/modelgenerator/typebasedflow/CaptureTypeBasedSummaryModels.expected +++ b/csharp/ql/test/utils/modelgenerator/typebasedflow/CaptureTypeBasedSummaryModels.expected @@ -124,7 +124,6 @@ | Summaries;TypeBasedCollection;false;Add;(T);;Argument[0];Argument[this].Element;value;tb-generated | | Summaries;TypeBasedCollection;false;AddMany;(System.Collections.Generic.IEnumerable);;Argument[0].Element;Argument[this].Element;value;tb-generated | | Summaries;TypeBasedCollection;false;First;();;Argument[this].Element;ReturnValue;value;tb-generated | -| Summaries;TypeBasedCollection;false;GetEnumerator;();;Argument[this].Element;ReturnValue.SyntheticField[ArgType0];value;tb-generated | | Summaries;TypeBasedCollection;false;GetMany;();;Argument[this].Element;ReturnValue.Element;value;tb-generated | | Summaries;TypeBasedComplex;false;AddMany;(System.Collections.Generic.IEnumerable);;Argument[0].Element;Argument[this].SyntheticField[ArgType0];value;tb-generated | | Summaries;TypeBasedComplex;false;Apply;(System.Func);;Argument[this].SyntheticField[ArgType0];Argument[0].Parameter[0];value;tb-generated | From c7045fbb99b7b3058b909c82686a6666761c5e5a Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Wed, 20 Dec 2023 15:50:24 +0100 Subject: [PATCH 3/7] C#: Add some test cases for excluding methods for model generation. --- .../dataflow/CaptureNeutralModels.ext.yml | 6 ++++++ .../dataflow/CaptureSummaryModels.ext.yml | 12 ++++++++++++ .../modelgenerator/dataflow/NoSummaries.cs | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 csharp/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.ext.yml create mode 100644 csharp/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.ext.yml diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.ext.yml b/csharp/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.ext.yml new file mode 100644 index 000000000000..319d0a5ac621 --- /dev/null +++ b/csharp/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.ext.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/csharp-all + extensible: neutralModel + data: + - [ "NoSummaries", "ManuallyModelled", "HasNeutralSummaryNoFlow", "(System.Object)", "summary", "manual"] diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.ext.yml b/csharp/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.ext.yml new file mode 100644 index 000000000000..05e3b67313ce --- /dev/null +++ b/csharp/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.ext.yml @@ -0,0 +1,12 @@ +extensions: + - addsTo: + pack: codeql/csharp-all + extensible: summaryModel + data: + - [ "NoSummaries", "ManuallyModelled", False, "HasSummary", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "manual"] + + - addsTo: + pack: codeql/csharp-all + extensible: neutralModel + data: + - [ "NoSummaries", "ManuallyModelled", "HasNeutralSummary", "(System.Object)", "summary", "manual"] diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/NoSummaries.cs b/csharp/ql/test/utils/modelgenerator/dataflow/NoSummaries.cs index b2c21b579177..b9f08329480f 100644 --- a/csharp/ql/test/utils/modelgenerator/dataflow/NoSummaries.cs +++ b/csharp/ql/test/utils/modelgenerator/dataflow/NoSummaries.cs @@ -143,3 +143,22 @@ public ParameterlessConstructor() IsInitialized = true; } } + +// No models should be created, if there exist either a manual summary or neutral summary. +public class ManuallyModelled +{ + public object HasSummary(object o) + { + return o; + } + + public object HasNeutralSummary(object o) + { + return o; + } + + public object HasNeutralSummaryNoFlow(object o) + { + return null; + } +} From 03d4025b992983c0926edd7bb4a03b85373b5d86 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 11 Jan 2024 11:26:02 +0100 Subject: [PATCH 4/7] Java: Add a testcase where both a neutral summary and summary is being generated. --- .../dataflow/CaptureNeutralModels.expected | 1 + .../dataflow/CaptureSummaryModels.expected | 1 + .../dataflow/p/MultipleImpl2.java | 20 +++++++++++++++++++ 3 files changed, 22 insertions(+) create mode 100644 java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java diff --git a/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected b/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected index 5ae6398fb6fb..d8ffe6752c38 100644 --- a/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected +++ b/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected @@ -3,6 +3,7 @@ | p;FluentAPI$Inner;notThis;(String);summary;df-generated | | p;ImmutablePojo;getX;();summary;df-generated | | p;Joiner;length;();summary;df-generated | +| p;MultipleImpl2$IInterface;m;(Object);summary;df-generated | | p;ParamFlow;ignorePrimitiveReturnValue;(String);summary;df-generated | | p;ParamFlow;mapType;(Class);summary;df-generated | | p;Pojo;doNotSetValue;(String);summary;df-generated | diff --git a/java/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.expected b/java/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.expected index 24eb6d63e0d9..014d47f25dd3 100644 --- a/java/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.expected +++ b/java/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.expected @@ -22,6 +22,7 @@ | p;Joiner;false;setEmptyValue;(CharSequence);;Argument[0];Argument[this];taint;df-generated | | p;Joiner;false;setEmptyValue;(CharSequence);;Argument[this];ReturnValue;value;df-generated | | p;Joiner;false;toString;();;Argument[this];ReturnValue;taint;df-generated | +| p;MultipleImpl2$IInterface;true;m;(Object);;Argument[0];ReturnValue;taint;df-generated | | p;MultipleImpls$Strat2;true;getValue;();;Argument[this];ReturnValue;taint;df-generated | | p;MultipleImpls$Strategy;true;doSomething;(String);;Argument[0];Argument[this];taint;df-generated | | p;MultipleImpls$Strategy;true;doSomething;(String);;Argument[0];ReturnValue;taint;df-generated | diff --git a/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java b/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java new file mode 100644 index 000000000000..1606ebb74121 --- /dev/null +++ b/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java @@ -0,0 +1,20 @@ +package p; + +class MultipleImpl2 { + + public interface IInterface { + Object m(Object value); + } + + public class Impl1 implements IInterface { + public Object m(Object value) { + return null; + } + } + + public class Impl2 implements IInterface { + public Object m(Object value) { + return value; + } + } +} From 6af0bca7777a19cd40b71199e46e4b6cf1826979 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 11 Jan 2024 11:28:14 +0100 Subject: [PATCH 5/7] Java: Avoid generating contradicting summary and neutral summary models. --- .../internal/CaptureModelsSpecific.qll | 23 +++++++++++++------ .../internal/CaptureSummaryFlowQuery.qll | 2 +- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll b/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll index 22372ac4e34f..2b69c7d0aff5 100644 --- a/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll +++ b/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll @@ -22,6 +22,8 @@ class Type = J::Type; class Unit = J::Unit; +class Callable = J::Callable; + private J::Method superImpl(J::Method m) { result = m.getAnOverride() and not exists(result.getAnOverride()) and @@ -36,7 +38,7 @@ private predicate isInfrequentlyUsed(J::CompilationUnit cu) { /** * Holds if it is relevant to generate models for `api`. */ -private predicate isRelevantForModels(J::Callable api) { +private predicate isRelevantForModels(Callable api) { not isUninterestingForModels(api) and not isInfrequentlyUsed(api.getCompilationUnit()) and // Disregard all APIs that have a manual model. @@ -48,7 +50,7 @@ private predicate isRelevantForModels(J::Callable api) { /** * Holds if it is relevant to generate models for `api` based on data flow analysis. */ -predicate isRelevantForDataFlowModels(J::Callable api) { +predicate isRelevantForDataFlowModels(Callable api) { isRelevantForModels(api) and (not api.getDeclaringType() instanceof J::Interface or exists(api.getBody())) } @@ -61,7 +63,7 @@ predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1; * In the Standard library and 3rd party libraries it the Callables that can be called * from outside the library itself. */ -class TargetApiSpecific extends J::Callable { +class TargetApiSpecific extends Callable { TargetApiSpecific() { this.isPublic() and this.fromSource() and @@ -71,6 +73,15 @@ class TargetApiSpecific extends J::Callable { ) and isRelevantForModels(this) } + + /** + * Gets the callable that a model will be lifted to, if any. + */ + Callable lift() { + exists(Method m | m = superImpl(this) and m.fromSource() | result = m) + or + not exists(superImpl(this)) and result = this + } } private string isExtensible(J::RefType ref) { @@ -84,9 +95,7 @@ private string typeAsModel(J::RefType type) { } private J::RefType bestTypeForModel(TargetApiSpecific api) { - if exists(superImpl(api)) - then superImpl(api).fromSource() and result = superImpl(api).getDeclaringType() - else result = api.getDeclaringType() + result = api.lift().getDeclaringType() } /** @@ -200,7 +209,7 @@ string returnNodeAsOutput(DataFlowImplCommon::ReturnNodeExt node) { /** * Gets the enclosing callable of `ret`. */ -J::Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) { +Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) { result = DataFlowImplCommon::getNodeEnclosingCallable(ret).asCallable() } diff --git a/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll b/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll index 33b18acf0c52..e353a39afc5b 100644 --- a/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll +++ b/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll @@ -77,6 +77,6 @@ string captureFlow(DataFlowTargetApi api) { * A neutral model is generated, if there does not exist any summary model. */ string captureNoFlow(DataFlowTargetApi api) { - not exists(captureFlow(api)) and + not exists(DataFlowTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and result = ModelPrinting::asNeutralSummaryModel(api) } From 74cdcab6d880986709392ba57384d0132fa7f831 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Thu, 11 Jan 2024 11:40:44 +0100 Subject: [PATCH 6/7] Java: Update expected test output. --- .../utils/modelgenerator/dataflow/CaptureNeutralModels.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected b/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected index d8ffe6752c38..5ae6398fb6fb 100644 --- a/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected +++ b/java/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.expected @@ -3,7 +3,6 @@ | p;FluentAPI$Inner;notThis;(String);summary;df-generated | | p;ImmutablePojo;getX;();summary;df-generated | | p;Joiner;length;();summary;df-generated | -| p;MultipleImpl2$IInterface;m;(Object);summary;df-generated | | p;ParamFlow;ignorePrimitiveReturnValue;(String);summary;df-generated | | p;ParamFlow;mapType;(Class);summary;df-generated | | p;Pojo;doNotSetValue;(String);summary;df-generated | From 37a21ec548321ecdc8f7d2f89b84ea88b1ea9b97 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 12 Jan 2024 13:34:50 +0100 Subject: [PATCH 7/7] Java: Address review comments. --- .../modelgenerator/internal/CaptureSummaryFlowQuery.qll | 5 +++-- .../test/utils/modelgenerator/dataflow/p/MultipleImpl2.java | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll b/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll index e353a39afc5b..40190711e402 100644 --- a/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll +++ b/java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll @@ -73,8 +73,9 @@ string captureFlow(DataFlowTargetApi api) { } /** - * Gets the neutral summary for `api`, if any. - * A neutral model is generated, if there does not exist any summary model. + * Gets the neutral summary model for `api`, if any. + * A neutral summary model is generated, if we are not generating + * a summary model that applies to `api`. */ string captureNoFlow(DataFlowTargetApi api) { not exists(DataFlowTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and diff --git a/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java b/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java index 1606ebb74121..9de2d59b2e4f 100644 --- a/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java +++ b/java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpl2.java @@ -2,6 +2,9 @@ class MultipleImpl2 { + // Multiple implementations of the same interface. + // This is used to test that we only generate a summary model and + // not neutral summary model for `IInterface.m`. public interface IInterface { Object m(Object value); }