[quandary] only treat overrides of service methods as endpoints

Reviewed By: mbouaziz

Differential Revision: D8010176

fbshipit-source-id: 14a4b32

infer/src/absint/PatternMatch.ml

@@ -278,7 +278,7 @@ let proc_calls resolve_attributes pdesc filter : (Typ.Procname.t * ProcAttribute
   List.rev !res
 
 
-let override_exists f tenv proc_name =
+let override_exists ?(check_current_type= true) f tenv proc_name =
   let method_name = Typ.Procname.get_method proc_name in
   let rec super_type_exists_ tenv super_class_name =
     match Tenv.lookup tenv super_class_name with
@@ -297,7 +297,7 @@ let override_exists f tenv proc_name =
     List.exists ~f:(super_type_exists_ tenv)
       (type_get_direct_supertypes tenv (Typ.mk (Tstruct type_name)))
   in
-  f proc_name
+  (check_current_type && f proc_name)
   ||
   match proc_name with
   | Typ.Procname.Java proc_name_java ->

infer/src/absint/PatternMatch.mli

@@ -50,9 +50,9 @@ val proc_calls :
   -> (Typ.Procname.t -> ProcAttributes.t -> bool) -> (Typ.Procname.t * ProcAttributes.t) list
 (** Return the callees that satisfy [filter]. *)
 
-val override_exists : (Typ.Procname.t -> bool) -> Tenv.t -> Typ.Procname.t -> bool
-(** Return true if applying the given predicate to an override of [procname] or [procname] itself
-    returns true. For the moment, this only works for Java *)
+val override_exists :
+  ?check_current_type:bool -> (Typ.Procname.t -> bool) -> Tenv.t -> Typ.Procname.t -> bool
+(** Return true if applying the given predicate to an override of [procname] (including [procname] itself when [check_current_type] is true, which it is by default) returns true. *)
 
 val override_iter : (Typ.Procname.t -> unit) -> Tenv.t -> Typ.Procname.t -> unit
 (** Apply the given predicate to procname and each override of [procname]. For the moment, this only

infer/src/quandary/JavaTrace.ml

@@ -171,7 +171,7 @@ module SourceKind = struct
     in
     let formals = Procdesc.get_formals pdesc in
     match Procdesc.get_proc_name pdesc with
-    | Typ.Procname.Java java_pname
+    | Typ.Procname.Java java_pname as pname
       -> (
         let method_name = Typ.Procname.Java.get_method java_pname in
         let taint_matching_supertype typename =
@@ -217,7 +217,10 @@ module SourceKind = struct
             | Some typ ->
                 if
                   Annotations.struct_typ_has_annot typ Annotations.ia_is_thrift_service
-                  && PredSymb.equal_access (Procdesc.get_access pdesc) PredSymb.Public
+                  && PatternMatch.override_exists ~check_current_type:false
+                       (fun superclass_pname ->
+                         String.equal (Typ.Procname.get_method superclass_pname) method_name )
+                       tenv pname
                 then
                   (* assume every non-this formal of a Thrift service is tainted *)
                   (* TODO: may not want to taint numbers or Enum's *)

infer/tests/codetoanalyze/java/quandary/Services.java

@@ -20,72 +20,78 @@ class Services {
 
 }
 
-// classes annotated with @ThriftService are servers (sources), whereas interfaces
-// annotated with @ThriftService are clients (sinks): see
-// https://github.com/facebook/swift/blob/master/swift-service/README.md#clients-and-servers
 @Retention(RetentionPolicy.CLASS)
 @interface ThriftService {
 }
 
-
 @ThriftService
-class Service1 {
+interface GeneratedServiceInterface {
+  public void serviceMethodBad(String s) throws IOException;
+  public void paramToSql1Bad(String s) throws SQLException;
+  public void paramToSql2Bad(String s) throws SQLException;
+  public void paramToSql3Bad(String s) throws SQLException;
+  public void paramToSql4Bad(String s) throws SQLException;
+  public void paramToSql5Bad(String s) throws SQLException;
+  void packageProtectedServiceMethodBad(String s) throws IOException;
+}
 
+class Service1 implements GeneratedServiceInterface {
+
+  @Override
   public void serviceMethodBad(String s) throws IOException {
     Runtime.getRuntime().exec(s); // RCE if s is tainted, we should warn
   }
 
   Statement mStatement;
 
+  @Override
   public void paramToSql1Bad(String s) throws SQLException {
     mStatement.execute(s);
   }
 
+  @Override
   public void paramToSql2Bad(String s) throws SQLException {
     mStatement.executeLargeUpdate(s);
   }
 
+  @Override
   public void paramToSql3Bad(String s) throws SQLException {
     mStatement.executeQuery(s);
   }
 
+  @Override
   public void paramToSql4Bad(String s) throws SQLException {
     mStatement.executeUpdate(s);
   }
 
+  @Override
   public void paramToSql5Bad(String s) throws SQLException {
     mStatement.addBatch(s);
     mStatement.executeBatch();
   }
 
-  // assume protected methods aren't exported to Thrift
-  protected void protectedServiceMethodOk(String s) throws IOException {
+  @Override
+  public void packageProtectedServiceMethodBad(String s) throws IOException {
     Runtime.getRuntime().exec(s);
   }
 
-  // assume package-protected methods aren't exported to Thrift
-  void packageProtectedServiceMethodOk(String s) throws IOException {
+  // doesn't override a method from the service interface; not an endpoint
+  public void publicMethodNotEndpointOk(String s) throws IOException {
     Runtime.getRuntime().exec(s);
   }
 
-  // private methods can't be exported to thrift
-  private void privateMethodNotEndpointOk(String s) throws IOException {
+  // same
+  protected void protectedMethodNotEndpointOk(String s) throws IOException {
     Runtime.getRuntime().exec(s);
   }
 
-}
-
-@ThriftService
-interface ThriftInterface {
-
-  public void interfaceServiceMethodBad(String s) throws IOException;
-}
-
-// this is a service too
-class Implementer implements ThriftInterface {
+  void packageProtectedMethodNotEndpointOk(String s) throws IOException {
+    Runtime.getRuntime().exec(s);
+  }
 
-  public void interfaceServiceMethodBad(String s) throws IOException {
-    Runtime.getRuntime().exec(s); // RCE if s is tainted, we should warn
+  // same
+  private void privateMethodNotEndpointOk(String s) throws IOException {
+    Runtime.getRuntime().exec(s);
   }
 
 }

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

@@ -177,7 +177,7 @@ codetoanalyze/java/quandary/LoggingPrivateData.java, void LoggingPrivateData.log
 codetoanalyze/java/quandary/Recursion.java, void Recursion.callSinkThenDivergeBad(), 1, QUANDARY_TAINT_ERROR, ERROR, [Return from Object InferTaint.inferSecretSource(),Call to void Recursion.callSinkThenDiverge(Object) with tainted index 0,Call to void InferTaint.inferSensitiveSink(Object) with tainted index 0]
 codetoanalyze/java/quandary/Recursion.java, void Recursion.safeRecursionCallSinkBad(), 1, QUANDARY_TAINT_ERROR, ERROR, [Return from Object InferTaint.inferSecretSource(),Call to void Recursion.safeRecursionCallSink(int,Object) with tainted index 0,Call to void InferTaint.inferSensitiveSink(Object) with tainted index 0]
 codetoanalyze/java/quandary/Serialization.java, Object Serialization.taintedObjectInputStreamBad(), 2, QUANDARY_TAINT_ERROR, ERROR, [Return from Object InferTaint.inferSecretSource(),Call to ObjectInputStream.<init>(InputStream) with tainted index 1]
-codetoanalyze/java/quandary/Services.java, void Implementer.interfaceServiceMethodBad(String), 1, SHELL_INJECTION_RISK, ERROR, [Return from void Implementer.interfaceServiceMethodBad(String),Call to Process Runtime.exec(String) with tainted index 1]
+codetoanalyze/java/quandary/Services.java, void Service1.packageProtectedServiceMethodBad(String), 1, SHELL_INJECTION_RISK, ERROR, [Return from void Service1.packageProtectedServiceMethodBad(String),Call to Process Runtime.exec(String) with tainted index 1]
 codetoanalyze/java/quandary/Services.java, void Service1.paramToSql1Bad(String), 1, SQL_INJECTION_RISK, ERROR, [Return from void Service1.paramToSql1Bad(String),Call to boolean Statement.execute(String) with tainted index 1]
 codetoanalyze/java/quandary/Services.java, void Service1.paramToSql2Bad(String), 1, USER_CONTROLLED_SQL_RISK, ERROR, [Return from void Service1.paramToSql2Bad(String),Call to long Statement.executeLargeUpdate(String) with tainted index 1]
 codetoanalyze/java/quandary/Services.java, void Service1.paramToSql3Bad(String), 1, USER_CONTROLLED_SQL_RISK, ERROR, [Return from void Service1.paramToSql3Bad(String),Call to ResultSet Statement.executeQuery(String) with tainted index 1]