From 319463b3bcf689c9d332ca68cdbad8e3bad1b8a2 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 25 Oct 2016 09:13:19 -0700 Subject: [PATCH] [quandary] propagating taint from unknown procedures and constructors Summary: Right now, taint gets lost if it flows into a constructor or procedure whose implementation is missing. Since the core Java (e.g., String) and Android classes (e.g, Intent) are among these, this is bad. We could handle this by writing a bunch of models instead, but that would be a lot of work (plus we may still miss cases). Reviewed By: jvillard Differential Revision: D4051591 fbshipit-source-id: 65851c8 --- infer/src/quandary/CppTaintAnalysis.ml | 2 +- infer/src/quandary/JavaTaintAnalysis.ml | 64 +++++++++++++++---- infer/src/quandary/TaintAnalysis.ml | 2 +- infer/src/quandary/TaintSpec.ml | 5 +- infer/src/unit/TaintTests.ml | 2 +- .../codetoanalyze/java/quandary/Makefile | 2 +- .../codetoanalyze/java/quandary/Strings.java | 35 ---------- .../java/quandary/UnknownCode.java | 46 +++++++++++++ .../codetoanalyze/java/quandary/issues.exp | 4 +- 9 files changed, 105 insertions(+), 57 deletions(-) delete mode 100644 infer/tests/codetoanalyze/java/quandary/Strings.java create mode 100644 infer/tests/codetoanalyze/java/quandary/UnknownCode.java diff --git a/infer/src/quandary/CppTaintAnalysis.ml b/infer/src/quandary/CppTaintAnalysis.ml index b73e77287..70baddfac 100644 --- a/infer/src/quandary/CppTaintAnalysis.ml +++ b/infer/src/quandary/CppTaintAnalysis.ml @@ -22,6 +22,6 @@ include | QuandarySummary.Cpp trace -> trace | _ -> assert false - let handle_unknown_call _ _ = + let handle_unknown_call _ _ _ = [] end) diff --git a/infer/src/quandary/JavaTaintAnalysis.ml b/infer/src/quandary/JavaTaintAnalysis.ml index 98574b858..c08845dd9 100644 --- a/infer/src/quandary/JavaTaintAnalysis.ml +++ b/infer/src/quandary/JavaTaintAnalysis.ml @@ -22,34 +22,70 @@ include | QuandarySummary.Java trace -> trace | _ -> assert false + let make_nth_param_ap n pname ~propagate_all = + let raw_ap = + (* base of this access path is always ignored, so type/name don't matter *) + AccessPath.of_pvar + (Pvar.mk (Mangled.from_string ("fake_param" ^ string_of_int n)) pname) Typ.Tvoid in + if propagate_all then AccessPath.Abstracted raw_ap else AccessPath.Exact raw_ap + (* propagate the trace from the nth parameter of [site.pname] to the return value of - [site.pname]. if [propagate_reachable] is true, all traces reachable from the parameter will + [site.pname]. if [propagate_all] is true, all traces reachable from the parameter will be propagated as well (e.g., for foo(x), we'll also propagate the traces associated with x.f, x.f.g, and so on) *) let propagate_nth_to_return n site ret_typ ~propagate_all = let pname = CallSite.pname site in - let dummy_param_ap = - let raw_ap = - (* base of this access path is always ignored, so type/name don't matter *) - AccessPath.of_pvar (Pvar.mk (Mangled.from_string "fake_param") pname) Typ.Tvoid in - if propagate_all then AccessPath.Abstracted raw_ap else AccessPath.Exact raw_ap in - let input = QuandarySummary.make_formal_input n dummy_param_ap in + let nth_param_ap = make_nth_param_ap n pname ~propagate_all in + let input = QuandarySummary.make_formal_input n nth_param_ap in let output = QuandarySummary.make_return_output (AccessPath.Exact (AccessPath.of_pvar (Pvar.get_ret_pvar pname) ret_typ)) in - let footprint_source = Trace.Source.make_footprint dummy_param_ap site in + let footprint_source = Trace.Source.make_footprint nth_param_ap site in let footprint_trace = Trace.of_source footprint_source in QuandarySummary.make_in_out_summary input output (to_summary_trace footprint_trace) - let handle_unknown_call site ret_typ_opt = + (* propagate the trace associated with each actual to the actual corresponding to the + constructed object *) + let propagate_to_constructor site actuals ~propagate_all = + match actuals with + | [] -> + failwithf + "Constructor %a has 0 actuals, which should never happen" + Procname.pp (CallSite.pname site) + | _ :: [] -> + (* constructor has no actuals, nothing to propagate *) + [] + | _ :: actuals -> + let pname = CallSite.pname site in + let constructor_ap = make_nth_param_ap 0 pname ~propagate_all in + let output = QuandarySummary.make_formal_output 0 constructor_ap in + let make_propagation_summary acc n _ = + let n = n + 1 in (* skip the constructor actual *) + let nth_param_ap = make_nth_param_ap n pname ~propagate_all in + let input = QuandarySummary.make_formal_input n nth_param_ap in + let footprint_source = Trace.Source.make_footprint nth_param_ap site in + let footprint_trace = Trace.of_source footprint_source in + let summary = + QuandarySummary.make_in_out_summary input output (to_summary_trace footprint_trace) in + summary :: acc in + IList.fold_lefti make_propagation_summary [] actuals + + let propagate_actuals_to_return site ret_type actuals ~propagate_all = + IList.mapi + (fun actual_num _-> propagate_nth_to_return actual_num site ret_type ~propagate_all) + actuals + + let handle_unknown_call site ret_typ_opt actuals = match CallSite.pname site with - | Procname.Java pname -> + | (Procname.Java java_pname) as pname -> begin - match Procname.java_get_class_name pname, - Procname.java_get_method pname, + match Procname.java_get_class_name java_pname, + Procname.java_get_method java_pname, ret_typ_opt with - | "java.lang.String", "valueOf", Some ret_typ -> - [propagate_nth_to_return 0 site ret_typ ~propagate_all:true] + | _ when Procname.is_constructor pname -> + propagate_to_constructor site actuals ~propagate_all:true + | _, _, Some ret_typ -> + propagate_actuals_to_return site ret_typ actuals ~propagate_all:true | _ -> [] end diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index ec96d642b..ce2e0766d 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -342,7 +342,7 @@ module Make (TaintSpec : TaintSpec.S) = struct let summary = match Summary.read_summary proc_data.tenv proc_data.pdesc callee_pname with | Some summary -> summary - | None -> TaintSpec.handle_unknown_call call_site (Option.map snd ret) in + | None -> TaintSpec.handle_unknown_call call_site (Option.map snd ret) actuals in apply_summary ret actuals summary astate_with_source proc_data call_site in Domain.join astate_acc astate_with_summary in diff --git a/infer/src/quandary/TaintSpec.ml b/infer/src/quandary/TaintSpec.ml index 6f236c9a2..f82e38093 100644 --- a/infer/src/quandary/TaintSpec.ml +++ b/infer/src/quandary/TaintSpec.ml @@ -13,8 +13,9 @@ module type S = sig module Trace : Trace.S - (** return a summary for handling an unknown call at the given site with the given return type *) - val handle_unknown_call : CallSite.t -> Typ.t option -> QuandarySummary.t + (** return a summary for handling an unknown call at the given site with the given return type + and actuals *) + val handle_unknown_call : CallSite.t -> Typ.t option -> (Exp.t * Typ.t) list -> QuandarySummary.t (** convert a trace type into a summary trace. can be killed if we functorize specs.ml *) val to_summary_trace : Trace.t -> QuandarySummary.summary_trace diff --git a/infer/src/unit/TaintTests.ml b/infer/src/unit/TaintTests.ml index cbea2c647..6d37e534a 100644 --- a/infer/src/unit/TaintTests.ml +++ b/infer/src/unit/TaintTests.ml @@ -65,7 +65,7 @@ module MockTaintAnalysis = TaintAnalysis.Make(struct let of_summary_trace _ = assert false let to_summary_trace _ = assert false - let handle_unknown_call _ _ = [] + let handle_unknown_call _ _ _ = [] end) module TestInterpreter = AnalyzerTester.Make diff --git a/infer/tests/codetoanalyze/java/quandary/Makefile b/infer/tests/codetoanalyze/java/quandary/Makefile index d875c8dec..b339685c1 100644 --- a/infer/tests/codetoanalyze/java/quandary/Makefile +++ b/infer/tests/codetoanalyze/java/quandary/Makefile @@ -20,7 +20,7 @@ FILES = \ Interprocedural.java \ LoggingPrivateData.java \ Recursion.java \ - Strings.java \ + UnknownCode.java \ compile: javac -cp $(CLASSPATH) $(FILES) diff --git a/infer/tests/codetoanalyze/java/quandary/Strings.java b/infer/tests/codetoanalyze/java/quandary/Strings.java deleted file mode 100644 index 1e7b192b5..000000000 --- a/infer/tests/codetoanalyze/java/quandary/Strings.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * 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; - -/** testing how the analysis handles strings and string manipulation functions */ - -public class Strings { - - static class Wrapper { - Object f; - } - - static void valueOfStringBad() { - Object source = InferTaint.inferSecretSource(); - String stringSource = String.valueOf(source); - InferTaint.inferSensitiveSink(stringSource); - } - - static void valueOfStringWrapperBad() { - Wrapper w = new Wrapper(); - w.f = InferTaint.inferSecretSource(); - String stringSource = String.valueOf(w.f); - InferTaint.inferSensitiveSink(stringSource); - } - -} diff --git a/infer/tests/codetoanalyze/java/quandary/UnknownCode.java b/infer/tests/codetoanalyze/java/quandary/UnknownCode.java new file mode 100644 index 000000000..355617d80 --- /dev/null +++ b/infer/tests/codetoanalyze/java/quandary/UnknownCode.java @@ -0,0 +1,46 @@ +/* + * 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; + +/** testing how the analysis handles missing/unknown code */ + +public class UnknownCode { + + native static Object id(Object o); + + public UnknownCode() {} + + static void propagateViaUnknownCodeBad() { + Object source = InferTaint.inferSecretSource(); + Object launderedSource = id(source); + InferTaint.inferSensitiveSink(launderedSource); + } + + static void propagateViaUnknownConstructorBad() { + String source = (String) InferTaint.inferSecretSource(); + // we don't analyze the code for the core Java libraries, so this constructor will be unknown + String unknownConstructor = new String(source); + InferTaint.inferSensitiveSink(unknownConstructor); + } + + static void propagateViaUnknownConstructorOk() { + String unknownConstructor = new String(""); + InferTaint.inferSensitiveSink(unknownConstructor); + } + + static void propagateViaUnknownCodeOk() { + Object notASource = new UnknownCode(); + Object launderedSource = id(notASource); + InferTaint.inferSensitiveSink(launderedSource); + } + +} diff --git a/infer/tests/codetoanalyze/java/quandary/issues.exp b/infer/tests/codetoanalyze/java/quandary/issues.exp index 54b749b29..debd385ed 100644 --- a/infer/tests/codetoanalyze/java/quandary/issues.exp +++ b/infer/tests/codetoanalyze/java/quandary/issues.exp @@ -109,5 +109,5 @@ LoggingPrivateData.java:37: ERROR: QUANDARY_TAINT_ERROR Error: SharedPreferences Recursion.java:26: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 26]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 21]) via { void Recursion.callSinkThenDiverge(Object) at [line 26] } Recursion.java:36: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 36]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 31]) via { void Recursion.safeRecursionCallSink(int,Object) at [line 36] } Recursion.java:42: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 42]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 41]) via { void Recursion.recursionBad(int,Object) at [line 42] } -Strings.java:25: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 23]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 25]) via { String String.valueOf(Object) at [line 24] } -Strings.java:32: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 30]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 32]) via { String String.valueOf(Object) at [line 31] } +UnknownCode.java:25: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 23]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 25]) via { Object UnknownCode.id(Object) at [line 24] } +UnknownCode.java:32: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 29]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 32]) via { String.(String) at [line 31] }