From 6b8900746b469f1f3f0b80fede7ef9955ebe17ef Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 1 May 2018 06:17:21 -0700 Subject: [PATCH] [quandary] only treat overrides of service methods as endpoints Reviewed By: jeremydubreil Differential Revision: D7813903 fbshipit-source-id: 7d77f6a --- infer/src/absint/PatternMatch.ml | 23 +++++--- infer/src/quandary/ClangTrace.ml | 25 ++++---- .../codetoanalyze/cpp/quandary/endpoints.cpp | 58 ++++++++++++++++--- .../codetoanalyze/cpp/quandary/issues.exp | 1 + 4 files changed, 79 insertions(+), 28 deletions(-) diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index 18dd96d3a..c991c58cb 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/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 super_proc_name = Typ.Procname.replace_class proc_name super_class_name in + let method_name = Typ.Procname.get_method proc_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 = - Typ.Name.Java.from_string (Typ.Procname.Java.get_class_name proc_name_java) - in - List.exists ~f:(super_type_exists tenv) - (type_get_direct_supertypes tenv (Typ.mk (Tstruct type_name))) + super_type_exists tenv + (Typ.Name.Java.from_string (Typ.Procname.Java.get_class_name proc_name_java)) + | Typ.Procname.ObjC_Cpp proc_name_cpp -> + super_type_exists tenv (Typ.Procname.ObjC_Cpp.get_class_type_name proc_name_cpp) | _ -> false diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index 71d9cf338..0d7a51bcb 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/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 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 + let overrides_service_method pname tenv = + PatternMatch.override_exists + (function + | Typ.Procname.ObjC_Cpp cpp_pname -> + let class_name = Typ.Procname.ObjC_Cpp.get_class_name cpp_pname in + let res = + String.is_suffix ~suffix:"SvIf" class_name + || String.is_suffix ~suffix:"SvAsyncIf" class_name + in + res + | _ -> + 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 | _ -> diff --git a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp index 12a7fb3e4..994ec182b 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp +++ b/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 { diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index 9c298d646..1c8143210 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/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_>,Call to basics::template_sink_> 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]