[quandary] don't report FP's due to flow-insensitive traces

Summary:
We only ought to report a source-sink flow at the call site where the sink is introduced.
Otherwise, we will report silly false positives.

Reviewed By: jeremydubreil

Differential Revision: D4234766

fbshipit-source-id: 118051f

infer/src/checkers/Trace.ml

@@ -67,9 +67,6 @@ module type S = sig
   (** add a sink to the current trace. *)
   val add_sink : Sink.t -> t -> t
 
-  (** remove the given sinks from the current trace *)
-  val filter_sinks : t -> Sink.t list -> t
-
   (** append the trace for given call site to the current caller trace *)
   val append : t -> t -> CallSite.t -> t
 
@@ -296,10 +293,6 @@ module Make (Spec : Spec) = struct
     let sinks = Sinks.add sink t.sinks in
     { t with sinks; }
 
-  let filter_sinks t sinks_to_filter =
-    let sinks = Sinks.diff t.sinks (Sinks.of_list sinks_to_filter) in
-    { t with sinks; }
-
   (** compute caller_trace + callee_trace *)
   let append caller_trace callee_trace callee_site =
     if is_empty callee_trace

infer/src/checkers/Trace.mli

@@ -67,9 +67,6 @@ module type S = sig
   (** add a sink to the current trace. *)
   val add_sink : Sink.t -> t -> t
 
-  (** remove the given sinks from the current trace *)
-  val filter_sinks : t -> Sink.t list -> t
-
   (** append the trace for given call site to the current caller trace *)
   val append : t -> t -> CallSite.t -> t
 

infer/src/quandary/TaintAnalysis.ml

@@ -153,8 +153,8 @@ module Make (TaintSpecification : TaintSpec.S) = struct
       let id_ap = AccessPath.Exact (AccessPath.of_id ret_id ret_typ) in
       TaintDomain.add_trace id_ap trace access_tree
 
-    (* log any reportable source-sink flows in [trace] and remove logged sinks from the trace *)
-    let report_and_filter_trace trace callee_site (proc_data : formal_map ProcData.t) =
+    (** log any new reportable source-sink flows in [trace] *)
+    let report_trace trace callee_site (proc_data : formal_map ProcData.t) =
       let trace_of_pname pname =
         match Summary.read_summary proc_data.pdesc pname with
         | Some summary ->
@@ -173,32 +173,30 @@ module Make (TaintSpecification : TaintSpec.S) = struct
           TraceDomain.Source.pp original_source
           TraceDomain.Sink.pp final_sink in
 
-      match TraceDomain.get_reportable_paths trace ~trace_of_pname with
-      | [] ->
-          trace
-      | paths ->
-          let report_error path =
-            let caller_pname = Procdesc.get_proc_name proc_data.pdesc in
-            let msg = Localise.to_string Localise.quandary_taint_error in
-            let trace_str = F.asprintf "%a" pp_path_short path in
-            let ltr = TraceDomain.to_loc_trace path in
-            let exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in
-            Reporting.log_error caller_pname ~loc:(CallSite.loc callee_site) ~ltr exn in
-
-          let reported_sinks =
-            IList.map
-              (fun ((_, sources, sinks) as path) ->
-                 let source = fst (IList.hd (IList.rev (sources))) in
-                 let sink = fst (IList.hd (IList.rev sinks)) in
-                 if not (CallSite.equal
-                           (TraceDomain.Source.call_site source)
-                           (TraceDomain.Sink.call_site sink))
-                 then report_error path;
-                 sink)
-              paths in
-          (* got new source -> sink flow. report it, but don't add the sink to the trace. if we do,
-             we will double-report later on. *)
-          TraceDomain.filter_sinks trace reported_sinks
+      let report_error path =
+        let caller_pname = Procdesc.get_proc_name proc_data.pdesc in
+        let msg = Localise.to_string Localise.quandary_taint_error in
+        let trace_str = F.asprintf "%a" pp_path_short path in
+        let ltr = TraceDomain.to_loc_trace path in
+        let exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in
+        Reporting.log_error caller_pname ~loc:(CallSite.loc callee_site) ~ltr exn in
+
+      let reportable_paths = TraceDomain.get_reportable_paths trace ~trace_of_pname in
+      IList.iter
+        (fun ((_, sources, sinks) as path) ->
+           (* this is the first callee in a chain that transitively calls a sink *)
+           let indirect_sink = fst (IList.hd (IList.rev sinks)) in
+           let indirect_sink_site = TraceDomain.Sink.call_site indirect_sink in
+           (* report when: (1) this call site introduces the sink, and (2) this call site does not
+              also introduce the source. otherwise, we'll report paths that don't respect control
+              flow. *)
+           if CallSite.equal indirect_sink_site callee_site &&
+              (* the is the first callee in a chain that transitively returns a source *)
+              let indirect_source = fst (IList.hd (IList.rev sources)) in
+              not (CallSite.equal indirect_sink_site (TraceDomain.Source.call_site indirect_source))
+           then
+             report_error path)
+        reportable_paths
 
     let add_sinks sinks actuals ({ Domain.access_tree; id_map; } as astate) proc_data callee_site =
       let f_resolve_id = resolve_id id_map in
@@ -223,12 +221,8 @@ module Make (TaintSpecification : TaintSpec.S) = struct
               match access_path_get_node
                       actual_ap access_tree_acc proc_data (CallSite.loc callee_site) with
               | Some (actual_trace, _) ->
-                  (* add callee_pname to actual trace as a sink *)
-                  let actual_trace' =
-                    report_and_filter_trace
-                      (TraceDomain.add_sink sink_param.sink actual_trace)
-                      callee_site
-                      proc_data in
+                  let actual_trace' = TraceDomain.add_sink sink_param.sink actual_trace in
+                  report_trace actual_trace' callee_site proc_data;
                   TaintDomain.add_trace actual_ap actual_trace' access_tree_acc
               | None ->
                   access_tree_acc
@@ -302,8 +296,14 @@ module Make (TaintSpecification : TaintSpec.S) = struct
             let output_trace = TaintSpecification.of_summary_trace in_out_summary.output_trace in
             let appended_trace = TraceDomain.append in_trace output_trace callee_site in
             let joined_trace = TraceDomain.join caller_trace appended_trace in
-            let filtered_trace = report_and_filter_trace joined_trace callee_site proc_data in
-            TaintDomain.add_trace caller_ap filtered_trace access_tree
+            if appended_trace == joined_trace
+            then
+              access_tree
+            else
+              begin
+                report_trace joined_trace callee_site proc_data;
+                TaintDomain.add_trace caller_ap joined_trace access_tree
+              end
         | None ->
             access_tree in
 

infer/tests/codetoanalyze/java/quandary/FlowSensitivity.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2016 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+package codetoanalyze.java.quandary;
+
+import com.facebook.infer.builtins.InferTaint;
+
+/** making sure the traces we report respect control-flow */
+
+class FlowSensitivity {
+
+  static class Obj {
+    Object f;
+  }
+
+  static void callSink(Obj o) {
+    InferTaint.inferSensitiveSink(o.f);
+  }
+
+  static void returnSource(Obj o) {
+    o.f = InferTaint.inferSecretSource();
+  }
+
+  static void interproceduralFlowSensitivityOk1(Obj o) {
+    InferTaint.inferSensitiveSink(o.f);
+    returnSource(o);
+  }
+
+  static void interproceduralFlowSensitivityOk2(Obj o) {
+    callSink(o);
+    o.f = InferTaint.inferSecretSource();
+  }
+
+  static void interproceduralFlowSensitivityOk3(Obj o) {
+    callSink(o);
+    returnSource(o);
+  }
+
+  static void interproceduralFlowSensitivityBad(Obj o) {
+    returnSource(o);
+    callSink(o);
+  }
+
+  static void sourceAndSink(Obj o) {
+    InferTaint.inferSensitiveSink(o.f);
+    o.f = InferTaint.inferSecretSource();
+  }
+
+  static void callSourceAndSinkOk(Obj o) {
+    sourceAndSink(o);
+  }
+
+  static void callSourceAndSinkBad1(Obj o) {
+    sourceAndSink(o);
+    InferTaint.inferSensitiveSink(o.f);
+  }
+
+  static void callSourceAndSinkBad2(Obj o) {
+    o.f = InferTaint.inferSecretSource();
+    sourceAndSink(o);
+  }
+
+}

infer/tests/codetoanalyze/java/quandary/Interprocedural.java

@@ -295,23 +295,4 @@ class Interprocedural {
     InferTaint.inferSensitiveSink(notASource);
   }
 
-  static void sourceAndSink(Obj o) {
-    InferTaint.inferSensitiveSink(o.f);
-    o.f = InferTaint.inferSecretSource();
-  }
-
-  static void callSourceAndSinkOk(Obj o) {
-    sourceAndSink(o);
-  }
-
-  static void callSourceAndSinkBad1(Obj o) {
-    sourceAndSink(o);
-    InferTaint.inferSensitiveSink(InferTaint.inferSecretSource());
-  }
-
-  static void callSourceAndSinkBad2(Obj o) {
-    o.f = InferTaint.inferSecretSource();
-    sourceAndSink(o);
-  }
-
 }

infer/tests/codetoanalyze/java/quandary/issues.exp

@@ -50,6 +50,9 @@ codetoanalyze/java/quandary/Fields.java, void Fields.viaFieldBad2(), 3, QUANDARY
 codetoanalyze/java/quandary/Fields.java, void Fields.viaFieldBad3(), 4, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Fields.java, void Fields.viaNestedFieldBad1(Fields$Obj), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Fields.java, void Fields.viaNestedFieldBad2(), 4, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void InferTaint.inferSensitiveSink(Object)]
+codetoanalyze/java/quandary/FlowSensitivity.java, void FlowSensitivity.callSourceAndSinkBad1(FlowSensitivity$Obj), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),return from void FlowSensitivity.sourceAndSink(FlowSensitivity$Obj),call to void InferTaint.inferSensitiveSink(Object)]
+codetoanalyze/java/quandary/FlowSensitivity.java, void FlowSensitivity.callSourceAndSinkBad2(FlowSensitivity$Obj), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void FlowSensitivity.sourceAndSink(FlowSensitivity$Obj),call to void InferTaint.inferSensitiveSink(Object)]
+codetoanalyze/java/quandary/FlowSensitivity.java, void FlowSensitivity.interproceduralFlowSensitivityBad(FlowSensitivity$Obj), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),return from void FlowSensitivity.returnSource(FlowSensitivity$Obj),call to void FlowSensitivity.callSink(FlowSensitivity$Obj),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Intents.java, void Intents.callAllSinksBad(Activity,String), 13, QUANDARY_TAINT_ERROR, [return from Intent Intent.parseUri(String,int),call to boolean ContextWrapper.bindService(Intent,ServiceConnection,int)]
 codetoanalyze/java/quandary/Intents.java, void Intents.callAllSinksBad(Activity,String), 13, QUANDARY_TAINT_ERROR, [return from Intent Intent.parseIntent(Resources,XmlPullParser,AttributeSet),call to boolean ContextWrapper.bindService(Intent,ServiceConnection,int)]
 codetoanalyze/java/quandary/Intents.java, void Intents.callAllSinksBad(Activity,String), 14, QUANDARY_TAINT_ERROR, [return from Intent Intent.parseUri(String,int),call to void ContextWrapper.sendBroadcast(Intent)]
@@ -92,8 +95,6 @@ codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.callSinkO
 codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.callSinkParam1Bad(), 1, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void Interprocedural.callSinkParam1(Object,Object),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.callSinkParam2Bad(), 1, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void Interprocedural.callSinkParam2(Object,Object),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.callSinkVariadicBad(), 1, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void Interprocedural.callSinkVariadic(java.lang.Object[]),call to void InferTaint.inferSensitiveSink(Object)]
-codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.callSourceAndSinkBad1(Interprocedural$Obj), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void InferTaint.inferSensitiveSink(Object)]
-codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.callSourceAndSinkBad2(Interprocedural$Obj), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),call to void Interprocedural.sourceAndSink(Interprocedural$Obj),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.doublePassthroughBad(), 4, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),flow through Object Interprocedural.id(Object),flow through Object Interprocedural.id(Object),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.getGlobalThenCallSinkBad(), 2, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),flow through void Interprocedural.getGlobalThenCallSink(),call to void Interprocedural.getGlobalThenCallSink(),flow through Object Interprocedural.getGlobal(),call to void InferTaint.inferSensitiveSink(Object)]
 codetoanalyze/java/quandary/Interprocedural.java, void Interprocedural.returnSourceDirectBad(), 1, QUANDARY_TAINT_ERROR, [return from Object InferTaint.inferSecretSource(),return from Object Interprocedural.returnSourceDirect(),call to void InferTaint.inferSensitiveSink(Object)]