From 82a3b2649ec3a7941923d10489351064c97028b6 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Sat, 16 Dec 2017 07:31:52 -0800 Subject: [PATCH] [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 | 35 +++++++++++-------- .../codetoanalyze/cpp/quandary/execs.cpp | 12 +++++-- .../codetoanalyze/cpp/quandary/issues.exp | 4 +-- 3 files changed, 33 insertions(+), 18 deletions(-) diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index 8fb01807e..60d5dee5f 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/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 *) diff --git a/infer/tests/codetoanalyze/cpp/quandary/execs.cpp b/infer/tests/codetoanalyze/cpp/quandary/execs.cpp index 17c113220..46254cd74 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/execs.cpp +++ b/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); } diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index ea66d8516..304928f3f 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/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_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_bad, 0, SHELL_INJECTION, [Return from __global_access,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]