diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index 4ad636705..71d9cf338 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -200,7 +200,9 @@ module SinkKind = struct | CreateFile (** create/open a file *) | HeapAllocation (** heap memory allocation *) | ShellExec (** shell exec function *) - | SQL (** SQL query *) + | SQLInjection (** unescaped query to a SQL database (could be read or write) *) + | SQLRead (** escaped read to a SQL database *) + | SQLWrite (** escaped write to a SQL database *) | StackAllocation (** stack memory allocation *) | URL (** URL creation *) | Other (** for testing or uncategorized sinks *) @@ -217,8 +219,12 @@ module SinkKind = struct HeapAllocation | "ShellExec" -> ShellExec - | "SQL" -> - SQL + | "SQLInjection" -> + SQLInjection + | "SQLRead" -> + SQLRead + | "SQLWrite" -> + SQLWrite | "StackAllocation" -> StackAllocation | "URL" -> @@ -365,8 +371,12 @@ module SinkKind = struct "HeapAllocation" | ShellExec -> "ShellExec" - | SQL -> - "SQL" + | SQLInjection -> + "SQLInjection" + | SQLRead -> + "SQLRead" + | SQLWrite -> + "SQLWrite" | StackAllocation -> "StackAllocation" | URL -> @@ -459,13 +469,16 @@ include Trace.Make (struct IssueType.untrusted_url_risk | (CommandLineFlag _ | EnvironmentVariable | ReadFile), URL -> None - | (Endpoint (_, typ) | UserControlledEndpoint (_, typ)), SQL -> + | (Endpoint (_, typ) | UserControlledEndpoint (_, typ)), SQLInjection -> if is_injection_possible ~typ Sanitizer.EscapeSQL sanitizers then (* SQL injection if the caller of the endpoint doesn't sanitize on its end *) Some IssueType.sql_injection_risk else (* no injection risk, but still user-controlled *) Some IssueType.user_controlled_sql_risk + | (Endpoint _ | UserControlledEndpoint _), (SQLRead | SQLWrite) -> + (* no injection risk, but still user-controlled *) + Some IssueType.user_controlled_sql_risk | (Endpoint (_, typ) | UserControlledEndpoint (_, typ)), ShellExec -> (* code injection if the caller of the endpoint doesn't sanitize on its end *) Option.some_if @@ -490,12 +503,12 @@ include Trace.Make (struct Option.some_if (is_injection_possible ~typ Sanitizer.EscapeShell sanitizers) IssueType.shell_injection - | (EnvironmentVariable | ReadFile | Other), SQL -> + | (EnvironmentVariable | ReadFile | Other), SQLInjection -> (* untrusted flag, environment var, or file data flowing to SQL *) Option.some_if (is_injection_possible Sanitizer.EscapeSQL sanitizers) IssueType.sql_injection - | CommandLineFlag (_, typ), SQL -> + | CommandLineFlag (_, typ), SQLInjection -> (* untrusted flag, flowing to shell *) Option.some_if (is_injection_possible ~typ Sanitizer.EscapeSQL sanitizers) @@ -514,7 +527,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 -> + | (CommandLineFlag _ | EnvironmentVariable | ReadFile), (CreateFile | SQLRead | SQLWrite) -> None | Other, _ -> (* Other matches everything *) diff --git a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig index bb8d77818..d69bff779 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig +++ b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig @@ -39,7 +39,17 @@ }, { "procedure": "__infer_sql_sink", - "kind": "SQL", + "kind": "SQLInjection", + "index": "all" + }, + { + "procedure": "__infer_sql_read_sink", + "kind": "SQLRead", + "index": "all" + }, + { + "procedure": "__infer_sql_write_sink", + "kind": "SQLWrite", "index": "all" }, { diff --git a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp index c58b2bc72..12a7fb3e4 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp @@ -13,6 +13,8 @@ #include extern void __infer_sql_sink(std::string); +extern void __infer_sql_read_sink(std::string); +extern void __infer_sql_write_sink(std::string); extern std::string __infer_shell_sanitizer(std::string); extern std::string __infer_sql_sanitizer(std::string); @@ -71,6 +73,16 @@ class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf { __infer_sql_sink(__infer_sql_sanitizer(formal)); } + void service1_endpoint_sql_read_bad(std::string formal) { + // this should report USER_CONTROLLED_SQL_RISK + __infer_sql_read_sink(formal); + } + + void service1_endpoint_sql_write_bad(std::string formal) { + // this should report USER_CONTROLLED_SQL_RISK + __infer_sql_write_sink(formal); + } + void service1_endpoint_shell_sanitized_ok(std::string formal) { system(__infer_shell_sanitizer(formal).c_str()); } diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index 5fbf3669e..9c298d646 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -54,7 +54,9 @@ codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_s codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_open_or_create_c_style_file_bad, 6, UNTRUSTED_FILE_RISK, ERROR, [Return from endpoints::Service1_open_or_create_c_style_file_bad,Call to rename with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_sanitized_sql_with_shell_bad, 2, SQL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_sanitized_sql_with_shell_bad,Call to __infer_sql_sink with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_bad, 2, SHELL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_service1_endpoint_bad,Call to system with tainted index 0] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_sql_read_bad, 2, USER_CONTROLLED_SQL_RISK, ERROR, [Return from endpoints::Service1_service1_endpoint_sql_read_bad,Call to __infer_sql_read_sink with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_sql_sanitized_bad, 2, USER_CONTROLLED_SQL_RISK, ERROR, [Return from endpoints::Service1_service1_endpoint_sql_sanitized_bad,Call to __infer_sql_sink with tainted index 0] +codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_sql_write_bad, 2, USER_CONTROLLED_SQL_RISK, ERROR, [Return from endpoints::Service1_service1_endpoint_sql_write_bad,Call to __infer_sql_write_sink with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_service1_endpoint_struct_string_field_bad, 1, SHELL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_service1_endpoint_struct_string_field_bad,Call to system with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_unsanitized_sql_bad, 2, SQL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_unsanitized_sql_bad,Call to __infer_sql_sink with tainted index 0] codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_endpoint_to_shell_bad, 2, SHELL_INJECTION_RISK, ERROR, [Return from endpoints::Service1_user_controlled_endpoint_to_shell_bad,Call to system with tainted index 0]