[quandary] distinguish between SQL reads and writes

Reviewed By: mbouaziz

Differential Revision: D7781377

fbshipit-source-id: 9c76e4d

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 *)

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"
     },
     {

infer/tests/codetoanalyze/cpp/quandary/endpoints.cpp

@@ -13,6 +13,8 @@
 #include <string>
 
 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());
   }

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]