From 6abbe66ee6a78063bce16fed404498118724c88e Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 7 Dec 2017 13:36:32 -0800 Subject: [PATCH] [quandary] file creation as sink Reviewed By: jeremydubreil, mbouaziz Differential Revision: D6486526 fbshipit-source-id: cad09f1 --- infer/src/base/IssueType.ml | 4 ++ infer/src/base/IssueType.mli | 4 ++ infer/src/quandary/ClangTrace.ml | 50 ++++++++++++++----- .../codetoanalyze/cpp/quandary/endpoints.cpp | 29 +++++++++++ .../codetoanalyze/cpp/quandary/files.cpp | 1 + .../codetoanalyze/cpp/quandary/issues.exp | 12 +++++ 6 files changed, 87 insertions(+), 13 deletions(-) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index a5dc1ebfc..c332b820b 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -321,6 +321,10 @@ let unsafe_guarded_by_access = from_string "UNSAFE_GUARDED_BY_ACCESS" let use_after_free = from_string "USE_AFTER_FREE" +let untrusted_file = from_string "UNTRUSTED_FILE" + +let untrusted_file_risk = from_string "UNTRUSTED_FILE_RISK" + let untrusted_variable_length_array = from_string "UNTRUSTED_VARIABLE_LENGTH_ARRAY" let user_controlled_sql_risk = from_string "USER_CONTROLLED_SQL_RISK" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 3487012df..cd745acd6 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -230,6 +230,10 @@ val unsafe_guarded_by_access : t val use_after_free : t +val untrusted_file : t + +val untrusted_file_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 aa5f4f036..e44807c1f 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -16,7 +16,7 @@ module SourceKind = struct | CommandLineFlag of Var.t (** 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 *) - | File (** source that was read from a file *) + | ReadFile (** source that was read from a file *) | Other (** for testing or uncategorized sources *) | UserControlledEndpoint of (Mangled.t * Typ.desc) (** source originating from formal of an endpoint that is known to hold user-controlled data *) @@ -31,8 +31,8 @@ module SourceKind = struct Endpoint (Mangled.from_string "NONE", Typ.Tvoid) | "EnvironmentVariable" -> EnvironmentVariable - | "File" -> - File + | "ReadFile" -> + ReadFile | "UserControlledEndpoint" -> Endpoint (Mangled.from_string "NONE", Typ.Tvoid) | _ -> @@ -73,7 +73,7 @@ module SourceKind = struct with | ( ["std"; ("basic_istream" | "basic_iostream")] , ("getline" | "read" | "readsome" | "operator>>") ) -> - Some (File, Some 1) + Some (ReadFile, Some 1) | _ -> get_external_source qualified_pname ) | Typ.Procname.C _ when Typ.Procname.equal pname BuiltinDecl.__global_access @@ -159,7 +159,7 @@ module SourceKind = struct F.fprintf fmt "Endpoint[%s]" (Mangled.to_string formal_name) | EnvironmentVariable -> F.fprintf fmt "EnvironmentVariable" - | File -> + | ReadFile -> F.fprintf fmt "File" | CommandLineFlag var -> F.fprintf fmt "CommandLineFlag[%a]" Var.pp var @@ -175,6 +175,7 @@ module CppSource = Source.Make (SourceKind) module SinkKind = struct type t = | BufferAccess (** read/write an array *) + | CreateFile (** create/open a file *) | HeapAllocation (** heap memory allocation *) | ShellExec (** shell exec function *) | SQL (** SQL query *) @@ -187,6 +188,8 @@ module SinkKind = struct let of_string = function | "BufferAccess" -> BufferAccess + | "CreateFile" -> + CreateFile | "HeapAllocation" -> HeapAllocation | "ShellExec" -> @@ -246,9 +249,16 @@ module SinkKind = struct || String.is_substring ~substring:"string" typename in match pname with - | Typ.Procname.ObjC_Cpp _ -> ( - match Typ.Procname.get_method pname with - | "operator[]" when Config.developer_mode && is_buffer_like pname -> + | Typ.Procname.ObjC_Cpp cpp_name -> ( + match + ( QualifiedCppName.to_list + (Typ.Name.unqualified_name (Typ.Procname.objc_cpp_get_class_type_name cpp_name)) + , Typ.Procname.get_method pname ) + with + | ( ["std"; ("basic_fstream" | "basic_ifstream" | "basic_ofstream")] + , ("basic_fstream" | "basic_ifstream" | "basic_ofstream" | "open") ) -> + taint_nth 1 CreateFile actuals + | _, "operator[]" when Config.developer_mode && is_buffer_like pname -> taint_nth 1 BufferAccess actuals | _ -> get_external_sink pname actuals ) @@ -260,12 +270,18 @@ module SinkKind = struct taint_nth 1 StackAllocation actuals | Typ.Procname.C _ -> ( match Typ.Procname.to_string pname with + | "creat" | "fopen" | "freopen" | "open" -> + taint_nth 0 CreateFile actuals | "execl" | "execlp" | "execle" | "execv" | "execve" | "execvp" | "system" -> taint_all ShellExec actuals + | "openat" -> + taint_nth 1 CreateFile actuals | "popen" -> taint_nth 0 ShellExec actuals | ("brk" | "calloc" | "malloc" | "realloc" | "sbrk") when Config.developer_mode -> taint_all HeapAllocation actuals + | "rename" -> + taint_all CreateFile actuals | "strcpy" when Config.developer_mode -> (* warn if source array is tainted *) taint_nth 1 BufferAccess actuals @@ -291,6 +307,8 @@ module SinkKind = struct ( match kind with | BufferAccess -> "BufferAccess" + | CreateFile -> + "CreateFile" | HeapAllocation -> "HeapAllocation" | ShellExec -> @@ -358,6 +376,10 @@ include Trace.Make (struct | _ when List.mem sanitizers Sanitizer.All ~equal:Sanitizer.equal -> (* the All sanitizer clears any form of taint; don't report *) None + | UserControlledEndpoint (_, typ), CreateFile -> + Option.some_if (is_injection_possible ~typ sanitizers) IssueType.untrusted_file + | Endpoint (_, typ), CreateFile -> + Option.some_if (is_injection_possible ~typ sanitizers) IssueType.untrusted_file_risk | UserControlledEndpoint (_, typ), SQL -> if is_injection_possible ~typ sanitizers then Some IssueType.sql_injection else @@ -382,24 +404,26 @@ include Trace.Make (struct | Endpoint _, (BufferAccess | HeapAllocation | StackAllocation) -> (* may want to report this in the future, but don't care for now *) None - | (CommandLineFlag _ | EnvironmentVariable | File | Other), BufferAccess -> + | (CommandLineFlag _ | EnvironmentVariable | ReadFile | Other), BufferAccess -> (* untrusted flag, environment var, or file data flowing to buffer *) Some IssueType.quandary_taint_error - | (CommandLineFlag _ | EnvironmentVariable | File | Other), ShellExec -> + | (CommandLineFlag _ | 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 | File | Other), SQL -> + | (CommandLineFlag _ | 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 | File | Other) + | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | ReadFile | Other) , HeapAllocation ) -> (* untrusted data of any kind flowing to heap allocation. this can cause crashes or DOS. *) Some IssueType.quandary_taint_error - | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | File | Other) + | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | ReadFile | Other) , StackAllocation ) -> (* 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 -> + None | Other, _ -> (* Other matches everything *) Some IssueType.quandary_taint_error diff --git a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp index a3d6d72a5..a851954da 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp @@ -7,6 +7,8 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +#include +#include #include extern void __infer_sql_sink(std::string); @@ -73,6 +75,33 @@ class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf { system(formal.s.c_str()); } + void open_or_create_c_style_file_bad(const char* filename) { + open(filename, 0); + openat(1, filename, 2); + creat(filename, 3); + fopen(filename, "w"); + freopen(filename, "w", nullptr); + rename(filename, "mud"); + } + + void ofstream_open_file_bad(std::string filename) { + std::ofstream file1(filename); + std::ofstream file2; + file2.open(filename); + } + + void ifstream_open_file_bad(std::string filename) { + std::ifstream file1(filename); + std::ifstream file2; + file2.open(filename); + } + + void fstream_open_file_bad(std::string filename) { + std::fstream file1(filename); + std::fstream file2; + file2.open(filename); + } + void FP_service1_endpoint_struct_int_field_ok(request formal) { system(std::to_string(formal.i).c_str()); } diff --git a/infer/tests/codetoanalyze/cpp/quandary/files.cpp b/infer/tests/codetoanalyze/cpp/quandary/files.cpp index e82f5ec26..250d7c357 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/files.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/files.cpp @@ -61,4 +61,5 @@ void read_file_call_exec_bad5(std::iostream is, int length) { execle(buffer, NULL); } } + } diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index 61cbce947..35a49b09c 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -37,6 +37,18 @@ codetoanalyze/cpp/quandary/basics.cpp, basics::via_field_bad2, 2, QUANDARY_TAINT codetoanalyze/cpp/quandary/basics.cpp, basics::via_passthrough_bad1, 4, QUANDARY_TAINT_ERROR, [Return from basics::Obj_string_source,Call to basics::Obj_string_sink] codetoanalyze/cpp/quandary/basics.cpp, basics::via_passthrough_bad2, 3, QUANDARY_TAINT_ERROR, [Return from basics::Obj_string_source,Call to basics::Obj_string_sink] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_FP_service1_endpoint_struct_int_field_ok, 1, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_FP_service1_endpoint_struct_int_field_ok,Call to system] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_fstream_open_file_bad, 1, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_fstream_open_file_bad,Call to std::basic_fstream>_basic_fstream] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_fstream_open_file_bad, 3, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_fstream_open_file_bad,Call to std::basic_fstream>_open] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_ifstream_open_file_bad, 1, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_ifstream_open_file_bad,Call to std::basic_ifstream>_basic_ifstream] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_ifstream_open_file_bad, 3, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_ifstream_open_file_bad,Call to std::basic_ifstream>_open] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_ofstream_open_file_bad, 1, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_ofstream_open_file_bad,Call to std::basic_ofstream>_basic_ofstream] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_ofstream_open_file_bad, 3, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_ofstream_open_file_bad,Call to std::basic_ofstream>_open] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 1, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to open] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 2, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to openat] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 3, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to creat] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 4, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to fopen] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 5, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to freopen] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 6, UNTRUSTED_FILE_RISK, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to rename] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_sanitized_sql_bad, 2, USER_CONTROLLED_SQL_RISK, [Return from endpoints::Service1_sanitized_sql_bad,Call to __infer_sql_sink] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_service1_endpoint_bad,Call to system] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_struct_string_field_bad, 1, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_service1_endpoint_struct_string_field_bad,Call to system]