diff --git a/csharp/ql/src/utils/modelgenerator/internal/CaptureModels.qll b/csharp/ql/src/utils/modelgenerator/internal/CaptureModels.qll index 0237e3fdc68b..25b5ceea5abc 100644 --- a/csharp/ql/src/utils/modelgenerator/internal/CaptureModels.qll +++ b/csharp/ql/src/utils/modelgenerator/internal/CaptureModels.qll @@ -145,7 +145,7 @@ private class TaintStore extends TaintState, TTaintStore { * * This can be used to generate Flow summaries for APIs from parameter to return. */ -module ThroughFlowConfig implements DataFlow::StateConfigSig { +module PropagateFlowConfig implements DataFlow::StateConfigSig { class FlowState = TaintState; predicate isSource(DataFlow::Node source, FlowState state) { @@ -190,14 +190,14 @@ module ThroughFlowConfig implements DataFlow::StateConfigSig { } } -private module ThroughFlow = TaintTracking::GlobalWithState; +private module PropagateFlow = TaintTracking::GlobalWithState; /** * Gets the summary model(s) of `api`, if there is flow from parameters to return value or parameter. */ string captureThroughFlow(DataFlowTargetApi api) { exists(DataFlow::ParameterNode p, ReturnNodeExt returnNodeExt, string input, string output | - ThroughFlow::flow(p, returnNodeExt) and + PropagateFlow::flow(p, returnNodeExt) and returnNodeExt.(DataFlow::Node).getEnclosingCallable() = api and input = parameterNodeAsInput(p) and output = returnNodeExt.getOutput() and @@ -213,8 +213,13 @@ string captureThroughFlow(DataFlowTargetApi api) { * This can be used to generate Source summaries for an API, if the API expose an already known source * via its return (then the API itself becomes a source). */ -module FromSourceConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { ExternalFlow::sourceNode(source, _) } +module PropagateFromSourceConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(string kind | + isRelevantSourceKind(kind) and + ExternalFlow::sourceNode(source, kind) + ) + } predicate isSink(DataFlow::Node sink) { exists(DataFlowTargetApi c | @@ -225,22 +230,26 @@ module FromSourceConfig implements DataFlow::ConfigSig { DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSinkCallContext } + predicate isBarrier(DataFlow::Node n) { + exists(Type t | t = n.getType() and not isRelevantType(t)) + } + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { isRelevantTaintStep(node1, node2) } } -private module FromSource = TaintTracking::Global; +private module PropagateFromSource = TaintTracking::Global; /** * Gets the source model(s) of `api`, if there is flow from an existing known source to the return of `api`. */ string captureSource(DataFlowTargetApi api) { exists(DataFlow::Node source, ReturnNodeExt sink, string kind | - FromSource::flow(source, sink) and + PropagateFromSource::flow(source, sink) and ExternalFlow::sourceNode(source, kind) and api = sink.getEnclosingCallable() and - isRelevantSourceKind(kind) and + not irrelevantSourceSinkApi(source.getEnclosingCallable(), api) and result = ModelPrinting::asSourceModel(api, sink.getOutput(), kind) ) } @@ -255,9 +264,15 @@ string captureSource(DataFlowTargetApi api) { module PropagateToSinkConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { apiSource(source) } - predicate isSink(DataFlow::Node sink) { ExternalFlow::sinkNode(sink, _) } + predicate isSink(DataFlow::Node sink) { + exists(string kind | isRelevantSinkKind(kind) and ExternalFlow::sinkNode(sink, kind)) + } - predicate isBarrier(DataFlow::Node node) { sinkModelSanitizer(node) } + predicate isBarrier(DataFlow::Node node) { + exists(Type t | t = node.getType() and not isRelevantType(t)) + or + sinkModelSanitizer(node) + } DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } @@ -276,7 +291,6 @@ string captureSink(DataFlowTargetApi api) { PropagateToSink::flow(src, sink) and ExternalFlow::sinkNode(sink, kind) and api = src.getEnclosingCallable() and - isRelevantSinkKind(kind) and result = ModelPrinting::asSinkModel(api, asInputArgument(src), kind) ) } diff --git a/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll b/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll index f37d2d8cf56b..20d919c46c87 100644 --- a/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll +++ b/csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll @@ -257,6 +257,29 @@ predicate apiSource(DataFlow::Node source) { ) } +private predicate uniquelyCalls(DataFlowCallable dc1, DataFlowCallable dc2) { + exists(DataFlowCall call | + dc1 = call.getEnclosingCallable() and + dc2 = unique(DataFlowCallable dc0 | dc0 = viableCallable(call) | dc0) + ) +} + +bindingset[dc1, dc2] +private predicate uniquelyCallsPlus(DataFlowCallable dc1, DataFlowCallable dc2) = + fastTC(uniquelyCalls/2)(dc1, dc2) + +/** + * Holds if it is not relevant to generate a source model for `api`, even + * if flow is detected from a node within `source` to a sink within `api`. + */ +bindingset[sourceEnclosing, api] +predicate irrelevantSourceSinkApi(Callable sourceEnclosing, TargetApiSpecific api) { + not exists(DataFlowCallable dc1, DataFlowCallable dc2 | uniquelyCallsPlus(dc1, dc2) or dc1 = dc2 | + dc1.getUnderlyingCallable() = api and + dc2.getUnderlyingCallable() = sourceEnclosing + ) +} + /** * Gets the MaD input string representation of `source`. */ diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/CaptureSinkModels.ext.yml b/csharp/ql/test/utils/modelgenerator/dataflow/CaptureSinkModels.ext.yml new file mode 100644 index 000000000000..277598af2f9d --- /dev/null +++ b/csharp/ql/test/utils/modelgenerator/dataflow/CaptureSinkModels.ext.yml @@ -0,0 +1,6 @@ +extensions: + - addsTo: + pack: codeql/csharp-all + extensible: sinkModel + data: + - [ "Sinks", "NewSinks", False, "Sink", "(System.Object)", "", "Argument[0]", "test-sink", "manual"] diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/Sinks.cs b/csharp/ql/test/utils/modelgenerator/dataflow/Sinks.cs index 8c8d541cc3a5..a75d6aea0e10 100644 --- a/csharp/ql/test/utils/modelgenerator/dataflow/Sinks.cs +++ b/csharp/ql/test/utils/modelgenerator/dataflow/Sinks.cs @@ -12,6 +12,10 @@ public class NewSinks public string TaintedProp { get; set; } public string PrivateSetTaintedProp { get; private set; } + // Sink defined in the extensible file next to the test. + // neutral=Sinks;NewSinks;Sink;(System.Object);summary;df-generated + public void Sink(object o) => throw null; + // New sink // sink=Sinks;NewSinks;false;WrapResponseWrite;(System.Object);;Argument[0];html-injection;df-generated // neutral=Sinks;NewSinks;WrapResponseWrite;(System.Object);summary;df-generated @@ -78,6 +82,14 @@ public void WrapPropPrivateSetResponseWriteFile() var response = new HttpResponse(); response.WriteFile(PrivateSetTaintedProp); } + + // Not a new sink because a simple type is used in an intermediate step + // neutral=Sinks;NewSinks;WrapResponseWriteFileSimpleType;(System.String);summary;df-generated + public void WrapResponseWriteFileSimpleType(string s) + { + var r = s == "hello"; + Sink(r); + } } public class CompoundSinks diff --git a/csharp/ql/test/utils/modelgenerator/dataflow/Sources.cs b/csharp/ql/test/utils/modelgenerator/dataflow/Sources.cs index 8d1edfc0b86d..8703f5b78f8d 100644 --- a/csharp/ql/test/utils/modelgenerator/dataflow/Sources.cs +++ b/csharp/ql/test/utils/modelgenerator/dataflow/Sources.cs @@ -34,4 +34,34 @@ public ConsoleKeyInfo WrapConsoleReadKey() { return Console.ReadKey(); } + + // Not a new source because a simple type is used in an intermediate step + // neutral=Sources;NewSources;WrapConsoleReadLineGetBool;();summary;df-generated + public bool WrapConsoleReadLineGetBool() + { + var s = Console.ReadLine(); + return s == "hello"; + } + + public class MyConsoleReader + { + // source=Sources;NewSources+MyConsoleReader;false;ToString;();;ReturnValue;local;df-generated + // neutral=Sources;NewSources+MyConsoleReader;ToString;();summary;df-generated + public override string ToString() + { + return Console.ReadLine(); + } + } + + + public class MyContainer + { + public T Value { get; set; } + + // summary=Sources;NewSources+MyContainer;false;Read;();;Argument[this];ReturnValue;taint;df-generated + public string Read() + { + return Value.ToString(); + } + } } diff --git a/java/ql/src/utils/modelgenerator/internal/CaptureModels.qll b/java/ql/src/utils/modelgenerator/internal/CaptureModels.qll index 0237e3fdc68b..25b5ceea5abc 100644 --- a/java/ql/src/utils/modelgenerator/internal/CaptureModels.qll +++ b/java/ql/src/utils/modelgenerator/internal/CaptureModels.qll @@ -145,7 +145,7 @@ private class TaintStore extends TaintState, TTaintStore { * * This can be used to generate Flow summaries for APIs from parameter to return. */ -module ThroughFlowConfig implements DataFlow::StateConfigSig { +module PropagateFlowConfig implements DataFlow::StateConfigSig { class FlowState = TaintState; predicate isSource(DataFlow::Node source, FlowState state) { @@ -190,14 +190,14 @@ module ThroughFlowConfig implements DataFlow::StateConfigSig { } } -private module ThroughFlow = TaintTracking::GlobalWithState; +private module PropagateFlow = TaintTracking::GlobalWithState; /** * Gets the summary model(s) of `api`, if there is flow from parameters to return value or parameter. */ string captureThroughFlow(DataFlowTargetApi api) { exists(DataFlow::ParameterNode p, ReturnNodeExt returnNodeExt, string input, string output | - ThroughFlow::flow(p, returnNodeExt) and + PropagateFlow::flow(p, returnNodeExt) and returnNodeExt.(DataFlow::Node).getEnclosingCallable() = api and input = parameterNodeAsInput(p) and output = returnNodeExt.getOutput() and @@ -213,8 +213,13 @@ string captureThroughFlow(DataFlowTargetApi api) { * This can be used to generate Source summaries for an API, if the API expose an already known source * via its return (then the API itself becomes a source). */ -module FromSourceConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node source) { ExternalFlow::sourceNode(source, _) } +module PropagateFromSourceConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { + exists(string kind | + isRelevantSourceKind(kind) and + ExternalFlow::sourceNode(source, kind) + ) + } predicate isSink(DataFlow::Node sink) { exists(DataFlowTargetApi c | @@ -225,22 +230,26 @@ module FromSourceConfig implements DataFlow::ConfigSig { DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSinkCallContext } + predicate isBarrier(DataFlow::Node n) { + exists(Type t | t = n.getType() and not isRelevantType(t)) + } + predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { isRelevantTaintStep(node1, node2) } } -private module FromSource = TaintTracking::Global; +private module PropagateFromSource = TaintTracking::Global; /** * Gets the source model(s) of `api`, if there is flow from an existing known source to the return of `api`. */ string captureSource(DataFlowTargetApi api) { exists(DataFlow::Node source, ReturnNodeExt sink, string kind | - FromSource::flow(source, sink) and + PropagateFromSource::flow(source, sink) and ExternalFlow::sourceNode(source, kind) and api = sink.getEnclosingCallable() and - isRelevantSourceKind(kind) and + not irrelevantSourceSinkApi(source.getEnclosingCallable(), api) and result = ModelPrinting::asSourceModel(api, sink.getOutput(), kind) ) } @@ -255,9 +264,15 @@ string captureSource(DataFlowTargetApi api) { module PropagateToSinkConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { apiSource(source) } - predicate isSink(DataFlow::Node sink) { ExternalFlow::sinkNode(sink, _) } + predicate isSink(DataFlow::Node sink) { + exists(string kind | isRelevantSinkKind(kind) and ExternalFlow::sinkNode(sink, kind)) + } - predicate isBarrier(DataFlow::Node node) { sinkModelSanitizer(node) } + predicate isBarrier(DataFlow::Node node) { + exists(Type t | t = node.getType() and not isRelevantType(t)) + or + sinkModelSanitizer(node) + } DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext } @@ -276,7 +291,6 @@ string captureSink(DataFlowTargetApi api) { PropagateToSink::flow(src, sink) and ExternalFlow::sinkNode(sink, kind) and api = src.getEnclosingCallable() and - isRelevantSinkKind(kind) and result = ModelPrinting::asSinkModel(api, asInputArgument(src), kind) ) } diff --git a/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll b/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll index 1ba7ece8e8ec..8ecab746e73e 100644 --- a/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll +++ b/java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll @@ -278,6 +278,12 @@ predicate apiSource(DataFlow::Node source) { ) } +/** + * Holds if it is not relevant to generate a source model for `api`, even + * if flow is detected from a node within `source` to a sink within `api`. + */ +predicate irrelevantSourceSinkApi(Callable source, TargetApiSpecific api) { none() } + /** * Gets the MaD input string representation of `source`. */ diff --git a/java/ql/test/utils/modelgenerator/dataflow/CaptureSourceModels.ext.yml b/java/ql/test/utils/modelgenerator/dataflow/CaptureSourceModels.ext.yml new file mode 100644 index 000000000000..1efde41cf141 --- /dev/null +++ b/java/ql/test/utils/modelgenerator/dataflow/CaptureSourceModels.ext.yml @@ -0,0 +1,7 @@ +extensions: + + - addsTo: + pack: codeql/java-all + extensible: sourceModel + data: + - [ "p", "Sources", False, "source", "()", "", "ReturnValue", "test-source", "manual" ] diff --git a/java/ql/test/utils/modelgenerator/dataflow/p/Sinks.java b/java/ql/test/utils/modelgenerator/dataflow/p/Sinks.java index c7121978573c..3cd3eb59cca6 100644 --- a/java/ql/test/utils/modelgenerator/dataflow/p/Sinks.java +++ b/java/ql/test/utils/modelgenerator/dataflow/p/Sinks.java @@ -64,4 +64,11 @@ public void hasManualSinkNeutral(Object o) { public void compoundPropgate(Sinks s) { s.fieldSink(); } + + // Not a new sink because a simple type is used in an intermediate step + // neutral=p;Sinks;wrapSinkSimpleType;(String);summary;df-generated + public void wrapSinkSimpleType(String s) { + Boolean b = s == "hello"; + sink(b); + } } diff --git a/java/ql/test/utils/modelgenerator/dataflow/p/Sources.java b/java/ql/test/utils/modelgenerator/dataflow/p/Sources.java index 436bf16797c7..fdae8f11ce91 100644 --- a/java/ql/test/utils/modelgenerator/dataflow/p/Sources.java +++ b/java/ql/test/utils/modelgenerator/dataflow/p/Sources.java @@ -8,6 +8,12 @@ public class Sources { + // Defined as a source in the model file next to the test. + // neutral=p;Sources;source;();summary;df-generated + public String source() { + return ""; + } + // source=p;Sources;true;readUrl;(URL);;ReturnValue;remote;df-generated // sink=p;Sources;true;readUrl;(URL);;Argument[0];request-forgery;df-generated // neutral=p;Sources;readUrl;(URL);summary;df-generated @@ -37,4 +43,27 @@ public void sourceToParameter(InputStream[] streams, List otherStre streams[0] = socket.accept().getInputStream(); otherStreams.add(socket.accept().getInputStream()); } + + // Not a new source because a simple type is used in an intermediate step + // neutral=p;Sources;wrapSourceGetBool;();summary;df-generated + public Boolean wrapSourceGetBool() { + String s = source(); + return s == "hello"; + } + + public class SourceReader { + @Override + public String toString() { + return source(); + } + } + + public class MyContainer { + private T value; + + // neutral=p;Sources$MyContainer;read;();summary;df-generated + public String read() { + return value.toString(); + } + } }