From 45aaa4da935b61c0ec11c2547652e79a094b5fc2 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 18 May 2017 09:48:53 -0700 Subject: [PATCH] [quandary] gflag globals as source Summary: Gflags is a popular library used to create command line arguments. Flags shouldn't flow directly to `exec` etc. Reviewed By: jvillard, mbouaziz Differential Revision: D5058393 fbshipit-source-id: ab062f8 --- infer/src/checkers/Trace.ml | 4 +- infer/src/quandary/ClangTrace.ml | 19 ++++++++-- infer/src/quandary/JavaTrace.ml | 37 ++++++++++--------- .../codetoanalyze/cpp/quandary/execs.cpp | 6 +++ .../codetoanalyze/cpp/quandary/issues.exp | 1 + 5 files changed, 44 insertions(+), 23 deletions(-) diff --git a/infer/src/checkers/Trace.ml b/infer/src/checkers/Trace.ml index 918857372..b4eebedf7 100644 --- a/infer/src/checkers/Trace.ml +++ b/infer/src/checkers/Trace.ml @@ -189,9 +189,7 @@ module Make (Spec : Spec) = struct else acc in Sinks.fold report_one sinks acc0 in let report_sources source acc = - if Source.is_footprint source - then acc - else report_source source t.sinks acc in + report_source source t.sinks acc in Sources.fold report_sources t.sources [] let pp_path cur_pname fmt (cur_passthroughs, sources_passthroughs, sinks_passthroughs) = diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index cc1b851ca..11a6a44eb 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -182,10 +182,23 @@ include | (EnvironmentVariable | File), Allocation -> (* untrusted data flowing to memory allocation *) true - | Other, _ - | _, Other -> + | _, (Allocation | Other | ShellExec) when Source.is_footprint source -> + (* is this var a command line flag created by the popular gflags library? *) + let is_gflag pvar = + String.is_substring ~substring:"FLAGS_" (Pvar.get_simplified_name pvar) in + begin + match Option.map ~f:AccessPath.extract (Source.get_footprint_access_path source) with + | Some ((Var.ProgramVar pvar, _), _) when Pvar.is_global pvar && is_gflag pvar -> + (* gflags globals come from the environment; treat them as sources *) + true + | _ -> + false + end + | Other, _ -> (* Other matches everything *) true - | _ -> + | _, Other -> + true + | Unknown, (Allocation | ShellExec) -> false end) diff --git a/infer/src/quandary/JavaTrace.ml b/infer/src/quandary/JavaTrace.ml index 3d60c4b3f..4ad1cf944 100644 --- a/infer/src/quandary/JavaTrace.ml +++ b/infer/src/quandary/JavaTrace.ml @@ -339,21 +339,24 @@ include module Sink = JavaSink let should_report source sink = - match Source.kind source, Sink.kind sink with - | PrivateData, Logging (* logging private data issue *) - | Intent, StartComponent (* intent reuse issue *) - | Intent, CreateIntent (* intent configured with external values issue *) - | Intent, JavaScript (* external data flows into JS: remote code execution risk *) - | PrivateData, JavaScript (* leaking private data into JS *) - | UserControlledURI, (CreateIntent | StartComponent) - (* create intent/launch component from user-controlled URI *) - | UserControlledURI, CreateFile - (* create file from user-controller URI; potential path-traversal vulnerability *) - | Clipboard, (StartComponent | CreateIntent | JavaScript | CreateFile) -> - (* do something sensitive with user-controlled data from the clipboard *) - true - | Other, _ | _, Other -> (* for testing purposes, Other matches everything *) - true - | _ -> - false + if Source.is_footprint source + then false + else + match Source.kind source, Sink.kind sink with + | PrivateData, Logging (* logging private data issue *) + | Intent, StartComponent (* intent reuse issue *) + | Intent, CreateIntent (* intent configured with external values issue *) + | Intent, JavaScript (* external data flows into JS: remote code execution risk *) + | PrivateData, JavaScript (* leaking private data into JS *) + | UserControlledURI, (CreateIntent | StartComponent) + (* create intent/launch component from user-controlled URI *) + | UserControlledURI, CreateFile + (* create file from user-controller URI; potential path-traversal vulnerability *) + | Clipboard, (StartComponent | CreateIntent | JavaScript | CreateFile) -> + (* do something sensitive with user-controlled data from the clipboard *) + true + | Other, _ | _, Other -> (* for testing purposes, Other matches everything *) + true + | _ -> + false end) diff --git a/infer/tests/codetoanalyze/cpp/quandary/execs.cpp b/infer/tests/codetoanalyze/cpp/quandary/execs.cpp index ac81223b7..357ecacd0 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/execs.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/execs.cpp @@ -12,8 +12,12 @@ extern int rand(); +// mocking gflags-generated field + namespace execs { +extern char* FLAGS_cli_string; + int callAllSinks(const char* stringSource, char ** arrSource) { switch (rand()) { case 1: @@ -88,4 +92,6 @@ void customGetEnvOk() { const char* source = execs::getenv("ENV_VAR"); return execl(NULL, source); } + +void exec_flag_bad() { execl(FLAGS_cli_string, NULL); } } diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index 0deab8bd6..18ba6fc2f 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -32,6 +32,7 @@ codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 27, QUANDARY_TAINT_ERR codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 29, QUANDARY_TAINT_ERROR, [return from getenv,call to execve] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 31, QUANDARY_TAINT_ERROR, [return from getenv,call to execve] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 33, QUANDARY_TAINT_ERROR, [return from getenv,call to system] +codetoanalyze/cpp/quandary/execs.cpp, execs::exec_flag_bad, 0, QUANDARY_TAINT_ERROR, [return from execs::exec_flag_bad,call to execl] codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad1, 5, QUANDARY_TAINT_ERROR, [return from std::basic_istream>_read,call to execle] codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad2, 5, QUANDARY_TAINT_ERROR, [return from std::basic_istream>_readsome,call to execle] codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad3, 5, QUANDARY_TAINT_ERROR, [return from std::basic_istream>_getline,call to execle]