[quandary] thrift services as sources + remote code execution risk issue type

Differential Revision: D6177526

fbshipit-source-id: 245095e

infer/src/base/IssueType.ml

@@ -269,6 +269,8 @@ let quandary_taint_error = from_string "QUANDARY_TAINT_ERROR"
 
 let registered_observer_being_deallocated = from_string "REGISTERED_OBSERVER_BEING_DEALLOCATED"
 
+let remote_code_execution_risk = from_string "REMOTE_CODE_EXECUTION_RISK"
+
 let resource_leak = from_string "RESOURCE_LEAK"
 
 let retain_cycle = from_string ~enabled:false "RETAIN_CYCLE"

infer/src/base/IssueType.mli

@@ -182,6 +182,8 @@ val quandary_taint_error : t
 
 val registered_observer_being_deallocated : t
 
+val remote_code_execution_risk : t
+
 val resource_leak : t
 
 val retain_cycle : t

infer/src/quandary/ClangTrace.ml

@@ -18,6 +18,8 @@ module SourceKind = struct
     | EnvironmentVariable  (** source that was read from an environment variable *)
     | File  (** source that was read from a file *)
     | Other  (** for testing or uncategorized sources *)
+    | UserControlledEndpoint of (Mangled.t * Typ.desc)
+        (** source originating from formal of an endpoint that is known to hold user-controlled data *)
     [@@deriving compare]
 
   let matches ~caller ~callee = Int.equal 0 (compare caller callee)
@@ -31,6 +33,8 @@ module SourceKind = struct
         EnvironmentVariable
     | "File" ->
         File
+    | "UserControlledEndpoint" ->
+        Endpoint (Mangled.from_string "NONE", Typ.Tvoid)
     | _ ->
         Other
 
@@ -108,26 +112,40 @@ module SourceKind = struct
         L.(die InternalError) "Non-C++ procname %a in C++ analysis" Typ.Procname.pp pname
 
 
-  let get_tainted_formals pdesc _ =
-    let get_tainted_formals_ qualified_pname =
-      if String.Set.mem endpoints qualified_pname then
+  let get_tainted_formals pdesc tenv =
+    if PredSymb.equal_access (Procdesc.get_attributes pdesc).ProcAttributes.access PredSymb.Private
+    then Source.all_formals_untainted pdesc
+    else
+      let is_thrift_service cpp_pname =
+        let is_thrift_service_ typename _ =
+          match QualifiedCppName.to_list (Typ.Name.unqualified_name typename) with
+          | ["facebook"; "fb303"; "cpp2"; ("FacebookServiceSvIf" | "FacebookServiceSvAsyncIf")] ->
+              true
+          | _ ->
+              false
+        in
+        let typename = Typ.Procname.objc_cpp_get_class_type_name cpp_pname in
+        PatternMatch.supertype_exists tenv is_thrift_service_ typename
+      in
+      let taint_all ~make_source =
         List.map
-          ~f:(fun (name, typ) -> (name, typ, Some (Endpoint (name, typ.Typ.desc))))
+          ~f:(fun (name, typ) -> (name, typ, Some (make_source name typ.Typ.desc)))
           (Procdesc.get_formals pdesc)
-      else Source.all_formals_untainted pdesc
-    in
-    match Procdesc.get_proc_name pdesc with
-    | Typ.Procname.ObjC_Cpp objc as pname ->
-        let qualified_pname =
-          F.sprintf "%s::%s"
-            (Typ.Procname.objc_cpp_get_class_name objc)
-            (Typ.Procname.get_method pname)
-        in
-        get_tainted_formals_ qualified_pname
-    | Typ.Procname.C _ as pname ->
-        get_tainted_formals_ (Typ.Procname.get_method pname)
-    | _ ->
-        Source.all_formals_untainted pdesc
+      in
+      match Procdesc.get_proc_name pdesc with
+      | Typ.Procname.ObjC_Cpp cpp_pname as pname ->
+          let qualified_pname =
+            F.sprintf "%s::%s"
+              (Typ.Procname.objc_cpp_get_class_name cpp_pname)
+              (Typ.Procname.get_method pname)
+          in
+          if String.Set.mem endpoints qualified_pname then
+            taint_all ~make_source:(fun name desc -> UserControlledEndpoint (name, desc))
+          else if is_thrift_service cpp_pname then
+            taint_all ~make_source:(fun name desc -> Endpoint (name, desc))
+          else Source.all_formals_untainted pdesc
+      | _ ->
+          Source.all_formals_untainted pdesc
 
 
   let pp fmt = function
@@ -141,6 +159,8 @@ module SourceKind = struct
         F.fprintf fmt "CommandLineFlag[%a]" Var.pp var
     | Other ->
         F.fprintf fmt "Other"
+    | UserControlledEndpoint (formal_name, _) ->
+        F.fprintf fmt "UserControlledEndpoint[%s]" (Mangled.to_string formal_name)
 
 end
 
@@ -285,6 +305,7 @@ include Trace.Make (struct
   module Sink = CppSink
 
   let get_report source sink =
+    (* TODO: make this accept structs/objects too, but not primitive types or enumes *)
     (* using this to match custom string wrappers such as folly::StringPiece *)
     let is_stringy typ =
       let lowercase_typ = String.lowercase (Typ.to_string (Typ.mk typ)) in
@@ -292,13 +313,19 @@ include Trace.Make (struct
       || String.is_substring ~substring:"char*" lowercase_typ
     in
     match (Source.kind source, Sink.kind sink) with
-    | Endpoint _, BufferAccess ->
+    | Endpoint (_, typ), (ShellExec | SQL) ->
+        (* remote code execution if the caller of the endpoint doesn't sanitize *)
+        Option.some_if (is_stringy typ) IssueType.remote_code_execution_risk
+    | Endpoint _, (BufferAccess | HeapAllocation | StackAllocation) ->
+        (* may want to report this in the future, but don't care for now *)
+        None
+    | UserControlledEndpoint _, BufferAccess ->
         (* untrusted data from an endpoint flowing into a buffer *)
         Some IssueType.quandary_taint_error
-    | Endpoint (_, typ), ShellExec ->
+    | UserControlledEndpoint (_, typ), ShellExec ->
         (* untrusted string data flowing to shell ShellExec *)
         Option.some_if (is_stringy typ) IssueType.shell_injection
-    | Endpoint (_, typ), SQL ->
+    | UserControlledEndpoint (_, typ), SQL ->
         (* untrusted string data flowing to SQL *)
         Option.some_if (is_stringy typ) IssueType.sql_injection
     | (CommandLineFlag _ | EnvironmentVariable | File | Other), BufferAccess ->
@@ -310,10 +337,12 @@ include Trace.Make (struct
     | (CommandLineFlag _ | EnvironmentVariable | File | Other), SQL ->
         (* untrusted flag, environment var, or file data flowing to SQL *)
         Some IssueType.sql_injection
-    | (CommandLineFlag _ | Endpoint _ | EnvironmentVariable | File | Other), HeapAllocation ->
+    | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | File | Other)
+      , HeapAllocation ) ->
         (* untrusted data of any kind flowing to heap allocation. this can cause crashes or DOS. *)
         Some IssueType.quandary_taint_error
-    | (CommandLineFlag _ | Endpoint _ | EnvironmentVariable | File | Other), StackAllocation ->
+    | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | File | Other)
+      , StackAllocation ) ->
         (* untrusted data of any kind flowing to stack buffer allocation. trying to allocate a stack
            buffer that's too large will cause a stack overflow. *)
         Some IssueType.untrusted_variable_length_array

infer/tests/codetoanalyze/cpp/quandary/.inferconfig

@@ -64,6 +64,9 @@
     }
   ],
   "quandary-sanitizers": [
+    {
+      "procedure": "__infer_string_sanitizer"
+    },
     {
       "procedure": "basics::Obj::sanitizer1"
     },

infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp

@@ -0,0 +1,51 @@
+/*
+ * 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 <string>
+
+extern std::string __infer_string_sanitizer(std::string);
+
+namespace facebook {
+namespace fb303 {
+namespace cpp2 {
+
+class FacebookServiceSvIf {};
+
+class FacebookServiceSvAsyncIf {};
+} // namespace cpp2
+} // namespace fb303
+} // namespace facebook
+
+namespace endpoints {
+
+class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf {
+
+ public:
+  void service1_endpoint_bad(std::string formal) { system(formal.c_str()); }
+
+  void service1_endpoint_sanitized_ok(std::string formal) {
+    system(__infer_string_sanitizer(formal).c_str());
+  }
+
+ private:
+  void private_not_endpoint_ok(std::string formal) { system(formal.c_str()); }
+};
+
+class Service2 : facebook::fb303::cpp2::FacebookServiceSvAsyncIf {
+
+ public:
+  void service2_endpoint_bad(std::string formal) { system(formal.c_str()); }
+};
+
+class Service3 : Service1 {
+ public:
+  void service3_endpoint_bad(std::string formal) { system(formal.c_str()); }
+};
+
+} // namespace endpoints

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

@@ -36,10 +36,9 @@ codetoanalyze/cpp/quandary/basics.cpp, basics::via_field_bad1, 3, QUANDARY_TAINT
 codetoanalyze/cpp/quandary/basics.cpp, basics::via_field_bad2, 2, QUANDARY_TAINT_ERROR, [Return from basics::template_source<std::basic_string<char>_>,Call to basics::template_sink<std::basic_string<char>_>]
 codetoanalyze/cpp/quandary/basics.cpp, basics::via_passthrough_bad1, 4, QUANDARY_TAINT_ERROR, [Return from basics::Obj_string_source,Call to basics::Obj_string_sink]
 codetoanalyze/cpp/quandary/basics.cpp, basics::via_passthrough_bad2, 3, QUANDARY_TAINT_ERROR, [Return from basics::Obj_string_source,Call to basics::Obj_string_sink]
-codetoanalyze/cpp/quandary/execs.cpp, execs::Obj_endpoint, 9, SQL_INJECTION, [Return from execs::Obj_endpoint,Call to __infer_sql_sink]
-codetoanalyze/cpp/quandary/execs.cpp, execs::Obj_endpoint, 10, SQL_INJECTION, [Return from execs::Obj_endpoint,Call to __infer_sql_sink]
-codetoanalyze/cpp/quandary/execs.cpp, execs::Obj_endpoint, 11, SQL_INJECTION, [Return from execs::Obj_endpoint,Call to __infer_sql_sink]
-codetoanalyze/cpp/quandary/execs.cpp, execs::Obj_endpoint, 12, SQL_INJECTION, [Return from execs::Obj_endpoint,Call to __infer_sql_sink]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_bad, 0, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_service1_endpoint_bad,Call to system]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service2_service2_endpoint_bad, 0, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service2_service2_endpoint_bad,Call to system]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service3_service3_endpoint_bad, 0, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service3_service3_endpoint_bad,Call to system]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 6, SHELL_INJECTION, [Return from getenv,Call to execl]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 8, SHELL_INJECTION, [Return from getenv,Call to execl]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 11, SHELL_INJECTION, [Return from getenv,Call to execl]