Support flow for factory and strategy pattern

* Support models for factories that create
new instances of an object while tainting it with incoming data
* Support models to infer super types for
private implementations to expose the models
at the right level

Author: Benjamin Muskalla

java/ql/src/utils/model-generator/CaptureSinkModels.ql

@@ -5,19 +5,24 @@
  */
 
 import java
-import Telemetry.ExternalAPI
-import semmle.code.java.dataflow.DataFlow
-import semmle.code.java.dataflow.TaintTracking
-import semmle.code.java.dataflow.ExternalFlow
-import ModelGeneratorUtils
+private import Telemetry.ExternalAPI
+private import semmle.code.java.dataflow.DataFlow
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.dataflow.ExternalFlow
+private import ModelGeneratorUtils
+private import semmle.code.java.dataflow.internal.DataFlowNodes::Private
 
 class PropagateToSinkConfiguration extends TaintTracking::Configuration {
-  PropagateToSinkConfiguration() { this = "parameters on public api flowing into sinks" }
+  PropagateToSinkConfiguration() { this = "parameters or  flowing into sinks" }
 
   override predicate isSource(DataFlow::Node source) {
-    source instanceof DataFlow::ParameterNode and
+    (source.asExpr() instanceof FieldAccess or source instanceof DataFlow::ParameterNode) and
     source.getEnclosingCallable().isPublic() and
-    source.getEnclosingCallable().getDeclaringType().isPublic() and
+    exists(RefType t |
+      t = source.getEnclosingCallable().getDeclaringType().getAnAncestor() and
+      not t instanceof TypeObject and
+      t.isPublic()
+    ) and
     isRelevantForModels(source.getEnclosingCallable())
   }
 
@@ -29,6 +34,9 @@ string asInputArgument(DataFlow::Node source) {
     source.(DataFlow::ParameterNode).isParameterOf(_, pos) and
     result = "Argument[" + pos + "]"
   )
+  or
+  source.asExpr() instanceof FieldAccess and
+  result = "Argument[-1]"
 }
 
 string captureSink(Callable api) {
@@ -42,4 +50,6 @@ string captureSink(Callable api) {
 
 from TargetAPI api, string sink
 where sink = captureSink(api)
+// and
+// api.hasName("openStream")
 select sink order by sink

java/ql/src/utils/model-generator/CaptureSourceModels.ql

@@ -13,7 +13,7 @@ private import ModelGeneratorUtils
 private import semmle.code.java.dataflow.internal.FlowSummaryImplSpecific
 private import semmle.code.java.dataflow.internal.FlowSummaryImpl
 private import semmle.code.java.dataflow.internal.DataFlowImplCommon
-import semmle.code.java.dataflow.internal.DataFlowNodes::Private
+private import semmle.code.java.dataflow.internal.DataFlowNodes::Private
 
 class FromSourceConfiguration extends TaintTracking::Configuration {
   FromSourceConfiguration() { this = "FromSourceConfiguration" }

java/ql/src/utils/model-generator/CaptureSummaryModels.ql

@@ -119,6 +119,7 @@ string captureFieldFlowIn(Callable api) {
   exists(DataFlow::ParameterNode source, DataFlow::ExprNode sink, ParameterToFieldConfig config |
     sink.asExpr().getEnclosingCallable().getDeclaringType() =
       source.asParameter().getCallable().getDeclaringType() and
+    not api.isStatic() and
     config.hasFlow(source, sink) and
     source.asParameter().getCallable() = api
   |
@@ -140,6 +141,11 @@ class ParameterToReturnValueTaintConfig extends TaintTracking::Configuration {
   }
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof ReturnNodeExt }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node2.asExpr().(ConstructorCall).getAnArgument() = node1.asExpr() and
+    node1.asExpr().(Argument).getCall().getCallee().fromSource()
+  }
 }
 
 predicate paramFlowToReturnValueExists(Parameter p) {

java/ql/src/utils/model-generator/ModelGeneratorUtils.qll

@@ -2,11 +2,20 @@ import java
 import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.dataflow.internal.ContainerFlow
 
+Method superImpl(Method m) {
+  result = m.getAnOverride() and
+  not exists(result.getAnOverride()) and
+  not m instanceof ToStringMethod
+}
+
 class TargetAPI extends Callable {
   TargetAPI() {
     this.isPublic() and
     this.fromSource() and
-    this.getDeclaringType().isPublic() and
+    (
+      this.getDeclaringType().isPublic() or
+      superImpl(this).getDeclaringType().isPublic()
+    ) and
     isRelevantForModels(this)
   }
 }
@@ -70,7 +79,7 @@ string asSourceModel(Callable api, string output, string kind) {
 private string asPartialModel(Callable api) {
   result =
     typeAsSummaryModel(api) + ";" //
-      + isExtensible(api.getDeclaringType()).toString() + ";" //
+      + isExtensible(bestTypeForModel(api)) + ";" //
       + api.getName() + ";" //
       + paramsString(api) + ";" //
       + /* ext + */ ";" //
@@ -80,18 +89,12 @@ private string asPartialModel(Callable api) {
  * Returns the appropriate type name for the model. Either the type
  * declaring the method or the supertype introducing the method.
  */
-private string typeAsSummaryModel(Callable api) {
-  if exists(superImpl(api.(Method)))
-  then
-    superImpl(api.(Method)).fromSource() and
-    result = typeAsModel(superImpl(api.(Method)).getDeclaringType())
-  else result = typeAsModel(api.getDeclaringType())
-}
+private string typeAsSummaryModel(Callable api) { result = typeAsModel(bestTypeForModel(api)) }
 
-Method superImpl(Method m) {
-  result = m.getAnOverride() and
-  not exists(result.getAnOverride()) and
-  not m instanceof ToStringMethod
+private RefType bestTypeForModel(Callable api) {
+  if exists(superImpl(api))
+  then superImpl(api).fromSource() and result = superImpl(api).getDeclaringType()
+  else result = api.getDeclaringType()
 }
 
 private string typeAsModel(RefType type) {

java/ql/test/utils/model-generator/CaptureSinkModels.expected

@@ -1,3 +1,4 @@
+| p;PrivateFlowViaPublicInterface$SPI;true;openStream;();;Argument[-1];create-file; |
 | p;Sinks;true;copyFileToDirectory;(Path,Path,CopyOption[]);;Argument[1];create-file; |
 | p;Sinks;true;readUrl;(URL,Charset);;Argument[0];open-url; |
 | p;Sources;true;readUrl;(URL);;Argument[0];open-url; |

java/ql/test/utils/model-generator/CaptureSummaryModels.expected

@@ -1,5 +1,5 @@
-| p;Factory;false;create;(String);;Argument[0];Argument[-1];taint; |
-| p;Factory;false;create;(String,int);;Argument[0];Argument[-1];taint; |
+| p;Factory;false;create;(String);;Argument[0];ReturnValue;taint; |
+| p;Factory;false;create;(String,int);;Argument[0];ReturnValue;taint; |
 | p;Factory;false;getValue;();;Argument[-1];ReturnValue;taint; |
 | p;FinalClass;false;returnsInput;(String);;Argument[0];ReturnValue;taint; |
 | p;FluentAPI;false;returnsThis;(String);;Argument[-1];ReturnValue;value; |
@@ -38,3 +38,4 @@
 | p;Pojo;false;getCharArray;();;Argument[-1];ReturnValue;taint; |
 | p;Pojo;false;getValue;();;Argument[-1];ReturnValue;taint; |
 | p;Pojo;false;setValue;(String);;Argument[0];Argument[-1];taint; |
+| p;PrivateFlowViaPublicInterface;true;createAnSPI;(File);;Argument[0];ReturnValue;taint; |

java/ql/test/utils/model-generator/p/PrivateFlowViaPublicInterface.java

@@ -0,0 +1,33 @@
+package p;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
+
+public class PrivateFlowViaPublicInterface {
+
+    public static interface SPI {
+        OutputStream openStream() throws IOException;
+    }
+
+    private static final class PrivateImplWithSink implements SPI {
+
+        private File file;
+
+        public PrivateImplWithSink(File file) {
+            this.file = file;
+        }
+
+        @Override
+        public OutputStream openStream() throws IOException {
+            return new FileOutputStream(file);
+        }
+
+    }
+
+    public static SPI createAnSPI(File file) {
+        return new PrivateImplWithSink(file);
+    }
+
+}