From 97bf3324c82e7add30cb459b2a1ed7f466a96e8c Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Mon, 5 Jun 2017 19:22:56 -0700 Subject: [PATCH] [quandary] add indexes to sinks Summary: First step toward addressing bad traces that happen in examples like ``` void sourceMethod() { Obj source = (Obj) InferTaint.inferSecretSource(); callSameSink(null, source); // index: 1 } void callSameSink(Obj o1, Obj o2) { callMySink1(o1); // flows via o1 ~= index 0, don't expand callMySink2(o2); // flows via o2 ~= index 1, can expand } void callMySink1(Obj o) { ... // maybe interesting something happens here that doesn't happen in callMySink2 InferTaint.inferSensitiveSink(o); // flows via o ~= index 0, can expand } void callMySink2(Obj o) { InferTaint.inferSensitiveSink(o); // flows via o ~= index 0, can expand } ``` The issue is that when we recreate a trace to the sink starting from `sourceMethod`, we don't know which of the calls to `callMySink` to expand/include in the trace. If we expand the call to `callMySink(o1)`, we'll get a bogus trace. In this example that's not such a big deal, but imagine the case where the first call to `callMySink` is a different function that transitively calls the sink through some long and confusing path. Remembering the index at which taint flows into each sink will let us choose which sinks are safe to expand. This diff just adds indexes to the API; it's not actually propagating the index info or using it during expansion yet. Reviewed By: jeremydubreil Differential Revision: D5170563 fbshipit-source-id: ba4b096 --- infer/src/checkers/Sink.ml | 39 ++++++-------------- infer/src/checkers/Sink.mli | 17 +++------ infer/src/checkers/SinkTrace.ml | 4 +- infer/src/checkers/SiofTrace.ml | 2 +- infer/src/checkers/Source.ml | 4 +- infer/src/checkers/ThreadSafetyDomain.ml | 2 +- infer/src/checkers/TraceElem.ml | 2 +- infer/src/quandary/ClangTrace.ml | 16 ++++---- infer/src/quandary/JavaTrace.ml | 47 +++++++++++------------- infer/src/quandary/TaintAnalysis.ml | 21 ++++++----- infer/src/unit/TaintTests.ml | 6 ++- infer/src/unit/TraceTests.ml | 4 +- 12 files changed, 72 insertions(+), 92 deletions(-) diff --git a/infer/src/checkers/Sink.ml b/infer/src/checkers/Sink.ml index 9fd47b262..ebd6969de 100644 --- a/infer/src/checkers/Sink.ml +++ b/infer/src/checkers/Sink.ml @@ -15,23 +15,15 @@ module L = Logging module type Kind = sig include TraceElem.Kind - (** return the parameter index and sink kind for the given call site with the given actuals *) - val get : Typ.Procname.t -> HilExp.t list -> Tenv.t -> (t * int) list + val get : Typ.Procname.t -> HilExp.t list -> Tenv.t -> (t * IntSet.t) option end module type S = sig include TraceElem.S - type parameter = - { - sink : t; - (** sink type of the parameter *) - index : int; - (** index of the parameter *) - } - - (** return the parameter index and sink kind for the given call site with the given actuals *) - val get : CallSite.t -> HilExp.t list -> Tenv.t -> parameter list + val get : CallSite.t -> HilExp.t list -> Tenv.t -> t option + + val indexes : t -> IntSet.t end module Make (Kind : Kind) = struct @@ -41,32 +33,25 @@ module Make (Kind : Kind) = struct { kind : Kind.t; site : CallSite.t; + indexes : IntSet.t; } [@@deriving compare] - type parameter = - { - sink : t; - (** sink type of the parameter *) - index : int; - (** index of the parameter *) - } - let kind t = t.kind let call_site t = t.site - let make kind site = - { kind; site; } + let indexes t = + t.indexes - let make_sink_param sink index = - { sink; index; } + let make ?(indexes=IntSet.empty) kind site = + { kind; site; indexes; } let get site actuals tenv = - List.map - ~f:(fun (kind, index) -> make_sink_param (make kind site) index) - (Kind.get (CallSite.pname site) actuals tenv) + match Kind.get (CallSite.pname site) actuals tenv with + | Some (kind, indexes) -> Some { kind; site; indexes; } + | None -> None let with_callsite t callee_site = { t with site = callee_site; } diff --git a/infer/src/checkers/Sink.mli b/infer/src/checkers/Sink.mli index 2f9c2800a..5e985e823 100644 --- a/infer/src/checkers/Sink.mli +++ b/infer/src/checkers/Sink.mli @@ -12,23 +12,18 @@ open! IStd module type Kind = sig include TraceElem.Kind - (** return the parameter index and sink kind for the given call site with the given actuals *) - val get : Typ.Procname.t -> HilExp.t list -> Tenv.t -> (t * int) list + (** return Some kind if the given procname/actuals are a sink, None otherwise *) + val get : Typ.Procname.t -> HilExp.t list -> Tenv.t -> (t * IntSet.t) option end module type S = sig include TraceElem.S - type parameter = - { - sink : t; - (** sink type of the parameter *) - index : int; - (** index of the parameter *) - } + (** return Some sink if the given call site/actuals are a sink, None otherwise *) + val get : CallSite.t -> HilExp.t list -> Tenv.t -> t option - (** return the parameter index and sink kind for the given call site with the given actuals *) - val get : CallSite.t -> HilExp.t list -> Tenv.t -> parameter list + (** return the indexes where taint can flow into the sink *) + val indexes : t -> IntSet.t end module Make (Kind : Kind) : S with module Kind = Kind diff --git a/infer/src/checkers/SinkTrace.ml b/infer/src/checkers/SinkTrace.ml index bdaac9e87..ab305bacc 100644 --- a/infer/src/checkers/SinkTrace.ml +++ b/infer/src/checkers/SinkTrace.ml @@ -32,9 +32,9 @@ end module MakeSink(TraceElem : TraceElem.S) = struct include TraceElem - type parameter = { sink : t; index : int; } - let get _ _ _ = [] + let get _ _ _ = None + let indexes _ = IntSet.empty end module Make (TraceElem : TraceElem.S) = struct diff --git a/infer/src/checkers/SiofTrace.ml b/infer/src/checkers/SiofTrace.ml index bf6f06ba0..2f3904bfd 100644 --- a/infer/src/checkers/SiofTrace.ml +++ b/infer/src/checkers/SiofTrace.ml @@ -35,7 +35,7 @@ module TraceElem = struct let kind { kind; } = snd kind - let make kind site = { kind = (`Call, kind); site; } + let make ?indexes:_ kind site = { kind = (`Call, kind); site; } let with_callsite { kind=(_, kind); } site = { kind=(`Call, kind); site; } diff --git a/infer/src/checkers/Source.ml b/infer/src/checkers/Source.ml index df4dc1e93..98e9d1053 100644 --- a/infer/src/checkers/Source.ml +++ b/infer/src/checkers/Source.ml @@ -85,7 +85,7 @@ module Make (Kind : Kind) = struct | Normal kind -> kind | Footprint _ -> Kind.unknown - let make kind site = + let make ?indexes:_ kind site = { site; kind = Normal kind; } let make_footprint ap pdesc = @@ -135,7 +135,7 @@ module Dummy = struct let kind t = t - let make kind _ = kind + let make ?indexes:_ kind _ = kind let pp _ () = () diff --git a/infer/src/checkers/ThreadSafetyDomain.ml b/infer/src/checkers/ThreadSafetyDomain.ml index dec63da68..6bbd25804 100644 --- a/infer/src/checkers/ThreadSafetyDomain.ml +++ b/infer/src/checkers/ThreadSafetyDomain.ml @@ -48,7 +48,7 @@ module TraceElem = struct let kind { kind; } = kind - let make kind site = { kind; site; } + let make ?indexes:_ kind site = { kind; site; } let with_callsite t site = { t with site; } diff --git a/infer/src/checkers/TraceElem.ml b/infer/src/checkers/TraceElem.ml index d1598fd3b..70f976e52 100644 --- a/infer/src/checkers/TraceElem.ml +++ b/infer/src/checkers/TraceElem.ml @@ -25,7 +25,7 @@ module type S = sig val call_site : t -> CallSite.t val kind : t -> Kind.t - val make : Kind.t -> CallSite.t -> t + val make : ?indexes:IntSet.t -> Kind.t -> CallSite.t -> t val with_callsite : t -> CallSite.t -> t val pp : F.formatter -> t -> unit diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index 9663b0936..a4019eb19 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -128,10 +128,10 @@ module SinkKind = struct (* taint the nth parameter (0-indexed) *) let taint_nth n kind = - [kind, n] + Some (kind, IntSet.singleton n) let taint_all actuals kind = - List.mapi ~f:(fun actual_num _ -> kind, actual_num) actuals + Some (kind, IntSet.of_list (List.mapi ~f:(fun actual_num _ -> actual_num) actuals)) (* return Some(sink kind) if [procedure_name] is in the list of externally specified sinks *) let get_external_sink pname actuals = @@ -143,10 +143,10 @@ module SinkKind = struct let kind = of_string kind in try let n = int_of_string index in - Some (taint_nth n kind) + taint_nth n kind with Failure _ -> (* couldn't parse the index, just taint everything *) - Some (taint_all actuals kind) + taint_all actuals kind else None) external_sinks @@ -154,7 +154,7 @@ module SinkKind = struct let get pname actuals _ = match pname with | Typ.Procname.ObjC_Cpp _ -> - Option.value (get_external_sink pname actuals) ~default:[] + get_external_sink pname actuals | Typ.Procname.C _ -> begin match Typ.Procname.to_string pname with @@ -163,12 +163,12 @@ module SinkKind = struct | "brk" | "calloc" | "malloc" | "realloc" | "sbrk" -> taint_all actuals Allocation | _ -> - Option.value (get_external_sink pname actuals) ~default:[] + get_external_sink pname actuals end | Typ.Procname.Block _ -> - [] + None | pname when BuiltinDecl.is_declared pname -> - [] + None | pname -> failwithf "Non-C++ procname %a in C++ analysis@." Typ.Procname.pp pname diff --git a/infer/src/quandary/JavaTrace.ml b/infer/src/quandary/JavaTrace.ml index 80681e76c..cc1b0ae64 100644 --- a/infer/src/quandary/JavaTrace.ml +++ b/infer/src/quandary/JavaTrace.ml @@ -218,13 +218,14 @@ module SinkKind = struct if Typ.Procname.java_is_static pname || taint_this then actuals, 0 else List.tl_exn actuals, 1 in - List.mapi - ~f:(fun param_num _ -> kind, param_num + offset) - actuals_to_taint in + let indexes = + IntSet.of_list (List.mapi ~f:(fun param_num _ -> param_num + offset) actuals_to_taint) in + Some (kind, indexes) in + (* taint the nth non-"this" parameter (0-indexed) *) let taint_nth n kind = let first_index = if Typ.Procname.java_is_static pname then n else n + 1 in - [kind, first_index] in + Some (kind, IntSet.singleton first_index) in match pname with | Typ.Procname.Java java_pname -> begin @@ -237,17 +238,17 @@ module SinkKind = struct | "java.nio.file.Paths", "get" -> taint_all CreateFile | "com.facebook.infer.builtins.InferTaint", "inferSensitiveSink" -> - [Other, 0] + taint_nth 0 Other | class_name, method_name -> let taint_matching_supertype typename _ = match Typ.Name.name typename, method_name with | "android.app.Activity", ("startActivityFromChild" | "startActivityFromFragment") -> - Some (taint_nth 1 StartComponent) + taint_nth 1 StartComponent | "android.app.Activity", "startIntentSenderForResult" -> - Some (taint_nth 2 StartComponent) + taint_nth 2 StartComponent | "android.app.Activity", "startIntentSenderFromChild" -> - Some (taint_nth 3 StartComponent) + taint_nth 3 StartComponent | "android.content.Context", ("bindService" | "sendBroadcast" | @@ -265,9 +266,9 @@ module SinkKind = struct "startNextMatchingActivity" | "startService" | "stopService") -> - Some (taint_nth 0 StartComponent) + taint_nth 0 StartComponent | "android.content.Context", "startIntentSender" -> - Some (taint_nth 1 StartComponent) + taint_nth 1 StartComponent | "android.content.Intent", ("parseUri" | "getIntent" | @@ -278,9 +279,9 @@ module SinkKind = struct "setDataAndType" | "setDataAndTypeAndNormalize" | "setPackage") -> - Some (taint_nth 0 CreateIntent) + taint_nth 0 CreateIntent | "android.content.Intent", "setClassName" -> - Some (taint_all CreateIntent) + taint_all CreateIntent | "android.webkit.WebView", ("evaluateJavascript" | "loadData" | @@ -288,7 +289,7 @@ module SinkKind = struct "loadUrl" | "postUrl" | "postWebMessage") -> - Some (taint_all JavaScript) + taint_all JavaScript | class_name, method_name -> (* check the list of externally specified sinks *) let procedure = class_name ^ "." ^ method_name in @@ -299,25 +300,19 @@ module SinkKind = struct let kind = of_string kind in try let n = int_of_string index in - Some (taint_nth n kind) + taint_nth n kind with Failure _ -> (* couldn't parse the index, just taint everything *) - Some (taint_all kind) + taint_all kind else None) external_sinks in - begin - match - PatternMatch.supertype_find_map_opt - tenv - taint_matching_supertype - (Typ.Name.Java.from_string class_name) with - | Some sinks -> sinks - | None -> [] - end - + PatternMatch.supertype_find_map_opt + tenv + taint_matching_supertype + (Typ.Name.Java.from_string class_name) end - | pname when BuiltinDecl.is_declared pname -> [] + | pname when BuiltinDecl.is_declared pname -> None | pname -> failwithf "Non-Java procname %a in Java analysis@." Typ.Procname.pp pname let pp fmt kind = diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index e4d08da4e..85ad09e53 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -158,16 +158,16 @@ module Make (TaintSpecification : TaintSpec.S) = struct List.iter ~f:report_error (TraceDomain.get_reportable_paths ~cur_site trace ~trace_of_pname) - let add_sinks sinks actuals access_tree proc_data callee_site = + let add_sink sink actuals access_tree proc_data callee_site = (* add [sink] to the trace associated with the [formal_index]th actual *) - let add_sink_to_actual access_tree_acc (sink_param : TraceDomain.Sink.parameter) = - match List.nth_exn actuals sink_param.index with + let add_sink_to_actual sink_index access_tree_acc = + match List.nth_exn actuals sink_index with | HilExp.AccessPath actual_ap_raw -> let actual_ap = AccessPath.Abstracted actual_ap_raw in begin match access_path_get_node actual_ap access_tree_acc proc_data with | Some (actual_trace, _) -> - let actual_trace' = TraceDomain.add_sink sink_param.sink actual_trace in + let actual_trace' = TraceDomain.add_sink sink actual_trace in report_trace actual_trace' callee_site proc_data; TaintDomain.add_trace actual_ap actual_trace' access_tree_acc | None -> @@ -175,7 +175,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct end | _ -> access_tree_acc in - List.fold ~f:add_sink_to_actual ~init:access_tree sinks + IntSet.fold add_sink_to_actual (TraceDomain.Sink.indexes sink) access_tree let apply_summary ret_opt @@ -382,10 +382,11 @@ module Make (TaintSpecification : TaintSpec.S) = struct let analyze_call astate_acc callee_pname = let call_site = CallSite.make callee_pname callee_loc in - let sinks = TraceDomain.Sink.get call_site actuals proc_data.ProcData.tenv in - let astate_with_sink = match sinks with - | [] -> astate - | sinks -> add_sinks sinks actuals astate proc_data call_site in + let sink = TraceDomain.Sink.get call_site actuals proc_data.ProcData.tenv in + let astate_with_sink = + match sink with + | Some sink -> add_sink sink actuals astate proc_data call_site + | None -> astate in let source = TraceDomain.Source.get call_site proc_data.tenv in let astate_with_source = match source with @@ -400,7 +401,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct astate_with_sink in let astate_with_summary = - if sinks <> [] || Option.is_some source + if Option.is_some source || Option.is_some sink then (* don't use a summary for a procedure that is a direct source or sink *) astate_with_source diff --git a/infer/src/unit/TaintTests.ml b/infer/src/unit/TaintTests.ml index 0c60fa2fa..1266664b1 100644 --- a/infer/src/unit/TaintTests.ml +++ b/infer/src/unit/TaintTests.ml @@ -32,8 +32,10 @@ module MockTrace = Trace.Make(struct let get pname _ _ = if String.is_prefix ~prefix:"SINK" (Typ.Procname.to_string pname) - then [CallSite.make pname Location.dummy, 0] - else [] + then Some (CallSite.make pname Location.dummy, IntSet.singleton 0) + else None + + let indexes _ = IntSet.empty end) let should_report _ _ = false diff --git a/infer/src/unit/TraceTests.ml b/infer/src/unit/TraceTests.ml index ddb7fc8be..01b3ee1bb 100644 --- a/infer/src/unit/TraceTests.ml +++ b/infer/src/unit/TraceTests.ml @@ -26,7 +26,7 @@ module MockTraceElem = struct let kind t = t - let make kind _ = kind + let make ?indexes:_ kind _ = kind let pp fmt = function | Kind1 -> F.fprintf fmt "Kind1" @@ -67,6 +67,8 @@ module MockSink = struct let get _ = assert false + let indexes _ = IntSet.empty + let equal = [%compare.equal : t] end