From 0972c8d262800cb8db60feab8e6e532e1eface5e Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 1 Dec 2016 10:48:39 -0800 Subject: [PATCH] [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 | 7 -- infer/src/checkers/Trace.mli | 3 - infer/src/quandary/TaintAnalysis.ml | 72 +++++++++---------- .../java/quandary/FlowSensitivity.java | 69 ++++++++++++++++++ .../java/quandary/Interprocedural.java | 19 ----- .../codetoanalyze/java/quandary/issues.exp | 5 +- 6 files changed, 108 insertions(+), 67 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/quandary/FlowSensitivity.java diff --git a/infer/src/checkers/Trace.ml b/infer/src/checkers/Trace.ml index aa07f4262..c6dbb068b 100644 --- a/infer/src/checkers/Trace.ml +++ b/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 diff --git a/infer/src/checkers/Trace.mli b/infer/src/checkers/Trace.mli index 4ad71fb55..e55977652 100644 --- a/infer/src/checkers/Trace.mli +++ b/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 diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 28131f38e..59d92f886 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/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 diff --git a/infer/tests/codetoanalyze/java/quandary/FlowSensitivity.java b/infer/tests/codetoanalyze/java/quandary/FlowSensitivity.java new file mode 100644 index 000000000..4bba780b8 --- /dev/null +++ b/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); + } + +} diff --git a/infer/tests/codetoanalyze/java/quandary/Interprocedural.java b/infer/tests/codetoanalyze/java/quandary/Interprocedural.java index d67dced6e..75404b04b 100644 --- a/infer/tests/codetoanalyze/java/quandary/Interprocedural.java +++ b/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); - } - } diff --git a/infer/tests/codetoanalyze/java/quandary/issues.exp b/infer/tests/codetoanalyze/java/quandary/issues.exp index 6cf65f358..1c225cde7 100644 --- a/infer/tests/codetoanalyze/java/quandary/issues.exp +++ b/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)]