From 0b9727214d43c9463a3f6cf2c0500e47687e0914 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 3 Nov 2016 09:44:33 -0700 Subject: [PATCH] [quandary] support `StringBuilder`'s and other methods for propagating `String` taint Summary: Our default strategy for handling unknown code is to propagate taint from the actuals to the return value. But for commonly-used methods like `StringBuilder.append` (used every time you do `+` with a string in Java), this doesn't work. The taint should be propagated to both the receiver and the return value in these cases. I'm considering a solution where we always propagate taint to the receiver of unknown functions in the future, but I am concerned about the performance. So let's stick with a few special string cases for now. Reviewed By: cristianoc Differential Revision: D4124355 fbshipit-source-id: 5b2a232 --- infer/src/quandary/JavaTaintAnalysis.ml | 14 ++-- infer/src/quandary/TaintAnalysis.ml | 5 +- .../codetoanalyze/java/quandary/Strings.java | 66 +++++++++++++++++++ .../codetoanalyze/java/quandary/issues.exp | 7 ++ 4 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/quandary/Strings.java diff --git a/infer/src/quandary/JavaTaintAnalysis.ml b/infer/src/quandary/JavaTaintAnalysis.ml index 856035793..6becaad14 100644 --- a/infer/src/quandary/JavaTaintAnalysis.ml +++ b/infer/src/quandary/JavaTaintAnalysis.ml @@ -44,9 +44,10 @@ include let footprint_trace = Trace.of_source footprint_source in QuandarySummary.make_in_out_summary input output (to_summary_trace footprint_trace) - (* propagate the trace associated with each actual to the actual corresponding to the - constructed object *) - let propagate_to_constructor site actuals ~propagate_all = + (* propagate the trace associated with non-receiver actual to the receiver actual. also useful + for propagating taint from constructor actuals to the constructed object (which, like the + receiver, is also the first argument) *) + let propagate_to_receiver site actuals ~propagate_all = match actuals with | [] -> failwithf @@ -83,7 +84,12 @@ include Procname.java_get_method java_pname, ret_typ_opt with | _ when Procname.is_constructor pname -> - propagate_to_constructor site actuals ~propagate_all:true + propagate_to_receiver site actuals ~propagate_all:true + | ("java.lang.StringBuffer" | "java.lang.StringBuilder" | "java.util.Formatter"), _, + Some ret_typ + when not (Procname.java_is_static pname) -> + (propagate_actuals_to_return site ret_typ actuals ~propagate_all:true) @ + (propagate_to_receiver site actuals ~propagate_all:true) | _, _, Some ret_typ -> propagate_actuals_to_return site ret_typ actuals ~propagate_all:true | _ -> diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index c710d14e6..a9ab9059e 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -263,7 +263,10 @@ module Make (TaintSpec : TaintSpec.S) = struct let caller_ap_trace_opt = match in_out_summary.output with | Out_return ret_ap -> - Some (apply_return ret_ap ret_id, TraceDomain.initial) + let caller_ret_ap = apply_return ret_ap ret_id in + let ret_trace = + access_path_get_trace caller_ret_ap access_tree proc_data callee_loc in + Some (caller_ret_ap, ret_trace) | Out_formal (formal_num, formal_ap) -> get_actual_ap_trace formal_num formal_ap access_tree | Out_global global_ap -> diff --git a/infer/tests/codetoanalyze/java/quandary/Strings.java b/infer/tests/codetoanalyze/java/quandary/Strings.java new file mode 100644 index 000000000..6a2e415d5 --- /dev/null +++ b/infer/tests/codetoanalyze/java/quandary/Strings.java @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2015 - 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.util.Formatter; + +import com.facebook.infer.builtins.InferTaint; + +/** a lot of tainted values are strings, so propagation through StringBuilder's and the like is very + * important. */ + +public class Strings { + + void viaStringBuilderSugarBad() { + Object source = InferTaint.inferSecretSource(); + InferTaint.inferSensitiveSink(source + ""); + } + + void viaStringBuilderBad() { + Object source = InferTaint.inferSecretSource(); + StringBuilder builder = new StringBuilder(); + InferTaint.inferSensitiveSink(builder.append(source).append("").toString()); + } + + void viaStringBuilderIgnoreReturnBad() { + Object source = InferTaint.inferSecretSource(); + StringBuilder builder = new StringBuilder(); + // builder should be tainted after this call even though we ignore the return value + builder.append(source); + InferTaint.inferSensitiveSink(builder.toString()); + } + + void viaStringBufferBad() { + Object source = InferTaint.inferSecretSource(); + StringBuffer buffer = new StringBuffer(); + InferTaint.inferSensitiveSink(buffer.append("").append(source).toString()); + } + + void viaStringBufferIgnoreReturnBad() { + Object source = InferTaint.inferSecretSource(); + StringBuffer buffer = new StringBuffer(); + buffer.append(source); + InferTaint.inferSensitiveSink(buffer.toString()); + } + + void viaFormatterBad() { + Object source = InferTaint.inferSecretSource(); + Formatter formatter = new Formatter(); + InferTaint.inferSensitiveSink(formatter.format("%s", source).toString()); + } + + void viaFormatterIgnoreReturnBad() { + Object source = InferTaint.inferSecretSource(); + Formatter formatter = new Formatter(); + formatter.format("%s", source); + InferTaint.inferSensitiveSink(formatter.toString()); + } + +} diff --git a/infer/tests/codetoanalyze/java/quandary/issues.exp b/infer/tests/codetoanalyze/java/quandary/issues.exp index 5fc628baf..09e4dd441 100644 --- a/infer/tests/codetoanalyze/java/quandary/issues.exp +++ b/infer/tests/codetoanalyze/java/quandary/issues.exp @@ -146,5 +146,12 @@ LoggingPrivateData.java:60: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(float Recursion.java:26: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 26]) -> Other(void Recursion.callSinkThenDiverge(Object) at [line 26]) Recursion.java:36: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 36]) -> Other(void Recursion.safeRecursionCallSink(int,Object) at [line 36]) Recursion.java:42: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 42]) -> Other(void Recursion.recursionBad(int,Object) at [line 42]) +Strings.java:23: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 22]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 23]) (via { StringBuilder StringBuilder.append(Object) at [line 23], StringBuilder StringBuilder.append(String) at [line 23], String StringBuilder.toString() at [line 23] }) +Strings.java:29: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 27]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 29]) (via { StringBuilder StringBuilder.append(Object) at [line 29], StringBuilder StringBuilder.append(String) at [line 29], String StringBuilder.toString() at [line 29] }) +Strings.java:37: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 33]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 37]) (via { StringBuilder StringBuilder.append(Object) at [line 36], String StringBuilder.toString() at [line 37] }) +Strings.java:43: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 41]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 43]) (via { StringBuffer StringBuffer.append(Object) at [line 43], StringBuffer StringBuffer.append(String) at [line 43], String StringBuffer.toString() at [line 43] }) +Strings.java:50: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 47]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 50]) (via { StringBuffer StringBuffer.append(Object) at [line 49], String StringBuffer.toString() at [line 50] }) +Strings.java:56: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 54]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 56]) (via { Formatter Formatter.format(String,java.lang.Object[]) at [line 56], String Formatter.toString() at [line 56] }) +Strings.java:63: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 60]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 63]) (via { Formatter Formatter.format(String,java.lang.Object[]) at [line 62], String Formatter.toString() at [line 63] }) UnknownCode.java:25: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 23]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 25]) (via { Object UnknownCode.id(Object) at [line 24] }) UnknownCode.java:32: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 29]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 32]) (via { String.(String) at [line 31] })