[quandary] only warn on shell/sql injection from stringy gflag sources

Reviewed By: jeremydubreil

Differential Revision: D6573250

fbshipit-source-id: 5680773

infer/src/quandary/ClangTrace.ml

@@ -13,7 +13,7 @@ module L = Logging
 
 module SourceKind = struct
   type t =
-    | CommandLineFlag of Var.t  (** source that was read from a command line flag *)
+    | CommandLineFlag of (Var.t * Typ.desc)  (** source that was read from a command line flag *)
     | Endpoint of (Mangled.t * Typ.desc)  (** source originating from formal of an endpoint *)
     | EnvironmentVariable  (** source that was read from an environment variable *)
     | ReadFile  (** source that was read from a file *)
@@ -60,7 +60,7 @@ module SourceKind = struct
       external_sources
 
 
-  let get pname actuals _ =
+  let get pname actuals tenv =
     let return = None in
     match pname with
     | Typ.Procname.ObjC_Cpp cpp_name
@@ -94,7 +94,14 @@ module SourceKind = struct
         match actuals with
         | [(HilExp.AccessPath access_path)] when is_gflag access_path ->
             let (global_pvar, _), _ = access_path in
-            Some (CommandLineFlag global_pvar, None)
+            let typ_desc =
+              match AccessPath.get_typ access_path tenv with
+              | Some {Typ.desc} ->
+                  desc
+              | None ->
+                  Typ.void_star.desc
+            in
+            Some (CommandLineFlag (global_pvar, typ_desc), None)
         | _ ->
             None )
     | Typ.Procname.C _ -> (
@@ -161,7 +168,7 @@ module SourceKind = struct
         F.fprintf fmt "EnvironmentVariable"
     | ReadFile ->
         F.fprintf fmt "File"
-    | CommandLineFlag var ->
+    | CommandLineFlag (var, _) ->
         F.fprintf fmt "CommandLineFlag[%a]" Var.pp var
     | Other ->
         F.fprintf fmt "Other"
@@ -411,15 +418,15 @@ include Trace.Make (struct
         None
     | UserControlledEndpoint (_, typ), CreateFile ->
         Option.some_if (is_injection_possible ~typ sanitizers) IssueType.untrusted_file
-    | Endpoint (_, typ), CreateFile ->
+    | (Endpoint (_, typ) | CommandLineFlag (_, typ)), CreateFile ->
         Option.some_if (is_injection_possible ~typ sanitizers) IssueType.untrusted_file_risk
     | UserControlledEndpoint (_, typ), Network ->
         Option.some_if (is_injection_possible ~typ sanitizers) IssueType.untrusted_url
-    | Endpoint (_, typ), Network ->
+    | (Endpoint (_, typ) | CommandLineFlag (_, typ)), Network ->
         Option.some_if (is_injection_possible ~typ sanitizers) IssueType.untrusted_url_risk
-    | (CommandLineFlag _ | EnvironmentVariable | ReadFile), Network ->
+    | (EnvironmentVariable | ReadFile), Network ->
         None
-    | UserControlledEndpoint (_, typ), SQL ->
+    | (UserControlledEndpoint (_, typ) | CommandLineFlag (_, typ)), SQL ->
         if is_injection_possible ~typ sanitizers then Some IssueType.sql_injection
         else
           (* no injection risk, but still user-controlled *)
@@ -431,12 +438,12 @@ include Trace.Make (struct
         else
           (* no injection risk, but still user-controlled *)
           Some IssueType.user_controlled_sql_risk
+    | (UserControlledEndpoint (_, typ) | CommandLineFlag (_, 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
     | 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
@@ -446,10 +453,10 @@ include Trace.Make (struct
     | (CommandLineFlag _ | EnvironmentVariable | ReadFile | Other), BufferAccess ->
         (* untrusted flag, environment var, or file data flowing to buffer *)
         Some IssueType.quandary_taint_error
-    | (CommandLineFlag _ | EnvironmentVariable | ReadFile | Other), ShellExec ->
+    | (EnvironmentVariable | ReadFile | Other), ShellExec ->
         (* untrusted flag, environment var, or file data flowing to shell *)
         Option.some_if (is_injection_possible sanitizers) IssueType.shell_injection
-    | (CommandLineFlag _ | EnvironmentVariable | ReadFile | Other), SQL ->
+    | (EnvironmentVariable | ReadFile | Other), SQL ->
         (* untrusted flag, environment var, or file data flowing to SQL *)
         Option.some_if (is_injection_possible sanitizers) IssueType.sql_injection
     | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | ReadFile | Other)
@@ -461,7 +468,7 @@ 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 ->
+    | (EnvironmentVariable | ReadFile), CreateFile ->
         None
     | Other, _ ->
         (* Other matches everything *)

infer/tests/codetoanalyze/cpp/quandary/execs.cpp

@@ -21,6 +21,8 @@ namespace execs {
 // mocking gflags-generated field
 extern char* FLAGS_cli_string;
 
+extern int FLAGS_cli_int;
+
 int callAllSinks(const char* stringSource, char ** arrSource) {
   switch (rand()) {
     case 1:
@@ -99,14 +101,20 @@ void customGetEnvOk() {
   return execl(NULL, source);
 }
 
-void exec_flag_bad() { execl(FLAGS_cli_string, NULL); }
+void exec_string_flag_bad() { execl(FLAGS_cli_string, NULL); }
+
+void exec_int_flag_ok() {
+  char buffer[25];
+  sprintf(buffer, "foo -i %d", FLAGS_cli_int);
+  execl(buffer, NULL);
+}
 
 char* return_global() {
   char* local = FLAGS_cli_string;
   return local;
 }
 
-void exec_flag_interproc_bad() {
+void exec_string_flag_interproc_bad() {
   char* flag = return_global();
   execl(flag, NULL);
 }

infer/tests/codetoanalyze/cpp/quandary/issues.exp

@@ -75,8 +75,8 @@ codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 29, SHELL_INJECTION, [
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 31, SHELL_INJECTION, [Return from getenv,Call to execve]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 33, SHELL_INJECTION, [Return from getenv,Call to system]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 35, SHELL_INJECTION, [Return from getenv,Call to popen]
-codetoanalyze/cpp/quandary/execs.cpp, execs::exec_flag_bad, 0, SHELL_INJECTION, [Return from __global_access,Call to execl]
+codetoanalyze/cpp/quandary/execs.cpp, execs::exec_string_flag_bad, 0, SHELL_INJECTION, [Return from __global_access,Call to execl]
-codetoanalyze/cpp/quandary/execs.cpp, execs::exec_flag_interproc_bad, 2, SHELL_INJECTION, [Return from __global_access with tainted data &return,Return from execs::return_global,Call to execl]
+codetoanalyze/cpp/quandary/execs.cpp, execs::exec_string_flag_interproc_bad, 2, SHELL_INJECTION, [Return from __global_access with tainted data &return,Return from execs::return_global,Call to execl]
 codetoanalyze/cpp/quandary/execs.cpp, execs::sql_on_env_var_bad, 2, SQL_INJECTION, [Return from getenv,Call to __infer_sql_sink]
 codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop1_bad, 3, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink]
 codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop2_bad, 5, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink]