[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

@@ -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

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 *)
 

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

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