From 8665386b8aa3b6f481a8dc23fa35f3b2433b2238 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 16 Nov 2017 11:05:29 -0800 Subject: [PATCH] [quandary] report USER_CONTROLLED_SQL_RISK on flows from endpoint -> SQL Reviewed By: mbouaziz Differential Revision: D6338997 fbshipit-source-id: 19c4380 --- infer/src/base/IssueType.ml | 2 + infer/src/base/IssueType.mli | 2 + infer/src/quandary/ClangTrace.ml | 72 ++++++++++--------- .../codetoanalyze/cpp/quandary/.inferconfig | 2 + .../codetoanalyze/cpp/quandary/endpoints.cpp | 45 ++++++++++-- .../codetoanalyze/cpp/quandary/issues.exp | 10 ++- 6 files changed, 93 insertions(+), 40 deletions(-) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 3eee72a19..f8b421156 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -311,4 +311,6 @@ let use_after_free = from_string "USE_AFTER_FREE" let untrusted_variable_length_array = from_string "UNTRUSTED_VARIABLE_LENGTH_ARRAY" +let user_controlled_sql_risk = from_string "USER_CONTROLLED_SQL_RISK" + let wrong_argument_number = from_string "Wrong_argument_number" ~hum:"Wrong Argument Number" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 05e3f6aca..2a354b33e 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -220,4 +220,6 @@ val use_after_free : t val untrusted_variable_length_array : t +val user_controlled_sql_risk : t + val wrong_argument_number : t diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index 23cf9eb3c..2f69e0909 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -331,57 +331,63 @@ include Trace.Make (struct module Sink = CppSink module Sanitizer = CppSanitizer - let is_sanitized (sink_kind: SinkKind.t) sanitizers = - match sanitizers with - | [] -> - false - | _ when List.mem sanitizers Sanitizer.All ~equal:Sanitizer.equal -> - (* the All sanitizer clears any form of taint *) - true - | _ -> - match sink_kind with - | ShellExec | SQL -> - List.mem sanitizers Sanitizer.Escape ~equal:Sanitizer.equal - | _ -> - false - - - let get_report source sink sanitizers = - (* TODO: make this accept structs/objects too, but not primitive types or enumes *) + (* return true if code injection is possible because the source is a string/is not sanitized *) + let is_injection_possible ?typ sanitizers = + let is_escaped = List.mem sanitizers Sanitizer.Escape ~equal:Sanitizer.equal in (* 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 String.is_substring ~substring:"string" lowercase_typ || String.is_substring ~substring:"char*" lowercase_typ in + not is_escaped + && + match typ with + | Some typ -> + is_stringy typ + | None -> + (* not sure if it's a string; assume injection possible *) + true + + + let get_report source sink sanitizers = match (Source.kind source, Sink.kind sink) with - | _, sink_kind when is_sanitized sink_kind sanitizers -> - (* trace is sanitized; don't report *) - None - | 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 *) + | _ when List.mem sanitizers Sanitizer.All ~equal:Sanitizer.equal -> + (* the All sanitizer clears any form of taint; don't report *) None + | UserControlledEndpoint (_, typ), SQL -> + if is_injection_possible ~typ sanitizers then Some IssueType.sql_injection + else + (* no injection risk, but still user-controlled *) + Some IssueType.user_controlled_sql_risk + | Endpoint (_, typ), SQL -> + if is_injection_possible ~typ sanitizers then + (* code injection if the caller of the endpoint doesn't sanitize on its end *) + Some IssueType.remote_code_execution_risk + else + (* no injection risk, but still user-controlled *) + Some IssueType.user_controlled_sql_risk + | Endpoint (_, typ), ShellExec -> + (* code injection if the caller of the endpoint doesn't sanitize on its end *) + Option.some_if (is_injection_possible ~typ sanitizers) IssueType.remote_code_execution_risk + | UserControlledEndpoint (_, typ), ShellExec -> + (* we know the user controls the endpoint, so it's code injection without a sanitizer *) + Option.some_if (is_injection_possible ~typ sanitizers) IssueType.shell_injection | UserControlledEndpoint _, BufferAccess -> (* untrusted data from an endpoint flowing into a buffer *) Some IssueType.quandary_taint_error - | UserControlledEndpoint (_, typ), ShellExec -> - (* untrusted string data flowing to shell ShellExec *) - Option.some_if (is_stringy typ) IssueType.shell_injection - | UserControlledEndpoint (_, typ), SQL -> - (* untrusted string data flowing to SQL *) - Option.some_if (is_stringy typ) IssueType.sql_injection + | Endpoint _, (BufferAccess | HeapAllocation | StackAllocation) -> + (* may want to report this in the future, but don't care for now *) + None | (CommandLineFlag _ | EnvironmentVariable | File | Other), BufferAccess -> (* untrusted flag, environment var, or file data flowing to buffer *) Some IssueType.quandary_taint_error | (CommandLineFlag _ | EnvironmentVariable | File | Other), ShellExec -> (* untrusted flag, environment var, or file data flowing to shell *) - Some IssueType.shell_injection + Option.some_if (is_injection_possible sanitizers) IssueType.shell_injection | (CommandLineFlag _ | EnvironmentVariable | File | Other), SQL -> (* untrusted flag, environment var, or file data flowing to SQL *) - Some IssueType.sql_injection + Option.some_if (is_injection_possible sanitizers) IssueType.sql_injection | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | File | Other) , HeapAllocation ) -> (* untrusted data of any kind flowing to heap allocation. this can cause crashes or DOS. *) diff --git a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig index 381d85d33..e59e68f2a 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig +++ b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig @@ -81,6 +81,8 @@ ], "quandary-endpoints": [ "basics::Obj::endpoint", + "endpoints::Service1::user_controlled_endpoint_to_sql_bad", + "endpoints::Service1::user_controlled_endpoint_to_shell_bad", "execs::Obj::endpoint" ] } diff --git a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp index f6900e4b3..eeefa0456 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp @@ -9,6 +9,8 @@ #include +extern void __infer_sql_sink(std::string); +extern std::string __infer_all_sanitizer(std::string); extern std::string __infer_string_sanitizer(std::string); namespace facebook { @@ -27,9 +29,38 @@ namespace endpoints { class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf { public: - void service1_endpoint_bad(std::string formal) { system(formal.c_str()); } + void service1_endpoint_bad(std::string formal) { + // this should report REMOTE_CODE_EXECUTION_RISK + system(formal.c_str()); + } + + // this is specified as user-controlled in .inferconfig + void user_controlled_endpoint_to_sql_bad(std::string formal) { + // this should report SQL_INJECTION + __infer_sql_sink(formal); + } + + // this is specified as user-controlled in .inferconfig + void user_controlled_endpoint_to_shell_bad(std::string formal) { + // this should report SHELL_INJECTION + system(formal.c_str()); + } + + void unsanitized_sql_bad(std::string formal) { + // this should report REMOTE_CODE_EXECUTION_RISK + __infer_sql_sink(formal); + } - void service1_endpoint_sanitized_ok(std::string formal) { + void sanitized_sql_bad(std::string formal) { + // this should report USER_CONTROLLED_SQL_RISK + __infer_sql_sink(__infer_string_sanitizer(formal)); + } + + void service1_endpoint_sql_sanitized_ok(std::string formal) { + __infer_sql_sink(__infer_all_sanitizer(formal)); + } + + void service1_endpoint_shell_sanitized_ok(std::string formal) { system(__infer_string_sanitizer(formal).c_str()); } @@ -40,12 +71,18 @@ class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf { class Service2 : facebook::fb303::cpp2::FacebookServiceSvAsyncIf { public: - void service2_endpoint_bad(std::string formal) { system(formal.c_str()); } + void service2_endpoint_bad(std::string formal) { + // this should report REMOTE_CODE_EXECUTION_RISK + system(formal.c_str()); + } }; class Service3 : Service1 { public: - void service3_endpoint_bad(std::string formal) { system(formal.c_str()); } + void service3_endpoint_bad(std::string formal) { + // this should report REMOTE_CODE_EXECUTION_RISK + 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 48f09b5e5..577efa06c 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -36,9 +36,13 @@ 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/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/endpoints.cpp, endpoints::Service1_sanitized_sql_bad, 2, USER_CONTROLLED_SQL_RISK, [Return from endpoints::Service1_sanitized_sql_bad,Call to __infer_sql_sink] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_service1_endpoint_bad,Call to system] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_unsanitized_sql_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_unsanitized_sql_bad,Call to __infer_sql_sink] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_endpoint_to_shell_bad, 2, SHELL_INJECTION, [Return from endpoints::Service1_user_controlled_endpoint_to_shell_bad,Call to system] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_endpoint_to_sql_bad, 2, SQL_INJECTION, [Return from endpoints::Service1_user_controlled_endpoint_to_sql_bad,Call to __infer_sql_sink] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service2_service2_endpoint_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service2_service2_endpoint_bad,Call to system] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service3_service3_endpoint_bad, 2, 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]