[quandary] report USER_CONTROLLED_SQL_RISK on flows from endpoint -> SQL

Reviewed By: mbouaziz

Differential Revision: D6338997

fbshipit-source-id: 19c4380

infer/src/base/IssueType.ml

@@ -311,4 +311,6 @@ let use_after_free = from_string "USE_AFTER_FREE"
 
 let untrusted_variable_length_array = from_string "UNTRUSTED_VARIABLE_LENGTH_ARRAY"
 
+let user_controlled_sql_risk = from_string "USER_CONTROLLED_SQL_RISK"
+
 let wrong_argument_number = from_string "Wrong_argument_number" ~hum:"Wrong Argument Number"

infer/src/base/IssueType.mli

@@ -220,4 +220,6 @@ val use_after_free : t
 
 val untrusted_variable_length_array : t
 
+val user_controlled_sql_risk : t
+
 val wrong_argument_number : t

infer/src/quandary/ClangTrace.ml

@@ -331,57 +331,63 @@ include Trace.Make (struct
   module Sink = CppSink
   module Sanitizer = CppSanitizer
 
-  let is_sanitized (sink_kind: SinkKind.t) sanitizers =
+  (* return true if code injection is possible because the source is a string/is not sanitized *)
-    match sanitizers with
+  let is_injection_possible ?typ sanitizers =
-    | [] ->
+    let is_escaped = List.mem sanitizers Sanitizer.Escape ~equal:Sanitizer.equal in
-        false
-    | _ when List.mem sanitizers Sanitizer.All ~equal:Sanitizer.equal ->
-        (* the All sanitizer clears any form of taint *)
-        true
-    | _ ->
-      match sink_kind with
-      | ShellExec | SQL ->
-          List.mem sanitizers Sanitizer.Escape ~equal:Sanitizer.equal
-      | _ ->
-          false
-
-
-  let get_report source sink sanitizers =
-    (* TODO: make this accept structs/objects too, but not primitive types or enumes *)
     (* using this to match custom string wrappers such as folly::StringPiece *)
     let is_stringy typ =
       let lowercase_typ = String.lowercase (Typ.to_string (Typ.mk typ)) in
       String.is_substring ~substring:"string" lowercase_typ
       || String.is_substring ~substring:"char*" lowercase_typ
     in
+    not is_escaped
+    &&
+    match typ with
+    | Some typ ->
+        is_stringy typ
+    | None ->
+        (* not sure if it's a string; assume injection possible *)
+        true
+
+
+  let get_report source sink sanitizers =
     match (Source.kind source, Sink.kind sink) with
-    | _, sink_kind when is_sanitized sink_kind sanitizers ->
+    | _ when List.mem sanitizers Sanitizer.All ~equal:Sanitizer.equal ->
-        (* trace is sanitized; don't report *)
+        (* the All sanitizer clears any form of taint; don't report *)
-        None
-    | Endpoint (_, typ), (ShellExec | SQL) ->
-        (* remote code execution if the caller of the endpoint doesn't sanitize *)
-        Option.some_if (is_stringy typ) IssueType.remote_code_execution_risk
-    | Endpoint _, (BufferAccess | HeapAllocation | StackAllocation) ->
-        (* may want to report this in the future, but don't care for now *)
         None
+    | UserControlledEndpoint (_, typ), SQL ->
+        if is_injection_possible ~typ sanitizers then Some IssueType.sql_injection
+        else
+          (* no injection risk, but still user-controlled *)
+          Some IssueType.user_controlled_sql_risk
+    | Endpoint (_, typ), SQL ->
+        if is_injection_possible ~typ sanitizers then
+          (* code injection if the caller of the endpoint doesn't sanitize on its end *)
+          Some IssueType.remote_code_execution_risk
+        else
+          (* no injection risk, but still user-controlled *)
+          Some IssueType.user_controlled_sql_risk
+    | 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
-    | UserControlledEndpoint (_, typ), ShellExec ->
+    | Endpoint _, (BufferAccess | HeapAllocation | StackAllocation) ->
-        (* untrusted string data flowing to shell ShellExec *)
+        (* may want to report this in the future, but don't care for now *)
-        Option.some_if (is_stringy typ) IssueType.shell_injection
+        None
-    | UserControlledEndpoint (_, typ), SQL ->
-        (* untrusted string data flowing to SQL *)
-        Option.some_if (is_stringy typ) IssueType.sql_injection
     | (CommandLineFlag _ | EnvironmentVariable | File | Other), BufferAccess ->
         (* untrusted flag, environment var, or file data flowing to buffer *)
         Some IssueType.quandary_taint_error
     | (CommandLineFlag _ | EnvironmentVariable | File | Other), ShellExec ->
         (* untrusted flag, environment var, or file data flowing to shell *)
-        Some IssueType.shell_injection
+        Option.some_if (is_injection_possible sanitizers) IssueType.shell_injection
     | (CommandLineFlag _ | EnvironmentVariable | File | Other), SQL ->
         (* untrusted flag, environment var, or file data flowing to SQL *)
-        Some IssueType.sql_injection
+        Option.some_if (is_injection_possible sanitizers) IssueType.sql_injection
     | ( (CommandLineFlag _ | UserControlledEndpoint _ | EnvironmentVariable | File | Other)
       , HeapAllocation ) ->
         (* untrusted data of any kind flowing to heap allocation. this can cause crashes or DOS. *)

infer/tests/codetoanalyze/cpp/quandary/.inferconfig

@@ -81,6 +81,8 @@
   ],
   "quandary-endpoints": [
     "basics::Obj::endpoint",
+    "endpoints::Service1::user_controlled_endpoint_to_sql_bad",
+    "endpoints::Service1::user_controlled_endpoint_to_shell_bad",
     "execs::Obj::endpoint"
   ]
 }

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

@@ -9,6 +9,8 @@
 
 #include <string>
 
+extern void __infer_sql_sink(std::string);
+extern std::string __infer_all_sanitizer(std::string);
 extern std::string __infer_string_sanitizer(std::string);
 
 namespace facebook {
@@ -27,9 +29,38 @@ namespace endpoints {
 class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf {
 
  public:
-  void service1_endpoint_bad(std::string formal) { system(formal.c_str()); }
+  void service1_endpoint_bad(std::string formal) {
+    // this should report REMOTE_CODE_EXECUTION_RISK
+    system(formal.c_str());
+  }
+
+  // this is specified as user-controlled in .inferconfig
+  void user_controlled_endpoint_to_sql_bad(std::string formal) {
+    // this should report SQL_INJECTION
+    __infer_sql_sink(formal);
+  }
+
+  // this is specified as user-controlled in .inferconfig
+  void user_controlled_endpoint_to_shell_bad(std::string formal) {
+    // this should report SHELL_INJECTION
+    system(formal.c_str());
+  }
+
+  void unsanitized_sql_bad(std::string formal) {
+    // this should report REMOTE_CODE_EXECUTION_RISK
+    __infer_sql_sink(formal);
+  }
 
-  void service1_endpoint_sanitized_ok(std::string formal) {
+  void sanitized_sql_bad(std::string formal) {
+    // this should report USER_CONTROLLED_SQL_RISK
+    __infer_sql_sink(__infer_string_sanitizer(formal));
+  }
+
+  void service1_endpoint_sql_sanitized_ok(std::string formal) {
+    __infer_sql_sink(__infer_all_sanitizer(formal));
+  }
+
+  void service1_endpoint_shell_sanitized_ok(std::string formal) {
     system(__infer_string_sanitizer(formal).c_str());
   }
 
@@ -40,12 +71,18 @@ class Service1 : facebook::fb303::cpp2::FacebookServiceSvIf {
 class Service2 : facebook::fb303::cpp2::FacebookServiceSvAsyncIf {
 
  public:
-  void service2_endpoint_bad(std::string formal) { system(formal.c_str()); }
+  void service2_endpoint_bad(std::string formal) {
+    // this should report REMOTE_CODE_EXECUTION_RISK
+    system(formal.c_str());
+  }
 };
 
 class Service3 : Service1 {
  public:
-  void service3_endpoint_bad(std::string formal) { system(formal.c_str()); }
+  void service3_endpoint_bad(std::string formal) {
+    // this should report REMOTE_CODE_EXECUTION_RISK
+    system(formal.c_str());
+  }
 };
 
 } // namespace endpoints

infer/tests/codetoanalyze/cpp/quandary/issues.exp

@@ -36,9 +36,13 @@ codetoanalyze/cpp/quandary/basics.cpp, basics::via_field_bad1, 3, QUANDARY_TAINT
 codetoanalyze/cpp/quandary/basics.cpp, basics::via_field_bad2, 2, QUANDARY_TAINT_ERROR, [Return from basics::template_source<std::basic_string<char>_>,Call to basics::template_sink<std::basic_string<char>_>]
 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_service1_endpoint_bad, 0, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_service1_endpoint_bad,Call to system]
+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::Service2_service2_endpoint_bad, 0, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service2_service2_endpoint_bad,Call to system]
+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::Service3_service3_endpoint_bad, 0, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service3_service3_endpoint_bad,Call to system]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_unsanitized_sql_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service1_unsanitized_sql_bad,Call to __infer_sql_sink]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_endpoint_to_shell_bad, 2, SHELL_INJECTION, [Return from endpoints::Service1_user_controlled_endpoint_to_shell_bad,Call to system]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service1_user_controlled_endpoint_to_sql_bad, 2, SQL_INJECTION, [Return from endpoints::Service1_user_controlled_endpoint_to_sql_bad,Call to __infer_sql_sink]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service2_service2_endpoint_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service2_service2_endpoint_bad,Call to system]
+codetoanalyze/cpp/quandary/endpoints.cpp, endpoints::Service3_service3_endpoint_bad, 2, REMOTE_CODE_EXECUTION_RISK, [Return from endpoints::Service3_service3_endpoint_bad,Call to system]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 6, SHELL_INJECTION, [Return from getenv,Call to execl]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 8, SHELL_INJECTION, [Return from getenv,Call to execl]
 codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 11, SHELL_INJECTION, [Return from getenv,Call to execl]