From ab77cfe80380c323eb997aa3d983f67ea0c000f6 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 31 Jan 2018 14:40:08 -0800 Subject: [PATCH] [quandary] thrift services as sources in Java Reviewed By: jeremydubreil Differential Revision: D6813966 fbshipit-source-id: 8cddca6 --- infer/src/checkers/annotations.ml | 4 ++ infer/src/checkers/annotations.mli | 2 + infer/src/quandary/JavaTrace.ml | 42 ++++++++++++--- .../codetoanalyze/java/quandary/Services.java | 51 +++++++++++++++++++ .../codetoanalyze/java/quandary/issues.exp | 2 + 5 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/quandary/Services.java diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 2691eb150..620914396 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -93,6 +93,8 @@ let thread_confined = "ThreadConfined" let thread_safe = "ThreadSafe" +let thrift_service = "ThriftService" + let true_on_null = "TrueOnNull" let ui_thread = "UiThread" @@ -178,6 +180,8 @@ let ia_is_synchronized_collection ia = ia_ends_with ia synchronized_collection let ia_is_thread_safe ia = ia_ends_with ia thread_safe +let ia_is_thrift_service ia = ia_ends_with ia thrift_service + let ia_is_true_on_null ia = ia_ends_with ia true_on_null let ia_is_initializer ia = ia_ends_with ia initializer_ diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 3370985bb..5bca4fb7e 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -110,6 +110,8 @@ val ia_is_thread_safe : Annot.Item.t -> bool val ia_is_thread_confined : Annot.Item.t -> bool +val ia_is_thrift_service : Annot.Item.t -> bool + val ia_is_ui_thread : Annot.Item.t -> bool val ia_is_volatile : Annot.Item.t -> bool diff --git a/infer/src/quandary/JavaTrace.ml b/infer/src/quandary/JavaTrace.ml index 746c1310e..4925ca964 100644 --- a/infer/src/quandary/JavaTrace.ml +++ b/infer/src/quandary/JavaTrace.ml @@ -14,6 +14,7 @@ module L = Logging module SourceKind = struct type t = | DrawableResource of Pvar.t (** Drawable resource ID read from a global *) + | Endpoint of (Mangled.t * Typ.desc) (** source originating from formal of an endpoint *) | Intent (** external Intent or a value read from one *) | IntentFromURI (** Intent created from a URI *) | Other (** for testing or uncategorized sources *) @@ -25,6 +26,8 @@ module SourceKind = struct let matches ~caller ~callee = Int.equal 0 (compare caller callee) let of_string = function + | "Endpoint" -> + Endpoint (Mangled.from_string "NONE", Typ.Tvoid) | "Intent" -> Intent | "IntentFromURI" -> @@ -150,6 +153,20 @@ module SourceKind = struct in List.map ~f:taint_formal_with_types formals in + (* taint all formals except for [this] *) + let taint_all_but_this ~make_source = + List.map + ~f:(fun (name, typ) -> + let taint = + match Mangled.to_string name with + | "this" -> + None + | _ -> + Some (make_source name typ.Typ.desc) + in + (name, typ, taint) ) + (Procdesc.get_formals pdesc) + in let formals = Procdesc.get_formals pdesc in match Procdesc.get_proc_name pdesc with | Typ.Procname.Java java_pname -> ( @@ -199,7 +216,15 @@ module SourceKind = struct , ("onJsAlert" | "onJsBeforeUnload" | "onJsConfirm" | "onJsPrompt") ) -> Some (taint_formals_with_types ["java.lang.String"] UserControlledURI formals) | _ -> - None + match Tenv.lookup tenv typename with + | Some typ -> + if Annotations.struct_typ_has_annot typ Annotations.ia_is_thrift_service then + (* assume every non-this formal of a Thrift service is tainted *) + (* TODO: may not want to taint numbers or Enum's *) + Some (taint_all_but_this ~make_source:(fun name desc -> Endpoint (name, desc))) + else None + | _ -> + None in match PatternMatch.supertype_find_map_opt tenv taint_matching_supertype @@ -219,6 +244,8 @@ module SourceKind = struct ( match kind with | DrawableResource pvar -> Pvar.to_string pvar + | Endpoint (formal_name, _) -> + F.asprintf "Endpoint[%s]" (Mangled.to_string formal_name) | Intent -> "Intent" | IntentFromURI -> @@ -457,19 +484,20 @@ include Trace.Make (struct | _ when not (List.is_empty sanitizers) -> (* assume any sanitizer clears all forms of taint *) None - | (Intent | UserControlledString | UserControlledURI), CreateIntent -> + | (Endpoint _ | Intent | UserControlledString | UserControlledURI), CreateIntent -> (* creating Intent from user-congrolled data *) Some IssueType.untrusted_intent_creation - | (Intent | IntentFromURI | UserControlledString | UserControlledURI), CreateFile -> + | (Endpoint _ | Intent | IntentFromURI | UserControlledString | UserControlledURI), CreateFile -> (* user-controlled file creation; may be vulnerable to path traversal + more *) Some IssueType.untrusted_file - | (Intent | IntentFromURI | UserControlledString | UserControlledURI), Deserialization -> + | ( (Endpoint _ | Intent | IntentFromURI | UserControlledString | UserControlledURI) + , Deserialization ) -> (* shouldn't let anyone external control what we deserialize *) Some IssueType.untrusted_deserialization - | (Intent | IntentFromURI | UserControlledString | UserControlledURI), HTML -> + | (Endpoint _ | Intent | IntentFromURI | UserControlledString | UserControlledURI), HTML -> (* untrusted data flows into HTML; XSS risk *) Some IssueType.cross_site_scripting - | (Intent | IntentFromURI | UserControlledString | UserControlledURI), JavaScript -> + | (Endpoint _ | Intent | IntentFromURI | UserControlledString | UserControlledURI), JavaScript -> (* untrusted data flows into JS *) Some IssueType.javascript_injection | DrawableResource _, OpenDrawableResource -> @@ -481,7 +509,7 @@ include Trace.Make (struct Some IssueType.create_intent_from_uri | PrivateData, Logging -> Some IssueType.logging_private_data - | (Intent | UserControlledString | UserControlledURI), ShellExec -> + | (Endpoint _ | Intent | UserControlledString | UserControlledURI), ShellExec -> Some IssueType.shell_injection | Other, _ | _, Other -> (* for testing purposes, Other matches everything *) diff --git a/infer/tests/codetoanalyze/java/quandary/Services.java b/infer/tests/codetoanalyze/java/quandary/Services.java new file mode 100644 index 000000000..3b2b31697 --- /dev/null +++ b/infer/tests/codetoanalyze/java/quandary/Services.java @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2018 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package codetoanalyze.java.quandary; + +import java.io.IOException; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.Runtime; + +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 { + + public void serviceMethodBad(String s) throws IOException { + Runtime.getRuntime().exec(s); // RCE if s is tainted, we should warn + } + +} + +@ThriftService +interface ThriftInterface { + + public void interfaceServiceMethodBad(String s) throws IOException; +} + +// this is a service too +class Implementer implements ThriftInterface { + + public void interfaceServiceMethodBad(String s) throws IOException { + Runtime.getRuntime().exec(s); // RCE if s is tainted, we should warn + } + +} diff --git a/infer/tests/codetoanalyze/java/quandary/issues.exp b/infer/tests/codetoanalyze/java/quandary/issues.exp index 4bd81d090..01482fea7 100644 --- a/infer/tests/codetoanalyze/java/quandary/issues.exp +++ b/infer/tests/codetoanalyze/java/quandary/issues.exp @@ -174,6 +174,8 @@ codetoanalyze/java/quandary/LoggingPrivateData.java, void LoggingPrivateData.log codetoanalyze/java/quandary/Recursion.java, void Recursion.callSinkThenDivergeBad(), 1, QUANDARY_TAINT_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, [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, [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, [Return from void Implementer.interfaceServiceMethodBad(String),Call to Process Runtime.exec(String) with tainted index 1] +codetoanalyze/java/quandary/Services.java, void Service1.serviceMethodBad(String), 1, SHELL_INJECTION, [Return from void Service1.serviceMethodBad(String),Call to Process Runtime.exec(String) with tainted index 1] codetoanalyze/java/quandary/Strings.java, void Strings.viaFormatterBad(), 3, QUANDARY_TAINT_ERROR, [Return from Object InferTaint.inferSecretSource(),Call to void InferTaint.inferSensitiveSink(Object) with tainted index 0] codetoanalyze/java/quandary/Strings.java, void Strings.viaFormatterIgnoreReturnBad(), 4, QUANDARY_TAINT_ERROR, [Return from Object InferTaint.inferSecretSource(),Call to void InferTaint.inferSensitiveSink(Object) with tainted index 0] codetoanalyze/java/quandary/Strings.java, void Strings.viaStringBufferBad(), 3, QUANDARY_TAINT_ERROR, [Return from Object InferTaint.inferSecretSource(),Call to void InferTaint.inferSensitiveSink(Object) with tainted index 0]