[quandary] only treat overrides of service methods as endpoints

Reviewed By: jeremydubreil

Differential Revision: D7813903

fbshipit-source-id: 7d77f6a

infer/src/absint/PatternMatch.ml

@@ -279,27 +279,32 @@ let proc_calls resolve_attributes pdesc filter : (Typ.Procname.t * ProcAttribute
 
 
 let override_exists f tenv proc_name =
-  let rec super_type_exists tenv super_class_name =
+  let method_name = Typ.Procname.get_method proc_name in
-    let super_proc_name = Typ.Procname.replace_class proc_name super_class_name in
+  let rec super_type_exists_ tenv super_class_name =
     match Tenv.lookup tenv super_class_name with
     | Some {methods; supers} ->
         let is_override pname =
-          Typ.Procname.equal pname super_proc_name && not (Typ.Procname.is_constructor pname)
+          (* Note: very coarse! TODO: match parameter names/types to get an exact match *)
+          String.equal (Typ.Procname.get_method pname) method_name
+          && not (Typ.Procname.is_constructor pname)
         in
         List.exists ~f:(fun pname -> is_override pname && f pname) methods
-        || List.exists ~f:(super_type_exists tenv) supers
+        || List.exists ~f:(super_type_exists_ tenv) supers
     | _ ->
         false
   in
+  let super_type_exists tenv type_name =
+    List.exists ~f:(super_type_exists_ tenv)
+      (type_get_direct_supertypes tenv (Typ.mk (Tstruct type_name)))
+  in
   f proc_name
   ||
   match proc_name with
   | Typ.Procname.Java proc_name_java ->
-      let type_name =
+      super_type_exists tenv
-        Typ.Name.Java.from_string (Typ.Procname.Java.get_class_name proc_name_java)
+        (Typ.Name.Java.from_string (Typ.Procname.Java.get_class_name proc_name_java))
-      in
+  | Typ.Procname.ObjC_Cpp proc_name_cpp ->
-      List.exists ~f:(super_type_exists tenv)
+      super_type_exists tenv (Typ.Procname.ObjC_Cpp.get_class_type_name proc_name_cpp)
-        (type_get_direct_supertypes tenv (Typ.mk (Tstruct type_name)))
   | _ ->
       false
 

infer/src/quandary/ClangTrace.ml

@@ -133,16 +133,19 @@ module SourceKind = struct
     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 overrides_service_method pname tenv =
-        let is_thrift_service_ typename _ =
+        PatternMatch.override_exists
-          match QualifiedCppName.to_list (Typ.Name.unqualified_name typename) with
+          (function
-          | ["facebook"; "fb303"; "cpp2"; ("FacebookServiceSvIf" | "FacebookServiceSvAsyncIf")] ->
+              | Typ.Procname.ObjC_Cpp cpp_pname ->
-              true
+                  let class_name = Typ.Procname.ObjC_Cpp.get_class_name cpp_pname in
-          | _ ->
+                  let res =
-              false
+                    String.is_suffix ~suffix:"SvIf" class_name
+                    || String.is_suffix ~suffix:"SvAsyncIf" class_name
                   in
-        let typename = Typ.Procname.ObjC_Cpp.get_class_type_name cpp_pname in
+                  res
-        PatternMatch.supertype_exists tenv is_thrift_service_ typename
+              | _ ->
+                  false)
+          tenv pname
       in
       (* taint all formals except for [this] *)
       let taint_all_but_this_and_return ~make_source =
@@ -170,7 +173,7 @@ module SourceKind = struct
           if String.Set.mem endpoints qualified_pname then
             taint_all_but_this_and_return ~make_source:(fun name desc ->
                 UserControlledEndpoint (name, desc) )
-          else if is_thrift_service cpp_pname then
+          else if overrides_service_method pname tenv then
             taint_all_but_this_and_return ~make_source:(fun name desc -> Endpoint (name, desc))
           else Source.all_formals_untainted pdesc
       | _ ->

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

@@ -20,24 +20,56 @@ extern std::string __infer_sql_sanitizer(std::string);
 
 extern void curl_easy_setopt(void*, int, ...);
 
+struct request {
+  std::string s;
+  int i;
+};
+
 namespace facebook {
 namespace fb303 {
 namespace cpp2 {
 
-class FacebookServiceSvIf {};
+class FacebookServiceSvIf {
+ public:
+  void service1_endpoint_bad(std::string formal);
+  void user_controlled_endpoint_to_sql_bad(std::string formal);
+  void unsanitized_sql_bad(std::string formal);
+  void sanitized_sql_with_shell_bad(std::string formal);
+  void service1_endpoint_sql_sanitized_bad(std::string formal);
+  void service1_endpoint_sql_read_bad(std::string formal);
+  void service1_endpoint_sql_write_bad(std::string formal);
+  void service1_endpoint_shell_sanitized_ok(std::string formal);
+  void service1_endpoint_struct_string_field_bad(request formal);
+  void open_or_create_c_style_file_bad(const char* filename);
+  void ofstream_open_file_bad(std::string filename);
+  void ifstream_open_file_bad(std::string filename);
+  void fstream_open_file_bad(std::string filename);
+  void endpoint_to_curl_url_bad(request formal);
+  void endpoint_to_curl_url_exp_bad(request formal);
+  void endpoint_to_curl_url_unknown_exp_bad(request formal, int i);
+  void endpoint_to_curl_other_const_ok(request formal);
+  void endpoint_to_curl_other_exp_ok(request formal);
+  void FP_service1_endpoint_struct_int_field_ok(request formal);
+  void service_this_ok();
+  void service_return_param_ok(std::string& _return);
+  void service3_endpoint_bad(std::string formal);
+
+ private:
+  void FP_private_not_endpoint_ok(std::string formal) {
+    system(formal.c_str());
+  }
+};
 
-class FacebookServiceSvAsyncIf {};
+class FacebookServiceSvAsyncIf {
+ public:
+  void service2_endpoint_bad(std::string formal);
+};
 } // namespace cpp2
 } // namespace fb303
 } // namespace facebook
 
 namespace endpoints {
 
-struct request {
-  std::string s;
-  int i;
-};
-
 class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf {
 
  public:
@@ -154,8 +186,18 @@ class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf {
     system(_return.c_str());
   }
 
+  // shadows a private method of super; not an override, but we'll flag it as
+  // one because the code that checks for overrides can't see access modifiers.
+  void FP_private_not_endpoint_ok(std::string formal) {
+    system(formal.c_str());
+  }
+
+  // doesn't override a method from super
+  void non_override_ok(std::string formal) { system(formal.c_str()); }
+
  private:
-  void private_not_endpoint_ok(std::string formal) { system(formal.c_str()); }
+  // similar to above, but even easier
+  void private_non_override_ok(std::string formal) { system(formal.c_str()); }
 };
 
 class Service2 : facebook::fb303::cpp2::FacebookServiceSvAsyncIf {

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

@@ -36,6 +36,7 @@ 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, ERROR, [Return from basics::template_source<std::basic_string<char>_>,Call to basics::template_sink<std::basic_string<char>_> with tainted index 0]
 codetoanalyze/cpp/quandary/basics.cpp, basics::via_passthrough_bad1, 4, QUANDARY_TAINT_ERROR, ERROR, [Return from basics::Obj_string_source,Call to basics::Obj_string_sink with tainted index 1]
 codetoanalyze/cpp/quandary/basics.cpp, basics::via_passthrough_bad2, 3, QUANDARY_TAINT_ERROR, ERROR, [Return from basics::Obj_string_source,Call to basics::Obj_string_sink with tainted index 1]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_FP_private_not_endpoint_ok, 0, SHELL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_FP_private_not_endpoint_ok,Call to system with tainted index 0]
 codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_FP_service1_endpoint_struct_int_field_ok, 1, SHELL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_FP_service1_endpoint_struct_int_field_ok,Call to system with tainted index 0]
 codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_endpoint_to_curl_url_bad, 1, UNTRUSTED_URL_RISK, ERROR, [Return from endpoints::Service1_endpoint_to_curl_url_bad,Call to curl_easy_setopt with tainted index 2]
 codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_endpoint_to_curl_url_exp_bad, 1, UNTRUSTED_URL_RISK, ERROR, [Return from endpoints::Service1_endpoint_to_curl_url_exp_bad,Call to curl_easy_setopt with tainted index 2]