Expose the new proposed issue handle in issues

Summary: Let's expose the proposed issue handle when exporting an issue to json. Note that the grouping of issues is still unchanged, hence we might see an issue with multiple handles. Reviewed By: alexkassil Differential Revision: D34099564 fbshipit-source-id: 7f6eb6c8a419e6a92fe3887904ecedf107a37b9a
facebook · Feb 9, 2022 · dc58c30 · dc58c30
1 parent 8fc346b
commit dc58c30
Show file tree

Hide file tree

Showing 70 changed files with 1,099 additions and 120 deletions.
diff --git a/source/interprocedural_analyses/taint/issue.ml b/source/interprocedural_analyses/taint/issue.ml
@@ -46,6 +46,7 @@ module SinkHandle = struct
     | Return
     | LiteralStringSink of Sinks.t
     | ConditionalTestSink of Sinks.t
+  [@@deriving compare]
 
   let make_call ~call_target:{ CallGraph.CallTarget.target; index; _ } ~root =
     let root =
@@ -64,8 +65,37 @@ module SinkHandle = struct
 
   let make_global ~call_target:{ CallGraph.CallTarget.target; index; _ } =
     Global { callee = target; index }
+
+
+  let master_handle ~callable ~code sink_handle =
+    let version = 0 (* Increment the version on format change. *) in
+    let sink_handle =
+      match sink_handle with
+      | Call { callee; index; parameter } ->
+          Format.asprintf
+            "Call|%s|%d|%s"
+            (Target.external_target_name callee)
+            index
+            (AccessPath.Root.to_string parameter)
+      | Global { callee; index } ->
+          Format.asprintf "Global|%s|%d" (Target.external_target_name callee) index
+      | Return -> "Return"
+      | LiteralStringSink sink -> Format.asprintf "LiteralStringSink|%a" Sinks.pp sink
+      | ConditionalTestSink sink -> Format.asprintf "ConditionalTestSink|%a" Sinks.pp sink
+    in
+    let full_handle = Format.asprintf "%s:%d:%d:%s" callable code version sink_handle in
+    let hash = full_handle |> Digest.string |> Digest.to_hex in
+    let short_handle =
+      String.sub
+        full_handle
+        ~pos:0
+        ~len:(min (String.length full_handle) (255 - String.length hash - 1))
+    in
+    Format.asprintf "%s:%s" short_handle hash
 end
 
+module SinkHandleSet = Caml.Set.Make (SinkHandle)
+
 module SinkTreeWithHandle = struct
   type t = {
     sink_tree: BackwardState.Tree.t;
@@ -89,6 +119,7 @@ type t = {
   code: int;
   flow: Flow.t;
   location: Location.WithModule.t;
+  sink_handles: SinkHandleSet.t;
   define: Statement.Define.t Node.t;
 }
 
@@ -112,10 +143,18 @@ module Candidate = struct
   type t = {
     flows: Flow.t list;
     location: Location.WithModule.t;
+    sink_handles: SinkHandleSet.t;
   }
 
-  let join { flows = left_flows; location } { flows = right_flows; _ } =
-    { flows = List.rev_append left_flows right_flows; location }
+  let join
+      { flows = left_flows; location; sink_handles = left_sink_handles }
+      { flows = right_flows; sink_handles = right_sink_handles; _ }
+    =
+    {
+      flows = List.rev_append left_flows right_flows;
+      location;
+      sink_handles = SinkHandleSet.union left_sink_handles right_sink_handles;
+    }
 end
 
 module TriggeredSinks = struct
@@ -130,7 +169,7 @@ end
    Let F and B for forward and backward taint respectively. For each path p in B from the root to
    some node with non-empty taint T, we match T with the join of taint in the upward and downward
    closure from node at path p in F. *)
-let generate_source_sink_matches ~location ~sink_handle:_ ~source_tree ~sink_tree =
+let generate_source_sink_matches ~location ~sink_handle ~source_tree ~sink_tree =
   let make_source_sink_matches (path, sink_taint) matches =
     let source_taint =
       ForwardState.Tree.read path source_tree
@@ -148,7 +187,7 @@ let generate_source_sink_matches ~location ~sink_handle:_ ~source_tree ~sink_tre
     else
       BackwardState.Tree.fold BackwardState.Tree.Path ~init:[] ~f:make_source_sink_matches sink_tree
   in
-  { Candidate.flows; location }
+  { Candidate.flows; location; sink_handles = SinkHandleSet.singleton sink_handle }
 
 
 let compute_triggered_sinks ~triggered_sinks ~location ~sink_handle ~source_tree ~sink_tree =
@@ -207,7 +246,7 @@ module PartitionedFlow = struct
   }
 end
 
-let generate_issues ~define { Candidate.flows; location } =
+let generate_issues ~define { Candidate.flows; location; sink_handles } =
   let partitions =
     let partition { Flow.source_taint; sink_taint } =
       {
@@ -302,7 +341,7 @@ let generate_issues ~define { Candidate.flows; location } =
   let apply_rule_separate_access_path issues_so_far (rule : Rule.t) =
     let fold_partitions issues candidate =
       match apply_rule_on_flow rule candidate with
-      | Some flow -> { code = rule.code; flow; location; define } :: issues
+      | Some flow -> { code = rule.code; flow; location; sink_handles; define } :: issues
       | None -> issues
     in
     List.fold partitions ~init:issues_so_far ~f:fold_partitions
@@ -322,7 +361,7 @@ let generate_issues ~define { Candidate.flows; location } =
     if Flow.is_bottom flow then
       None
     else
-      Some { code = rule.code; flow; location; define }
+      Some { code = rule.code; flow; location; sink_handles; define }
   in
   let group_by_handle map issue =
     (* SAPP invariant: There should be a single issue per issue handle.
@@ -484,7 +523,7 @@ let to_json ~filename_lookup callable issue =
         json_features;
       ]
   in
-  let traces =
+  let traces : Yojson.Safe.json =
     `List
       [
         `Assoc ["name", `String "forward"; "roots", source_traces];
@@ -500,6 +539,14 @@ let to_json ~filename_lookup callable issue =
     Location.WithModule.instantiate ~lookup:filename_lookup issue.location
   in
   let callable_line = Ast.(Location.line issue.define.location) in
+  let master_handles =
+    SinkHandleSet.fold
+      (fun sink_handle handles ->
+        `String (SinkHandle.master_handle ~callable:callable_name ~code:issue.code sink_handle)
+        :: handles)
+      issue.sink_handles
+      []
+  in
   `Assoc
     [
       "callable", `String callable_name;
@@ -512,6 +559,7 @@ let to_json ~filename_lookup callable issue =
       "message", `String message;
       "traces", traces;
       "features", `List json_features;
+      "master_handles", `List master_handles;
     ]
 
 

diff --git a/source/interprocedural_analyses/taint/issue.mli b/source/interprocedural_analyses/taint/issue.mli
@@ -44,6 +44,8 @@ module SinkHandle : sig
   val make_global : call_target:CallGraph.CallTarget.t -> t
 end
 
+module SinkHandleSet : Caml.Set.S with type elt = SinkHandle.t
+
 module SinkTreeWithHandle : sig
   type t = {
     sink_tree: BackwardState.Tree.t;
@@ -60,6 +62,7 @@ type t = {
   code: int;
   flow: Flow.t;
   location: Location.WithModule.t;
+  sink_handles: SinkHandleSet.t;
   (* Only used to create the Pyre errors. *)
   define: Ast.Statement.Define.t Ast.Node.t;
 }
@@ -75,6 +78,7 @@ module Candidate : sig
   type t = {
     flows: Flow.t list;
     location: Location.WithModule.t;
+    sink_handles: SinkHandleSet.t;
   }
 
   val join : t -> t -> t

diff --git a/source/interprocedural_analyses/taint/test/integration/add_feature_to_argument.py.models b/source/interprocedural_analyses/taint/test/integration/add_feature_to_argument.py.models
@@ -63,6 +63,9 @@
       { "always-via": "string_concat_lhs" },
       { "always-via": "special_source" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "add_feature_to_argument.test_add_feature_in_comprehension:5002:0:Call|_test_sink|0|formal(arg):e660dc46ee53a95cdb86c321f40be0a3"
     ]
   }
 }

diff --git a/source/interprocedural_analyses/taint/test/integration/always_in_none.py.models b/source/interprocedural_analyses/taint/test/integration/always_in_none.py.models
@@ -140,6 +140,9 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_sink" } ]
+    "features": [ { "always-via": "special_sink" } ],
+    "master_handles": [
+      "always_in_none.test:5002:0:Call|_test_sink|0|formal(arg):1168c6477767d16ce96c21f36d6fcbba"
+    ]
   }
 }
diff --git a/source/interprocedural_analyses/taint/test/integration/applies_to_index.py.models b/source/interprocedural_analyses/taint/test/integration/applies_to_index.py.models
@@ -61,6 +61,9 @@
       { "has": "first-index" },
       { "first-index": "a" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "applies_to_index.issue_only_with_a_key:5002:0:Call|_test_sink|0|formal(arg):76c5bfa4202be200a69fe5dce0318d74"
     ]
   }
 }
@@ -118,7 +121,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_sink" } ]
+    "features": [ { "always-via": "special_sink" } ],
+    "master_handles": [
+      "applies_to_index.issue_only_with_first:5002:0:Call|_test_sink|0|formal(arg):1b20364f53d3dcf69a13dac6b9528240"
+    ]
   }
 }
 {
@@ -175,7 +181,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_sink" } ]
+    "features": [ { "always-via": "special_sink" } ],
+    "master_handles": [
+      "applies_to_index.issue_only_with_second:5002:0:Call|_test_sink|1|formal(arg):c5de4b94ea7f50b90ee29115d6498c79"
+    ]
   }
 }
 {
@@ -240,6 +249,9 @@
       { "has": "first-field" },
       { "first-field": "a" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "applies_to_index.issue_with_member:5002:0:Call|_test_sink|0|formal(arg):4e6c314b245358eeea77b64cace8721b"
     ]
   }
 }
@@ -326,7 +338,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_sink" } ]
+    "features": [ { "always-via": "special_sink" } ],
+    "master_handles": [
+      "applies_to_index.issue_only_with_nested_first:5002:0:Call|_test_sink|0|formal(arg):12cbde4c9bc98dd60c8e3b1c36ce01b2"
+    ]
   }
 }
 {

diff --git a/source/interprocedural_analyses/taint/test/integration/attach_features.py.models b/source/interprocedural_analyses/taint/test/integration/attach_features.py.models
@@ -141,6 +141,9 @@
     "features": [
       { "always-via": "string_concat_lhs" },
       { "always-via": "special_source" }
+    ],
+    "master_handles": [
+      "attach_features.attach_to_returned_sink:5002:0:Return:8218e4adbc4250811127e266f8a13911"
     ]
   }
 }

diff --git a/source/interprocedural_analyses/taint/test/integration/attributes.py.models b/source/interprocedural_analyses/taint/test/integration/attributes.py.models
@@ -51,7 +51,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_source" } ]
+    "features": [ { "always-via": "special_source" } ],
+    "master_handles": [
+      "attributes.test_attribute_union_sink:5002:0:Global|Obj{attributes.Sink.token}|1:cb5576984115ab71838ed9c0be98c643"
+    ]
   }
 }
 {
@@ -106,7 +109,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_source" } ]
+    "features": [ { "always-via": "special_source" } ],
+    "master_handles": [
+      "attributes.test_attribute_union_sink:5002:0:Global|Obj{attributes.Sink.token}|0:14935f90c85d01ed835cdc0a92c85dbf"
+    ]
   }
 }
 {
@@ -174,6 +180,9 @@
       { "always-via": "issue-broadening" },
       { "always-via": "special_source" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "attributes.test_attribute_via_dunder_dict:5002:0:Call|_test_sink|0|formal(arg):62e7b46583720a9a3834cad383fbad3f"
     ]
   }
 }
@@ -242,6 +251,9 @@
       { "always-via": "tito" },
       { "always-via": "special_source" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "attributes.test_attribute_via_dunder_dict:5002:0:Call|_test_sink|1|formal(arg):da5fc540abbf310d75dd68aa49edc906"
     ]
   }
 }
@@ -302,6 +314,9 @@
     "features": [
       { "always-via": "tito" },
       { "always-via": "special_source" }
+    ],
+    "master_handles": [
+      "attributes.test_issue_with_update_to_self_attribute:5002:0:Global|Obj{attributes.D.buffer}|0:a87babfa59527e2779c9625bae05d282"
     ]
   }
 }
@@ -606,7 +621,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_sink" } ]
+    "features": [ { "always-via": "special_sink" } ],
+    "master_handles": [
+      "attributes.test_attribute_union_source:5002:0:Call|_test_sink|1|formal(arg):a2bd11fa9758293fdf005fbfa793a61a"
+    ]
   }
 }
 {
@@ -661,7 +679,10 @@
         ]
       }
     ],
-    "features": [ { "always-via": "special_sink" } ]
+    "features": [ { "always-via": "special_sink" } ],
+    "master_handles": [
+      "attributes.test_attribute_union_source:5002:0:Call|_test_sink|0|formal(arg):3e7e5ecb5638879f3585847c1274f5e1"
+    ]
   }
 }
 {
@@ -889,6 +910,9 @@
       { "has": "first-index" },
       { "first-index": "text" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "attributes.test_issue_with_text_key_of_dictionary:5002:0:Call|_test_sink|0|formal(arg):07aaa6860d20d5f97b7ef18d4f0ad4f6"
     ]
   }
 }

diff --git a/source/interprocedural_analyses/taint/test/integration/binary_operators.py.models b/source/interprocedural_analyses/taint/test/integration/binary_operators.py.models
@@ -60,6 +60,9 @@
     "features": [
       { "always-via": "special_source" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "binary_operators.test1:5002:0:Call|binary_operators.Add.__add__|0|formal(other):e6a103bbaecc137e992946ecaead11e6"
     ]
   }
 }
@@ -124,6 +127,9 @@
     "features": [
       { "always-via": "special_source" },
       { "always-via": "special_sink" }
+    ],
+    "master_handles": [
+      "binary_operators.test2:5002:0:Call|binary_operators.Add.__add__|0|formal(other):5ef261b25d55ce37fd84795645c4c318"
     ]
   }
 }