quandary: Detect flows to EnvironmentChange that includes putenv only

Reviewed By: mbouaziz

Differential Revision: D13161904

fbshipit-source-id: dc5d87c6c

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"

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

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 *)

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<char*>(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

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]