From 38a336694a95d919bbe3c6fdb4c44aeaa5c7f60e Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 1 Feb 2017 10:23:47 -0800 Subject: [PATCH] [quandary] improve taint propagation for unknown calls Summary: When the receiver type and return type of an unknown call are the same, propagate taint to both the receiver and the return type. This does the right thing for common "builder-style" methods that both update and return the receiver. We already had custom models for a few such methods (e.g., `StringBuilder.append`), but we can remove them now. Reviewed By: jeremydubreil Differential Revision: D4490071 fbshipit-source-id: 325ea88 --- infer/src/quandary/ClangTaintAnalysis.ml | 2 +- infer/src/quandary/JavaTaintAnalysis.ml | 29 ++++++++++++++++-------- infer/src/quandary/TaintAnalysis.ml | 14 +++++------- infer/src/quandary/TaintSpec.ml | 3 ++- infer/src/unit/TaintTests.ml | 2 +- 5 files changed, 30 insertions(+), 20 deletions(-) diff --git a/infer/src/quandary/ClangTaintAnalysis.ml b/infer/src/quandary/ClangTaintAnalysis.ml index 3789102d0..8f47f15fa 100644 --- a/infer/src/quandary/ClangTaintAnalysis.ml +++ b/infer/src/quandary/ClangTaintAnalysis.ml @@ -23,6 +23,6 @@ include | QuandarySummary.AccessTree.Clang tree -> tree | _ -> 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 c794a7629..ce96b5598 100644 --- a/infer/src/quandary/JavaTaintAnalysis.ml +++ b/infer/src/quandary/JavaTaintAnalysis.ml @@ -23,25 +23,36 @@ include | QuandarySummary.AccessTree.Java access_tree -> access_tree | _ -> assert false - let handle_unknown_call pname ret_typ_opt = + let handle_unknown_call pname ret_typ_opt actuals = + let types_match typ class_string = match typ with + | Typ.Tptr (Tstruct typename, _) -> String.equal (Typename.name typename) class_string + | _ -> false in match pname with | (Procname.Java java_pname) as pname -> + let is_static = Procname.java_is_static pname in begin match Procname.java_get_class_name java_pname, Procname.java_get_method java_pname, ret_typ_opt with | _ when Procname.is_constructor pname -> [TaintSpec.Propagate_to_receiver] - | ("java.lang.StringBuffer" | "java.lang.StringBuilder" | "java.util.Formatter"), _, - Some _ - when not (Procname.java_is_static pname) -> - [TaintSpec.Propagate_to_receiver; TaintSpec.Propagate_to_return] - | _, _, (Some Typ.Tvoid | None) when not (Procname.java_is_static pname) -> + | _, _, (Some Typ.Tvoid | None) when not is_static -> (* for instance methods with no return value, propagate the taint to the receiver *) [TaintSpec.Propagate_to_receiver] - | _, _, Some _ -> - [TaintSpec.Propagate_to_return] - | _, _, None -> + | classname, _, Some (Typ.Tptr _ | Tstruct _) -> + begin + match actuals with + | (_, receiver_typ) :: _ + when not is_static && types_match receiver_typ classname -> + (* if the receiver and return type are the same, propagate to both. we're + assuming the call is one of the common "builder-style" methods that both + updates and returns the receiver *) + [TaintSpec.Propagate_to_receiver; TaintSpec.Propagate_to_return] + | _ -> + (* receiver doesn't match return type; just propagate to the return type *) + [TaintSpec.Propagate_to_return] + end + | _ -> [] end | pname when BuiltinDecl.is_declared pname -> diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 7cdcb763c..b0fb5c72f 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -399,21 +399,19 @@ module Make (TaintSpecification : TaintSpec.S) = struct | TaintSpec.Propagate_to_receiver, (receiver_exp, receiver_typ) :: (_ :: _ as other_actuals), _ -> - let receiver_ap = + begin match AccessPath.of_lhs_exp receiver_exp receiver_typ ~f_resolve_id with | Some ap -> - AccessPath.Exact ap + propagate_to_access_path (AccessPath.Exact ap) other_actuals astate_acc | None -> - failwithf - "Receiver for called procedure %a does not have an access path" - Procname.pp - callee_pname in - propagate_to_access_path receiver_ap other_actuals astate_acc + (* this can happen when (for example) the receiver is a string literal *) + astate_acc + end | _ -> astate_acc in let propagations = - TaintSpecification.handle_unknown_call callee_pname (Option.map ~f:snd ret) in + TaintSpecification.handle_unknown_call callee_pname (Option.map ~f:snd ret) actuals in IList.fold_left handle_unknown_call_ astate propagations in let analyze_call astate_acc callee_pname = diff --git a/infer/src/quandary/TaintSpec.ml b/infer/src/quandary/TaintSpec.ml index c529c1a25..2d6c18e59 100644 --- a/infer/src/quandary/TaintSpec.ml +++ b/infer/src/quandary/TaintSpec.ml @@ -22,7 +22,8 @@ module type S = sig (** return a summary for handling an unknown call at the given site with the given return type and actuals *) - val handle_unknown_call : Procname.t -> Typ.t option -> handle_unknown list + val handle_unknown_call : + Procname.t -> Typ.t option -> (Exp.t * Typ.t) list -> handle_unknown list val to_summary_access_tree : AccessTree.t -> QuandarySummary.AccessTree.t diff --git a/infer/src/unit/TaintTests.ml b/infer/src/unit/TaintTests.ml index 1f5c81e63..d13d52336 100644 --- a/infer/src/unit/TaintTests.ml +++ b/infer/src/unit/TaintTests.ml @@ -45,7 +45,7 @@ module MockTaintAnalysis = TaintAnalysis.Make(struct let of_summary_access_tree _ = assert false let to_summary_access_tree _ = assert false - let handle_unknown_call _ _ = [] + let handle_unknown_call _ _ _ = [] end) module TestInterpreter = AnalyzerTester.Make