From 2d22b631c32cb7f9381186daf1a0c082fc86b51b Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 17 Oct 2017 04:17:48 -0700 Subject: [PATCH] [quandary] track flow of `Drawable` resource id's to methods that inflate them Reviewed By: jvillard Differential Revision: D5919172 fbshipit-source-id: 046fe03 --- infer/src/harness/androidFramework.ml | 2 ++ infer/src/harness/androidFramework.mli | 3 ++ infer/src/quandary/JavaTrace.ml | 27 +++++++++++++-- infer/src/quandary/TaintAnalysis.ml | 46 ++++++++++++++++++++++---- 4 files changed, 69 insertions(+), 9 deletions(-) diff --git a/infer/src/harness/androidFramework.ml b/infer/src/harness/androidFramework.ml index c81eed2d2..c0b2ffddd 100644 --- a/infer/src/harness/androidFramework.ml +++ b/infer/src/harness/androidFramework.ml @@ -17,6 +17,8 @@ let on_destroy = "onDestroy" let on_destroy_view = "onDestroyView" +let drawable_prefix = "R$drawable" + (** return true if [pname] is a special lifecycle cleanup method *) let is_destroy_method pname = match pname with diff --git a/infer/src/harness/androidFramework.mli b/infer/src/harness/androidFramework.mli index 9d93d42c6..9a99dd68e 100644 --- a/infer/src/harness/androidFramework.mli +++ b/infer/src/harness/androidFramework.mli @@ -11,6 +11,9 @@ open! IStd (** Android lifecycle types and their lifecycle methods that are called by the framework *) +val drawable_prefix : string +(** prefix for Drawable fields in generated resources *) + val get_lifecycles : (string * string * string list) list (** return the complete list of (package, lifecycle_classname, lifecycle_methods) trios *) diff --git a/infer/src/quandary/JavaTrace.ml b/infer/src/quandary/JavaTrace.ml index 1587fbb32..d36879c26 100644 --- a/infer/src/quandary/JavaTrace.ml +++ b/infer/src/quandary/JavaTrace.ml @@ -13,6 +13,7 @@ module L = Logging module SourceKind = struct type t = + | DrawableResource of Pvar.t (** Drawable resource ID read from a global *) | Intent (** external Intent or a value read from one *) | Other (** for testing or uncategorized sources *) | PrivateData (** private user or device-specific data *) @@ -39,7 +40,7 @@ module SourceKind = struct ~f:(fun {QuandaryConfig.Source.procedure; kind} -> (Str.regexp procedure, kind)) (QuandaryConfig.Source.of_json Config.quandary_sources) - let get pname _ tenv = + let get pname actuals tenv = let return = None in match pname with | Typ.Procname.Java pname -> ( @@ -85,6 +86,18 @@ module SourceKind = struct if Str.string_match procedure_regex procedure 0 then Some (of_string kind, return) else None) external_sources ) + | Typ.Procname.C _ when Typ.Procname.equal pname BuiltinDecl.__global_access -> ( + match (* accessed global will be passed to us as the only parameter *) + actuals with + | [(HilExp.AccessPath ((Var.ProgramVar pvar, _), _))] + -> let pvar_string = Pvar.to_string pvar in + (* checking substring instead of prefix because we expect field names like + com.myapp.R$drawable.whatever *) + if String.is_substring ~substring:AndroidFramework.drawable_prefix pvar_string then + Some (DrawableResource pvar, None) + else None + | _ + -> None ) | pname when BuiltinDecl.is_declared pname -> None | pname @@ -156,8 +169,10 @@ module SourceKind = struct "Non-Java procedure %a where only Java procedures are expected" Typ.Procname.pp procname let pp fmt kind = - F.fprintf fmt + F.fprintf fmt "%s" ( match kind with + | DrawableResource pvar + -> Pvar.to_string pvar | Intent -> "Intent" | Other @@ -176,6 +191,7 @@ module SinkKind = struct type t = | CreateFile (** sink that creates a file *) | CreateIntent (** sink that creates an Intent *) + | OpenDrawableResource (** sink that inflates a Drawable resource from an integer ID *) | Deserialization (** sink that deserializes a Java object *) | HTML (** sink that creates HTML *) | JavaScript (** sink that passes its arguments to untrusted JS code *) @@ -199,6 +215,8 @@ module SinkKind = struct -> JavaScript | "Logging" -> Logging + | "OpenDrawableResource" + -> OpenDrawableResource | "StartComponent" -> StartComponent | _ @@ -312,6 +330,8 @@ module SinkKind = struct -> "JavaScript" | Logging -> "Logging" + | OpenDrawableResource + -> "OpenDrawableResource" | StartComponent -> "StartComponent" | Other @@ -346,6 +366,9 @@ include Trace.Make (struct | (Intent | UserControlledURI | UserControlledString), Deserialization -> (* shouldn't let anyone external control what we deserialize *) true + | DrawableResource _, OpenDrawableResource + -> (* not a security issue, but useful for debugging flows from resource IDs to inflation *) + true | Other, _ | _, Other -> (* for testing purposes, Other matches everything *) true diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 27323bf91..ca956ce84 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -357,6 +357,38 @@ module Make (TaintSpecification : TaintSpec.S) = struct in TaintDomain.trace_fold add_to_caller_tree summary caller_access_tree + let preprocess_exp exp = + (* hack: preprocessor for expressions to convert some literals into dummy field reads. needed in + Java when we want to us an R.id.whatever field as a source, but the compiler inlines the + field read because it's a static final constant *) + let convert_id_literal_to_read = function + | HilExp.Constant Const.Cint i as e + -> let int_value = + try IntLit.to_int i + with _ -> 0 + in + (* heuristic to decide if this looks like a resource ID *) + if Int.abs int_value > 1000 then + (* convert this resource ID literal into a dummy field read *) + let dummy_pvar = + Pvar.mk_global + (Mangled.from_string + (F.sprintf "%s_%d" AndroidFramework.drawable_prefix int_value)) + Pvar.TUExtern + in + HilExp.AccessPath ((Var.of_pvar dummy_pvar, Typ.mk (Typ.Tint Typ.IInt)), []) + else e + | e + -> e + in + convert_id_literal_to_read exp + + (* avoid overhead of allocating new list in the (very) common case that preprocess is a no-op *) + let preprocess_exps exps = + if List.exists ~f:(fun exp -> not (phys_equal exp (preprocess_exp exp))) exps then + List.map ~f:preprocess_exp exps + else exps + let exec_instr (astate: Domain.astate) (proc_data: extras ProcData.t) _ (instr: HilInstr.t) = (* not all sinks are function calls; we might want to treat an array or field access as a sink too. do this by pretending an access is a call to a dummy function and using the @@ -382,13 +414,11 @@ module Make (TaintSpecification : TaintSpec.S) = struct List.fold ~f:add_sinks_for_access ~init:astate accesses in let add_sources_for_access_path ((var, _), _ as ap) loc astate = - L.d_strln "adding sources for access path" ; if Var.is_global var then let dummy_call_site = CallSite.make BuiltinDecl.__global_access loc in match TraceDomain.Source.get dummy_call_site [HilExp.AccessPath ap] proc_data.tenv with | Some {TraceDomain.Source.source} - -> L.d_strln "Got source to add" ; - let access_path = AccessPath.Abs.Exact ap in + -> let access_path = AccessPath.Abs.Exact ap in let trace, subtree = Option.value ~default:TaintDomain.empty_node (TaintDomain.get_node access_path astate) @@ -400,7 +430,6 @@ module Make (TaintSpecification : TaintSpec.S) = struct else astate in let add_sources_sinks_for_exp exp loc astate = - L.d_strln "adding source/sinks" ; match exp with | HilExp.AccessPath access_path -> add_sinks_for_access_path access_path loc astate @@ -428,12 +457,15 @@ module Make (TaintSpecification : TaintSpec.S) = struct `return null` in a void function *) astate | Assign (lhs_access_path, rhs_exp, loc) - -> add_sources_sinks_for_exp rhs_exp loc astate + -> let rhs_exp = preprocess_exp rhs_exp in + add_sources_sinks_for_exp rhs_exp loc astate |> add_sinks_for_access_path lhs_access_path loc |> exec_write lhs_access_path rhs_exp | Assume (assume_exp, _, _, loc) - -> add_sources_sinks_for_exp assume_exp loc astate + -> let assume_exp = preprocess_exp assume_exp in + add_sources_sinks_for_exp assume_exp loc astate | Call (ret_opt, Direct called_pname, actuals, call_flags, callee_loc) - -> let astate = + -> let actuals = preprocess_exps actuals in + let astate = List.fold ~f:(fun acc exp -> add_sources_sinks_for_exp exp callee_loc acc) actuals ~init:astate