From 29fe7d1689fcee911ff94f9799183e3ed4126ae4 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Sun, 5 Nov 2017 02:02:14 -0800 Subject: [PATCH] [quandary] thrift services as sources + remote code execution risk issue type Differential Revision: D6177526 fbshipit-source-id: 245095e --- infer/src/base/IssueType.ml | 2 + infer/src/base/IssueType.mli | 2 + infer/src/quandary/ClangTrace.ml | 75 +++++++++++++------ .../codetoanalyze/cpp/quandary/.inferconfig | 3 + .../codetoanalyze/cpp/quandary/endpoints.cpp | 51 +++++++++++++ .../codetoanalyze/cpp/quandary/issues.exp | 7 +- 6 files changed, 113 insertions(+), 27 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index ca2d89df1..aa028b8c5 100644 --- a/infer/src/base/IssueType.ml +++ b/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" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index a93b48914..25d39100c 100644 --- a/infer/src/base/IssueType.mli +++ b/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 diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index 441e14ff7..12b3919e5 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/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 diff --git a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig index b434de8ee..da5bd5d60 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig +++ b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig @@ -64,6 +64,9 @@ } ], "quandary-sanitizers": [ + { + "procedure": "__infer_string_sanitizer" + }, { "procedure": "basics::Obj::sanitizer1" }, diff --git a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp new file mode 100644 index 000000000..f6900e4b3 --- /dev/null +++ b/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 + +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 diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index e147f337d..8c98ce16b 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/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_>,Call to basics::template_sink_>] 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]