From e8b61f6dbbc11b8c23a9fb426d32fddd18ebefce Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 9 Nov 2016 16:57:30 -0800 Subject: [PATCH] [quandary] fix false positives from procedures that are both sources and sinks Summary: If a procedure is both a source and a sink for the same value, and it's a sink first, you will get a false positive when applying the summary for the procedure. Reviewed By: cristianoc Differential Revision: D4145246 fbshipit-source-id: 97f0022 --- infer/src/quandary/TaintAnalysis.ml | 22 +++++++++++++------ .../java/quandary/Interprocedural.java | 19 ++++++++++++++++ .../codetoanalyze/java/quandary/issues.exp | 2 ++ 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 75294a4f6..7bf44d5ca 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -172,15 +172,23 @@ module Make (TaintSpec : TaintSpec.S) = struct | [] -> trace | paths -> - let caller_pname = Cfg.Procdesc.get_proc_name proc_data.pdesc in + let report_error path = + let caller_pname = Cfg.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 exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in + Reporting.log_error caller_pname ~loc:(CallSite.loc callee_site) exn in + let reported_sinks = IList.map - (fun ((_, _, sinks) as path) -> - let msg = Localise.to_string Localise.quandary_taint_error in - let trace_str = F.asprintf "%a" pp_path_short path in - let exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in - Reporting.log_error caller_pname ~loc:(CallSite.loc callee_site) exn; - fst (IList.hd (IList.rev sinks))) + (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. *) diff --git a/infer/tests/codetoanalyze/java/quandary/Interprocedural.java b/infer/tests/codetoanalyze/java/quandary/Interprocedural.java index 75404b04b..d67dced6e 100644 --- a/infer/tests/codetoanalyze/java/quandary/Interprocedural.java +++ b/infer/tests/codetoanalyze/java/quandary/Interprocedural.java @@ -295,4 +295,23 @@ 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 e9308d0cc..16696925e 100644 --- a/infer/tests/codetoanalyze/java/quandary/issues.exp +++ b/infer/tests/codetoanalyze/java/quandary/issues.exp @@ -103,6 +103,8 @@ Interprocedural.java:228: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object Interp Interprocedural.java:239: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 237]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 239]) Interprocedural.java:251: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 251]) -> Other(void Interprocedural.callSinkVariadic(java.lang.Object[]) at [line 251]) Interprocedural.java:262: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 260]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 262]) +Interprocedural.java:309: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 309]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 309]) +Interprocedural.java:314: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 313]) -> Other(void Interprocedural.sourceAndSink(Interprocedural$Obj) at [line 314]) LoggingPrivateData.java:57: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(String TelephonyManager.getDeviceId() at [line 40]) -> Logging(int Log.e(String,String) at [line 57]) (via { String String.valueOf(double) at [line 25], String String.valueOf(double) at [line 31], String String.valueOf(double) at [line 34], String String.valueOf(float) at [line 28], String String.valueOf(float) at [line 37] }) LoggingPrivateData.java:57: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(String TelephonyManager.getLine1Number() at [line 43]) -> Logging(int Log.e(String,String) at [line 57]) (via { String String.valueOf(double) at [line 25], String String.valueOf(double) at [line 31], String String.valueOf(double) at [line 34], String String.valueOf(float) at [line 28], String String.valueOf(float) at [line 37] }) LoggingPrivateData.java:57: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(String TelephonyManager.getSimSerialNumber() at [line 46]) -> Logging(int Log.e(String,String) at [line 57]) (via { String String.valueOf(double) at [line 25], String String.valueOf(double) at [line 31], String String.valueOf(double) at [line 34], String String.valueOf(float) at [line 28], String String.valueOf(float) at [line 37] })