[traces] allow reported traces to return an issue type

Summary: A stepping stone to have descriptive issue types for each kind of flow rather that lumping everything into `QUANDARY_TAINT_ERROR`.

Reviewed By: mbouaziz

Differential Revision: D6126690

fbshipit-source-id: a7230c0

infer/src/base/IssueType.ml

@@ -148,6 +148,8 @@ let divide_by_zero = from_string ~enabled:false "DIVIDE_BY_ZERO"
 
 let double_lock = from_string "DOUBLE_LOCK"
 
+let do_not_report = from_string "DO_NOT_REPORT"
+
 let empty_vector_access = from_string "EMPTY_VECTOR_ACCESS"
 
 let eradicate_condition_redundant =

infer/src/base/IssueType.mli

@@ -99,6 +99,9 @@ val divide_by_zero : t
 
 val double_lock : t
 
+val do_not_report : t
+(** an issue type that should never be reported *)
+
 val empty_vector_access : t
 
 val eradicate_condition_redundant : t

infer/src/checkers/SinkTrace.ml

@@ -42,7 +42,7 @@ module Make (TraceElem : TraceElem.S) = struct
     module Source = Source.Dummy
     module Sink = MakeSink (TraceElem)
 
-    let should_report _ _ = true
+    let get_report _ _ = Some IssueType.do_not_report
   end)
 
   type sink_path = Passthroughs.t * (Sink.t * Passthroughs.t) list

infer/src/checkers/Trace.ml

@@ -16,8 +16,7 @@ module type Spec = sig
 
   module Sink : Sink.S
 
-  val should_report : Source.t -> Sink.t -> bool
+  val get_report : Source.t -> Sink.t -> IssueType.t option
-  (** should a flow originating at source and entering sink be reported? *)
 end
 
 module type S = sig
@@ -63,6 +62,12 @@ module type S = sig
   type path =
     Passthroughs.t * (path_source * Passthroughs.t) list * (path_sink * Passthroughs.t) list
 
+  type report =
+    { issue: IssueType.t
+    ; path_source: path_source
+    ; path_sink: path_sink
+    ; path_passthroughs: Passthroughs.t }
+
   val empty : t
 
   val sources : t -> Sources.t
@@ -74,7 +79,7 @@ module type S = sig
   val passthroughs : t -> Passthroughs.t
   (** get the passthroughs of the trace *)
 
-  val get_reports : ?cur_site:CallSite.t -> t -> (path_source * path_sink * Passthroughs.t) list
+  val get_reports : ?cur_site:CallSite.t -> t -> report list
   (** get the reportable source-sink flows in this trace. specifying [cur_site] restricts the
       reported paths to ones introduced by the call at [cur_site]  *)
 
@@ -250,6 +255,12 @@ module Make (Spec : Spec) = struct
   type path =
     Passthroughs.t * (path_source * Passthroughs.t) list * (path_sink * Passthroughs.t) list
 
+  type report =
+    { issue: IssueType.t
+    ; path_source: path_source
+    ; path_sink: path_sink
+    ; path_passthroughs: Passthroughs.t }
+
   let pp fmt {sources; sinks; passthroughs} =
     let pp_passthroughs fmt passthroughs =
       if not (Passthroughs.is_empty passthroughs) then
@@ -297,8 +308,16 @@ module Make (Spec : Spec) = struct
       (* written to avoid closure allocations in hot code. change with caution. *)
       let report_source source sinks acc0 =
         let report_one sink acc =
-          if Spec.should_report source sink && should_report_at_site source sink then
+          if should_report_at_site source sink then
-            (Known source, sink, t.passthroughs) :: acc
+            match Spec.get_report source sink with
+            | Some issue ->
+                { issue
+                ; path_source= Known source
+                ; path_sink= sink
+                ; path_passthroughs= t.passthroughs }
+                :: acc
+            | None ->
+                acc
           else acc
         in
         Sinks.fold report_one sinks acc0
@@ -384,8 +403,8 @@ module Make (Spec : Spec) = struct
           ([], [])
     in
     List.map
-      ~f:(fun (path_source, sink, passthroughs) ->
+      ~f:(fun {path_source; path_sink; path_passthroughs} ->
-        let sources_passthroughs, sinks_passthroughs = expand_path path_source sink in
+        let sources_passthroughs, sinks_passthroughs = expand_path path_source path_sink in
         let filtered_passthroughs =
           let source_site =
             match path_source with
@@ -394,7 +413,7 @@ module Make (Spec : Spec) = struct
             | Footprint _ ->
                 Option.value ~default:CallSite.dummy cur_site
           in
-          filter_passthroughs_ Top_level source_site (Sink.call_site sink) passthroughs
+          filter_passthroughs_ Top_level source_site (Sink.call_site path_sink) path_passthroughs
         in
         (filtered_passthroughs, sources_passthroughs, sinks_passthroughs))
       (get_reports ?cur_site t)

infer/src/checkers/Trace.mli

@@ -16,8 +16,8 @@ module type Spec = sig
 
   module Sink : Sink.S
 
-  val should_report : Source.t -> Sink.t -> bool
+  val get_report : Source.t -> Sink.t -> IssueType.t option
-  (** should a flow originating at source and entering sink be reported? *)
+  (** return Some(issue) if the source and sink match, None otherwise *)
 end
 
 module type S = sig
@@ -71,6 +71,12 @@ module type S = sig
   type path =
     Passthroughs.t * (path_source * Passthroughs.t) list * (path_sink * Passthroughs.t) list
 
+  type report =
+    { issue: IssueType.t
+    ; path_source: path_source
+    ; path_sink: path_sink
+    ; path_passthroughs: Passthroughs.t }
+
   val empty : t
   (** the empty trace *)
 
@@ -83,7 +89,7 @@ module type S = sig
   val passthroughs : t -> Passthroughs.t
   (** get the passthroughs of the trace *)
 
-  val get_reports : ?cur_site:CallSite.t -> t -> (path_source * path_sink * Passthroughs.t) list
+  val get_reports : ?cur_site:CallSite.t -> t -> report list
   (** get the reportable source-sink flows in this trace. specifying [cur_site] restricts the
       reported paths to ones introduced by the call at [cur_site]  *)
 

infer/src/quandary/ClangTrace.ml

@@ -277,7 +277,7 @@ include Trace.Make (struct
   module Source = CppSource
   module Sink = CppSink
 
-  let should_report source sink =
+  let get_report source sink =
     (* using this to match custom string wrappers such as folly::StringPiece *)
     let is_stringy typ =
       let lowercase_typ = String.lowercase (Typ.to_string (Typ.mk typ)) in
@@ -287,23 +287,23 @@ include Trace.Make (struct
     match (Source.kind source, Sink.kind sink) with
     | Endpoint _, BufferAccess ->
         (* untrusted data from an endpoint flowing into a buffer *)
-        true
+        Some IssueType.quandary_taint_error
     | Endpoint (_, typ), (ShellExec | SQL) ->
         (* untrusted string data flowing to shell exec/SQL *)
-        is_stringy typ
+        Option.some_if (is_stringy typ) IssueType.quandary_taint_error
     | (EnvironmentVariable | File), (BufferAccess | ShellExec | SQL) ->
         (* untrusted environment var or file data flowing to buffer or code injection *)
-        true
+        Some IssueType.quandary_taint_error
     | (Endpoint _ | EnvironmentVariable | File), Allocation ->
         (* untrusted data flowing to memory allocation *)
-        true
+        Some IssueType.quandary_taint_error
     | CommandLineFlag _, (Allocation | BufferAccess | Other | ShellExec | SQL) ->
         (* data controlled by a command line flag flowing somewhere sensitive *)
-        true
+        Some IssueType.quandary_taint_error
     | Other, _ ->
         (* Other matches everything *)
-        true
+        Some IssueType.quandary_taint_error
     | _, Other ->
-        true
+        Some IssueType.quandary_taint_error
 
 end)

infer/src/quandary/JavaTrace.ml

@@ -353,7 +353,7 @@ include Trace.Make (struct
   module Source = JavaSource
   module Sink = JavaSink
 
-  let should_report source sink =
+  let get_report source sink =
     match (Source.kind source, Sink.kind sink) with
     | PrivateData, Logging
     (* logging private data issue *)
@@ -371,17 +371,17 @@ include Trace.Make (struct
     (* create file from user-controller URI; potential path-traversal vulnerability *)
     | UserControlledString, (StartComponent | CreateIntent | JavaScript | CreateFile | HTML) ->
         (* do something sensitive with a user-controlled string *)
-        true
+        Some IssueType.quandary_taint_error
     | (Intent | UserControlledURI | UserControlledString), Deserialization ->
         (* shouldn't let anyone external control what we deserialize *)
-        true
+        Some IssueType.quandary_taint_error
     | DrawableResource _, OpenDrawableResource ->
         (* not a security issue, but useful for debugging flows from resource IDs to inflation *)
-        true
+        Some IssueType.quandary_taint_error
     | Other, _ | _, Other ->
         (* for testing purposes, Other matches everything *)
-        true
+        Some IssueType.quandary_taint_error
     | _ ->
-        false
+        None
 
 end)

infer/src/quandary/TaintAnalysis.ml

@@ -136,7 +136,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct
           final_sink
           (if is_endpoint original_path_source then ". Note: source is an endpoint." else "")
       in
-      let report_one (path_source, sink, _) =
+      let report_one {TraceDomain.issue; path_source; path_sink} =
         let open TraceDomain in
         let rec expand_source source0 ((report_acc, seen_acc) as acc) =
           let kind = Source.kind source0 in
@@ -215,7 +215,9 @@ module Make (TaintSpecification : TaintSpec.S) = struct
           | Footprint _ ->
               ([(None, path_source)], CallSite.Set.empty)
         in
-        let expanded_sinks, _ = expand_sink sink sink_indexes ([sink], CallSite.Set.empty) in
+        let expanded_sinks, _ =
+          expand_sink path_sink sink_indexes ([path_sink], CallSite.Set.empty)
+        in
         let source_trace =
           let pp_access_path_opt fmt = function
             | None ->
@@ -251,12 +253,11 @@ module Make (TaintSpecification : TaintSpec.S) = struct
               Errlog.make_trace_element 0 (CallSite.loc call_site) desc [])
             expanded_sinks
         in
-        let msg = IssueType.quandary_taint_error.unique_id in
         let _, original_path_source = List.hd_exn expanded_sources in
         let final_sink = List.hd_exn expanded_sinks in
         let trace_str = get_short_trace_string original_path_source final_sink in
         let ltr = source_trace @ List.rev sink_trace in
-        let exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in
+        let exn = Exceptions.Checkers (issue.unique_id, Localise.verbatim_desc trace_str) in
         Reporting.log_error proc_data.extras.summary ~loc:(CallSite.loc cur_site) ~ltr exn
       in
       List.iter ~f:report_one (TraceDomain.get_reports ~cur_site trace)

infer/src/unit/TaintTests.ml

@@ -39,7 +39,7 @@ module MockTrace = Trace.Make (struct
 
   end)
 
-  let should_report _ _ = false
+  let get_report _ _ = None
 end)
 
 module MockTaintAnalysis = TaintAnalysis.Make (struct

infer/src/unit/TraceTests.ml

@@ -80,8 +80,10 @@ module MockTrace = Trace.Make (struct
   module Source = MockSource
   module Sink = MockSink
 
-  let should_report source sink =
+  let get_report source sink =
-    [%compare.equal : MockTraceElem.t] (Source.kind source) (Sink.kind sink)
+    if [%compare.equal : MockTraceElem.t] (Source.kind source) (Sink.kind sink) then
+      Some IssueType.quandary_taint_error
+    else None
 
 end)
 
@@ -111,11 +113,13 @@ let tests =
       assert_equal (List.length reports) 2 ;
       assert_bool "Reports should contain source1 -> sink1"
         (List.exists
-           ~f:(fun (source, sink, _) -> source_equal source source1 && MockSink.equal sink sink1)
+           ~f:(fun {MockTrace.path_source; path_sink} ->
+             source_equal path_source source1 && MockSink.equal path_sink sink1)
            reports) ;
       assert_bool "Reports should contain source2 -> sink2"
         (List.exists
-           ~f:(fun (source, sink, _) -> source_equal source source2 && MockSink.equal sink sink2)
+           ~f:(fun {MockTrace.path_source; path_sink} ->
+             source_equal path_source source2 && MockSink.equal path_sink sink2)
            reports)
     in
     "get_reports" >:: get_reports_