From 370f5c80e6cceee0239feae88a49b589a5230fd7 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 16 May 2018 17:24:20 -0700 Subject: [PATCH] [quandary] only treat overrides of service methods as endpoints Reviewed By: mbouaziz Differential Revision: D8010176 fbshipit-source-id: 14a4b32 --- infer/src/absint/PatternMatch.ml | 4 +- infer/src/absint/PatternMatch.mli | 6 +-- infer/src/quandary/JavaTrace.ml | 7 ++- .../codetoanalyze/java/quandary/Services.java | 52 +++++++++++-------- .../codetoanalyze/java/quandary/issues.exp | 2 +- 5 files changed, 40 insertions(+), 31 deletions(-) diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index b48c4ecec..20ec47ed3 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/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 -> diff --git a/infer/src/absint/PatternMatch.mli b/infer/src/absint/PatternMatch.mli index 024887239..7f36d7182 100644 --- a/infer/src/absint/PatternMatch.mli +++ b/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 diff --git a/infer/src/quandary/JavaTrace.ml b/infer/src/quandary/JavaTrace.ml index e9bed38f3..cb0cb706b 100644 --- a/infer/src/quandary/JavaTrace.ml +++ b/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 *) diff --git a/infer/tests/codetoanalyze/java/quandary/Services.java b/infer/tests/codetoanalyze/java/quandary/Services.java index ae2de6916..d1f1184cc 100644 --- a/infer/tests/codetoanalyze/java/quandary/Services.java +++ b/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); } } diff --git a/infer/tests/codetoanalyze/java/quandary/issues.exp b/infer/tests/codetoanalyze/java/quandary/issues.exp index 9bff4fde8..1b04e29aa 100644 --- a/infer/tests/codetoanalyze/java/quandary/issues.exp +++ b/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.(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]