From 28b346a9034606fff462564672c7243c696f4c48 Mon Sep 17 00:00:00 2001 From: Ted Reed Date: Thu, 6 Dec 2018 05:24:34 -0800 Subject: [PATCH] quandary: Detect flows to EnvironmentChange that includes putenv only Reviewed By: mbouaziz Differential Revision: D13161904 fbshipit-source-id: dc5d87c6c --- infer/src/base/IssueType.ml | 2 ++ infer/src/base/IssueType.mli | 2 ++ infer/src/quandary/ClangTrace.ml | 18 +++++++++++++++--- .../codetoanalyze/cpp/quandary/endpoints.cpp | 18 ++++++++++++++++++ .../codetoanalyze/cpp/quandary/issues.exp | 2 ++ 5 files changed, 39 insertions(+), 3 deletions(-) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index c96a49b9b..33ca51d03 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -407,6 +407,8 @@ let untrusted_deserialization = from_string "UNTRUSTED_DESERIALIZATION" let untrusted_deserialization_risk = from_string "UNTRUSTED_DESERIALIZATION_RISK" +let untrusted_environment_change_risk = from_string "UNTRUSTED_ENVIRONMENT_CHANGE_RISK" + let untrusted_file = from_string "UNTRUSTED_FILE" let untrusted_file_risk = from_string "UNTRUSTED_FILE_RISK" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 440b34d35..500f9ce4f 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -317,6 +317,8 @@ val untrusted_intent_creation : t val untrusted_url_risk : t +val untrusted_environment_change_risk : t + val untrusted_variable_length_array : t val user_controlled_sql_risk : t diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index a68da2fab..6422f2792 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -195,6 +195,7 @@ module SinkKind = struct type t = | BufferAccess (** read/write an array *) | CreateFile (** create/open a file *) + | EnvironmentChange (** change environment variable or gflag *) | HeapAllocation (** heap memory allocation *) | ShellExec (** shell exec function *) | SQLInjection (** unescaped query to a SQL database (could be read or write) *) @@ -212,6 +213,8 @@ module SinkKind = struct BufferAccess | "CreateFile" -> CreateFile + | "EnvironmentChange" -> + EnvironmentChange | "HeapAllocation" -> HeapAllocation | "ShellExec" -> @@ -303,6 +306,9 @@ module SinkKind = struct taint_nth 1 BufferAccess actuals | _ -> get_external_sink pname actuals ) + | Typ.Procname.C _ + when String.is_substring ~substring:"SetCommandLineOption" (Typ.Procname.to_string pname) -> + taint_nth 1 EnvironmentChange actuals | Typ.Procname.C _ when Config.developer_mode && Typ.Procname.equal pname BuiltinDecl.__array_access -> taint_all BufferAccess actuals @@ -341,6 +347,8 @@ module SinkKind = struct taint_nth 1 CreateFile actuals | "popen" -> taint_nth 0 ShellExec actuals + | "putenv" -> + taint_nth 0 EnvironmentChange actuals | ("brk" | "calloc" | "malloc" | "realloc" | "sbrk") when Config.developer_mode -> taint_all HeapAllocation actuals | "rename" -> @@ -367,6 +375,8 @@ module SinkKind = struct "BufferAccess" | CreateFile -> "CreateFile" + | EnvironmentChange -> + "EnvironmentChange" | HeapAllocation -> "HeapAllocation" | ShellExec -> @@ -467,8 +477,6 @@ include Trace.Make (struct Option.some_if (is_injection_possible ~typ Sanitizer.EscapeURL sanitizers) IssueType.untrusted_url_risk - | (CommandLineFlag _ | EnvironmentVariable | ReadFile), URL -> - None | ( (CommandLineFlag (_, typ) | Endpoint (_, typ) | UserControlledEndpoint (_, typ)) , SQLInjection ) -> if is_injection_possible ~typ Sanitizer.EscapeSQL sanitizers then @@ -480,6 +488,9 @@ include Trace.Make (struct | (Endpoint _ | UserControlledEndpoint _), (SQLRead | SQLWrite) -> (* no injection risk, but still user-controlled *) Some IssueType.user_controlled_sql_risk + | (Endpoint _ | UserControlledEndpoint _), EnvironmentChange -> + (* user-controlled environment mutation *) + Some IssueType.untrusted_environment_change_risk | (CommandLineFlag (_, typ) | Endpoint (_, typ) | UserControlledEndpoint (_, typ)), ShellExec -> (* code injection if the caller of the endpoint doesn't sanitize on its end *) @@ -529,7 +540,8 @@ include Trace.Make (struct (* 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 - | (CommandLineFlag _ | EnvironmentVariable | ReadFile), (CreateFile | SQLRead | SQLWrite) -> + | ( (CommandLineFlag _ | EnvironmentVariable | ReadFile) + , (CreateFile | EnvironmentChange | SQLRead | SQLWrite | URL) ) -> None | Other, _ -> (* Other matches everything *) diff --git a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp index 2d2dda4e4..3553ceaf8 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp @@ -23,6 +23,12 @@ struct request { int i; }; +namespace gflags { + +// Mock gflags SetCommandLineOption. +void SetCommandLineOption(const char* name, const char* value) {} +} // namespace gflags + namespace facebook { namespace fb303 { namespace cpp2 { @@ -51,6 +57,8 @@ class FacebookServiceSvIf { void service_this_ok(); void service_return_param_ok(std::string& _return); void service3_endpoint_bad(std::string formal); + void service3_endpoint_envchange_putenv_bad(std::string formal); + void service3_endpoint_envchange_setoption_bad(std::string formal); private: void FP_private_not_endpoint_ok(std::string formal) { @@ -213,6 +221,16 @@ class Service3 : Service1 { // this should report REMOTE_CODE_EXECUTION_RISK system(formal.c_str()); } + + void service3_endpoint_envchange_putenv_bad(std::string formal) { + // this should report UNTRUSTED_ENVIRONMENT_CHANGE_RISK + putenv(const_cast(formal.c_str())); + } + + void service3_endpoint_envchange_setoption_bad(std::string formal) { + // this should report UNTRUSTED_ENVIRONMENT_CHANGE_RISK + gflags::SetCommandLineOption("teest", formal.c_str()); + } }; } // namespace endpoints diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index c4a39e0ed..a48586b1a 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -65,6 +65,8 @@ codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_en codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_endpoint_to_sql_bad, 2, SQL_INJECTION_RISK, no_bucket, ERROR, [Return from endpoints::Service1_user_controlled_endpoint_to_sql_bad,Call to __infer_sql_sink with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service2_service2_endpoint_bad, 2, SHELL_INJECTION_RISK, no_bucket, ERROR, [Return from endpoints::Service2_service2_endpoint_bad,Call to system with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service3_service3_endpoint_bad, 2, SHELL_INJECTION_RISK, no_bucket, ERROR, [Return from endpoints::Service3_service3_endpoint_bad,Call to system with tainted index 0] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service3_service3_endpoint_envchange_putenv_bad, 2, UNTRUSTED_ENVIRONMENT_CHANGE_RISK, no_bucket, ERROR, [Return from endpoints::Service3_service3_endpoint_envchange_putenv_bad,Call to putenv with tainted index 0] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service3_service3_endpoint_envchange_setoption_bad, 2, UNTRUSTED_ENVIRONMENT_CHANGE_RISK, no_bucket, ERROR, [Return from endpoints::Service3_service3_endpoint_envchange_setoption_bad,Call to gflags::SetCommandLineOption with tainted index 1] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 6, SHELL_INJECTION, no_bucket, ERROR, [Return from getenv,Call to execl with tainted index 1] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 8, SHELL_INJECTION, no_bucket, ERROR, [Return from getenv,Call to execl with tainted index 0] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 11, SHELL_INJECTION, no_bucket, ERROR, [Return from getenv,Call to execl with tainted index 2]