[quandary] support sources that taint a pointer arg or arg passed by ref rather than the return value

Summary: A lot of C++ library functions look like this, so it's important to have.

Reviewed By: mbouaziz

Differential Revision: D5026082

fbshipit-source-id: 6f421b6

infer/src/IR/Typ.re

@@ -261,6 +261,14 @@ module Name = {
         QualifiedCppName.append_template_args_to_last name args::template_suffix
       }
     | JavaClass _ => QualifiedCppName.empty;
+  let unqualified_name =
+    fun
+    | CStruct name
+    | CUnion name
+    | ObjcClass name
+    | ObjcProtocol name => name
+    | CppClass name _ => name
+    | JavaClass _ => QualifiedCppName.empty;
   let name n =>
     switch n {
     | CStruct _

infer/src/IR/Typ.rei

@@ -142,6 +142,7 @@ module Name: {
 
   /** qualified name of the type, may return nonsense for Java classes */
   let qual_name: t => QualifiedCppName.t;
+  let unqualified_name: t => QualifiedCppName.t;
   module C: {
     let from_string: string => t;
     let from_qual_name: QualifiedCppName.t => t;

infer/src/checkers/Source.ml

@@ -21,7 +21,7 @@ module type Kind = sig
 
   val unknown : t
 
-  val get : Typ.Procname.t -> Tenv.t -> t option
+  val get : Typ.Procname.t -> Tenv.t -> (t * int option) option
 
   val get_tainted_formals : Procdesc.t -> Tenv.t -> (Mangled.t * Typ.t * t option) list
 end
@@ -29,13 +29,19 @@ end
 module type S = sig
   include TraceElem.S
 
+  type spec =
+    {
+      source : t;
+      index : int option;
+    }
+
   val is_footprint : t -> bool
 
   val make_footprint : AccessPath.t -> Procdesc.t -> t
 
   val get_footprint_access_path: t -> AccessPath.t option
 
-  val get : CallSite.t -> Tenv.t -> t option
+  val get : CallSite.t -> Tenv.t -> spec option
 
   val get_tainted_formals : Procdesc.t -> Tenv.t -> (Mangled.t * Typ.t * t option) list
 end
@@ -58,6 +64,12 @@ module Make (Kind : Kind) = struct
       site : CallSite.t;
     } [@@deriving compare]
 
+  type spec =
+    {
+      source : t;
+      index : int option;
+    }
+
   let is_footprint t = match t.kind with
     | Footprint _ -> true
     | _ -> false
@@ -82,8 +94,11 @@ module Make (Kind : Kind) = struct
     { site; kind; }
 
   let get site tenv = match Kind.get (CallSite.pname site) tenv with
-    | Some kind -> Some (make kind site)
+    | Some (kind, index) ->
-    | None -> None
+        let source = make kind site in
+        Some { source; index; }
+    | None ->
+        None
 
   let get_tainted_formals pdesc tenv =
     let site = CallSite.make (Procdesc.get_proc_name pdesc) (Procdesc.get_loc pdesc) in
@@ -110,6 +125,12 @@ end
 module Dummy = struct
   type t = unit [@@deriving compare]
 
+  type spec =
+    {
+      source : t;
+      index : int option;
+    }
+
   let call_site _ = CallSite.dummy
 
   let kind t = t

infer/src/checkers/Source.mli

@@ -19,7 +19,7 @@ module type Kind = sig
   val unknown : t
 
   (** return Some (kind) if the procedure is a taint source, None otherwise *)
-  val get : Typ.Procname.t -> Tenv.t -> t option
+  val get : Typ.Procname.t -> Tenv.t -> (t * int option) option
 
   (** return each formal of the function paired with either Some(kind) if the formal is a taint
       source, or None if the formal is not a taint source *)
@@ -29,6 +29,14 @@ end
 module type S = sig
   include TraceElem.S
 
+  type spec =
+    {
+      source : t;
+      (** type of the returned source *)
+      index : int option;
+      (** index of the returned source if Some; return value if None *)
+    }
+
   (** return true if the current source is a footprint source *)
   val is_footprint : t -> bool
 
@@ -38,8 +46,8 @@ module type S = sig
   (** return Some(access path) if the current source is a footprint source, None otherwise *)
   val get_footprint_access_path: t -> AccessPath.t option
 
-  (** return Some (source) if the call site is a taint source, None otherwise *)
+  (** return Some (taint spec) if the call site is a taint source, None otherwise *)
-  val get : CallSite.t -> Tenv.t -> t option
+  val get : CallSite.t -> Tenv.t -> spec option
 
   (** return each formal of the function paired with either Some(source) if the formal is a taint
       source, or None if the formal is not a taint source *)

infer/src/quandary/ClangTrace.ml

@@ -15,6 +15,7 @@ module L = Logging
 module SourceKind = struct
   type t =
     | EnvironmentVariable (** source that was read from an environment variable *)
+    | File (** source that was read from a file *)
     | Other (** for testing or uncategorized sources *)
     | Unknown
   [@@deriving compare]
@@ -23,6 +24,7 @@ module SourceKind = struct
 
   let of_string = function
     | "EnvironmentVariable" -> EnvironmentVariable
+    | "File" -> File
     | _ -> Other
 
   let external_sources =
@@ -32,23 +34,38 @@ module SourceKind = struct
       (QuandaryConfig.Source.of_json Config.quandary_sources)
 
   (* return Some(source kind) if [procedure_name] is in the list of externally specified sources *)
-  let get_external_source pname =
+  let get_external_source qualified_pname =
-    let qualified_pname = Typ.Procname.get_qualifiers pname in
+    let return = None in
     List.find_map
       ~f:(fun (qualifiers, kind) ->
           if QualifiedCppName.Match.match_qualifiers qualifiers qualified_pname
-          then Some (of_string kind)
+          then Some (of_string kind, return)
           else None)
       external_sources
 
-  let get pname _ = match pname with
+  let get pname _ =
-    | Typ.Procname.ObjC_Cpp _ ->
+    let return = None in
-        get_external_source pname
+    match pname with
+    | Typ.Procname.ObjC_Cpp cpp_name ->
+        let qualified_pname = Typ.Procname.get_qualifiers pname in
+        begin
+          match
+            (QualifiedCppName.to_list
+               (Typ.Name.unqualified_name (Typ.Procname.objc_cpp_get_class_type_name cpp_name))),
+            Typ.Procname.get_method pname with
+          | ["std"; ("basic_istream" | "basic_iostream")],
+            ("getline" | "read" | "readsome" | "operator>>") ->
+              Some (File, Some 1)
+          | _ ->
+              get_external_source qualified_pname
+        end
     | Typ.Procname.C _ ->
         begin
           match Typ.Procname.to_string pname with
-          | "getenv" -> Some EnvironmentVariable
+          | "getenv" ->
-          | _ -> get_external_source pname
+              Some (EnvironmentVariable, return)
+          | _ ->
+              get_external_source (Typ.Procname.get_qualifiers pname)
         end
     | Typ.Procname.Block _ ->
         None
@@ -60,10 +77,13 @@ module SourceKind = struct
   let get_tainted_formals pdesc _ =
     Source.all_formals_untainted pdesc
 
-  let pp fmt = function
+  let pp fmt kind =
-    | EnvironmentVariable -> F.fprintf fmt "EnvironmentVariable"
+    F.fprintf fmt
-    | Other -> F.fprintf fmt "Other"
+      (match kind with
-    | Unknown -> F.fprintf fmt "Unknown"
+       | EnvironmentVariable -> "EnvironmentVariable"
+       | File -> "File"
+       | Other -> "Other"
+       | Unknown -> "Unknown")
 end
 
 module CppSource = Source.Make(SourceKind)
@@ -145,7 +165,9 @@ include
 
     let should_report source sink =
       match Source.kind source, Sink.kind sink with
-      | EnvironmentVariable, ShellExec ->
+      | EnvironmentVariable, ShellExec
+      | File, ShellExec ->
+          (* untrusted data flowing to exec *)
           true
       | Other, Other ->
           true

infer/src/quandary/JavaTrace.ml

@@ -34,31 +34,33 @@ module SourceKind = struct
       ~f:(fun { QuandaryConfig.Source.procedure; kind; } -> Str.regexp procedure, kind)
       (QuandaryConfig.Source.of_json Config.quandary_sources)
 
-  let get pname tenv = match pname with
+  let get pname tenv =
+    let return = None in
+    match pname with
     | Typ.Procname.Java pname ->
         begin
           match Typ.Procname.java_get_class_name pname, Typ.Procname.java_get_method pname with
           | "android.location.Location",
             ("getAltitude" | "getBearing" | "getLatitude" | "getLongitude" | "getSpeed") ->
-              Some PrivateData
+              Some (PrivateData, return)
           | "android.telephony.TelephonyManager",
             ("getDeviceId" |
              "getLine1Number" |
              "getSimSerialNumber" |
              "getSubscriberId" |
              "getVoiceMailNumber") ->
-              Some PrivateData
+              Some (PrivateData, return)
           | "com.facebook.infer.builtins.InferTaint", "inferSecretSource" ->
-              Some Other
+              Some (Other, return)
           | class_name, method_name ->
               let taint_matching_supertype typename _ =
                 match Typ.Name.name typename, method_name with
                 | "android.app.Activity", "getIntent" ->
-                    Some Intent
+                    Some (Intent, return)
                 | "android.content.Intent", "getStringExtra" ->
-                    Some Intent
+                    Some (Intent, return)
                 | "android.content.SharedPreferences", "getString" ->
-                    Some PrivateData
+                    Some (PrivateData, return)
                 | _ ->
                     None in
               let kind_opt =
@@ -75,7 +77,7 @@ module SourceKind = struct
                     List.find_map
                       ~f:(fun (procedure_regex, kind) ->
                           if Str.string_match procedure_regex procedure 0
-                          then Some (of_string kind)
+                          then Some (of_string kind, return)
                           else None)
                       external_sources
               end

infer/src/quandary/TaintAnalysis.ml

@@ -96,11 +96,22 @@ module Make (TaintSpecification : TaintSpec.S) = struct
       | AccessPath access_path -> exp_get_node_ ~abstracted access_path access_tree proc_data
       | _ -> None
 
-    let add_source source ret_base access_tree =
+    let add_return_source source ret_base access_tree =
       let trace = TraceDomain.of_source source in
       let id_ap = AccessPath.Exact (ret_base, []) in
       TaintDomain.add_trace id_ap trace access_tree
 
+    let add_actual_source source index actuals access_tree proc_data =
+      match List.nth_exn actuals index with
+      | HilExp.AccessPath actual_ap_raw ->
+          let actual_ap = AccessPath.Exact actual_ap_raw in
+          let trace = access_path_get_trace actual_ap access_tree proc_data in
+          TaintDomain.add_trace actual_ap (TraceDomain.add_source source trace) access_tree
+      | _ ->
+          access_tree
+      | exception (Failure _) ->
+          failwithf "Bad source specification: index %d out of bounds" index
+
     let endpoints = String.Set.of_list (QuandaryConfig.Endpoint.of_json Config.quandary_endpoints)
 
     let is_endpoint source =
@@ -368,9 +379,11 @@ module Make (TaintSpecification : TaintSpec.S) = struct
             let source = TraceDomain.Source.get call_site proc_data.tenv in
             let astate_with_source =
               match source, ret_opt with
-              | Some source, Some ret_base ->
+              | Some { TraceDomain.Source.source; index=None; }, Some ret_base ->
-                  add_source source ret_base astate_with_sink
+                  add_return_source source ret_base astate_with_sink
-              | Some source, None ->
+              | Some { TraceDomain.Source.source; index=Some index; }, _ ->
+                  add_actual_source source index actuals astate_with_sink proc_data
+              | Some { TraceDomain.Source.source; index=None; }, None ->
                   let warn_invalid_source () =
                     L.stderr
                       "Warning: %a is marked as a source, but has no return value"
@@ -384,7 +397,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct
                     match List.last actuals with
                     | Some (HilExp.AccessPath ((Var.ProgramVar pvar, _) as ret_base, []))
                       when Pvar.is_frontend_tmp pvar ->
-                        add_source source ret_base astate_with_sink
+                        add_return_source source ret_base astate_with_sink
                     | _ ->
                         warn_invalid_source ()
                   else

infer/src/unit/TaintTests.ml

@@ -20,7 +20,7 @@ module MockTrace = Trace.Make(struct
 
         let get pname _ =
           if String.is_prefix ~prefix:"SOURCE" (Typ.Procname.to_string pname)
-          then Some (CallSite.make pname Location.dummy)
+          then Some (CallSite.make pname Location.dummy, None)
           else None
 
         let get_tainted_formals _ _ =

infer/tests/codetoanalyze/cpp/quandary/Makefile

@@ -13,8 +13,6 @@ CLANG_OPTIONS = -x c++ -std=c++11 -nostdinc++ -isystem$(ROOT_DIR) -isystem$(CLAN
 INFER_OPTIONS = --ml-buckets cpp --no-filtering --debug-exceptions --project-root $(TESTS_DIR) --no-failures-allowed
 INFERPRINT_OPTIONS = --issues-tests
 
-SOURCES = \
+SOURCES = $(wildcard *.cpp)
-  basics.cpp \
-  execs.cpp \
 
 include $(TESTS_DIR)/clang.make

infer/tests/codetoanalyze/cpp/quandary/execs.cpp

@@ -78,7 +78,6 @@ int callExecBad() {
 extern char* getenv(const char* var);
 
 void execConstantStringOk() { callAllSinks("something.sh", NULL); }
-
 void customGetEnvOk() {
   const char* source = execs::getenv("ENV_VAR");
   return execl(NULL, source);

infer/tests/codetoanalyze/cpp/quandary/files.cpp

@@ -0,0 +1,64 @@
+/*
+ * Copyright (c) 2017 - 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.
+ */
+#include <iostream>
+#include <fstream>
+#include <unistd.h>
+
+namespace files {
+
+void read_file_call_exec_bad1(int length) {
+  std::ifstream is("test.txt", std::ifstream::binary);
+  if (is) {
+    char* buffer = new char[length];
+    is.read(buffer, length);
+    execle(buffer, NULL);
+    is.close();
+  }
+}
+
+void read_file_call_exec_bad2(int length) {
+  std::ifstream is("test.txt", std::ifstream::binary);
+  if (is) {
+    char* buffer = new char[length];
+    is.readsome(buffer, length);
+    execle(buffer, NULL);
+    is.close();
+  }
+}
+
+void read_file_call_exec_bad3(int length) {
+  std::ifstream is("test.txt", std::ifstream::binary);
+  if (is) {
+    char* buffer = new char[length];
+    is.getline(buffer, length);
+    execle(buffer, NULL);
+    is.close();
+  }
+}
+
+// have to do matching on C procnames to make this work
+void FN_read_file_call_exec_bad5(int length) {
+  std::ifstream is("test.txt", std::ifstream::binary);
+  if (is) {
+    char* buffer = new char[length];
+    is >> buffer;
+    execle(buffer, NULL);
+    is.close();
+  }
+}
+
+// make sure we handle reads via iostreams too
+void read_file_call_exec_bad5(std::iostream is, int length) {
+  if (is) {
+    char* buffer = new char[length];
+    is.getline(buffer, length);
+    execle(buffer, NULL);
+  }
+}
+}

infer/tests/codetoanalyze/cpp/quandary/issues.exp

@@ -20,3 +20,7 @@ codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 21, QUANDARY_TAINT_ERR
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 23, QUANDARY_TAINT_ERROR, [return from getenv,call to execvp]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 25, QUANDARY_TAINT_ERROR, [return from getenv,call to execv]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 27, QUANDARY_TAINT_ERROR, [return from getenv,call to execvp]
+codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad1, 5, QUANDARY_TAINT_ERROR, [return from std::basic_istream<char,std::char_traits<char>>_read,call to execle]
+codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad2, 5, QUANDARY_TAINT_ERROR, [return from std::basic_istream<char,std::char_traits<char>>_readsome,call to execle]
+codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad3, 5, QUANDARY_TAINT_ERROR, [return from std::basic_istream<char,std::char_traits<char>>_getline,call to execle]
+codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad5, 4, QUANDARY_TAINT_ERROR, [return from std::basic_istream<char,std::char_traits<char>>_getline,call to execle]