diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 5fb8d8608..f0602ed64 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -72,10 +72,21 @@ module Make (TaintSpecification : TaintSpec.S) = struct (* get the node associated with [exp] in [access_tree] *) - let hil_exp_get_node ?(abstracted= false) (exp: HilExp.t) access_tree proc_data = + let rec hil_exp_get_node ?(abstracted= false) (exp: HilExp.t) access_tree proc_data = match exp with | AccessPath access_path -> exp_get_node_ ~abstracted access_path access_tree proc_data + | Cast (_, e) | Exception e | UnaryOperator (_, e, _) -> + hil_exp_get_node ~abstracted e access_tree proc_data + | BinaryOperator (_, e1, e2) -> ( + match + ( hil_exp_get_node ~abstracted e1 access_tree proc_data + , hil_exp_get_node ~abstracted e2 access_tree proc_data ) + with + | Some node1, Some node2 -> + Some (TaintDomain.node_join node1 node2) + | node_opt, None | None, node_opt -> + node_opt ) | _ -> None @@ -267,39 +278,40 @@ module Make (TaintSpecification : TaintSpec.S) = struct (* add [sink] to the trace associated with the [formal_index]th actual *) let add_sink_to_actual sink_index access_tree_acc = match List.nth actuals sink_index with - | Some HilExp.AccessPath actual_ap_raw - -> ( - let actual_ap = AccessPath.Abs.Abstracted actual_ap_raw in - match access_path_get_node actual_ap access_tree_acc proc_data with - | Some (actual_trace, _) -> - let sink' = - let indexes = TraceDomain.get_footprint_indexes actual_trace in - TraceDomain.Sink.make ~indexes (TraceDomain.Sink.kind sink) callee_site - in - let actual_trace' = TraceDomain.add_sink sink' actual_trace in - report_trace actual_trace' callee_site proc_data ; - if TraceDomain.Sources.Footprint.is_empty - (TraceDomain.sources actual_trace').footprint - then + | Some exp -> ( + match hil_exp_get_node ~abstracted:true exp access_tree_acc proc_data with + | Some (actual_trace, _) + -> ( + let sink' = + let indexes = TraceDomain.get_footprint_indexes actual_trace in + TraceDomain.Sink.make ~indexes (TraceDomain.Sink.kind sink) callee_site + in + let actual_trace' = TraceDomain.add_sink sink' actual_trace in + report_trace actual_trace' callee_site proc_data ; + match exp with + | HilExp.AccessPath actual_ap_raw + when not + (TraceDomain.Sources.Footprint.is_empty + (TraceDomain.sources actual_trace').footprint) -> + let actual_ap = AccessPath.Abs.Abstracted actual_ap_raw in + TaintDomain.add_trace actual_ap actual_trace' access_tree_acc + | _ -> (* no more sources can flow into this sink; no sense in keeping track of it *) - access_tree_acc - else TaintDomain.add_trace actual_ap actual_trace' access_tree_acc - | _ -> - access_tree_acc ) + access_tree_acc ) + | _ -> + access_tree_acc ) | None -> L.internal_error "Taint is supposed to flow into sink %a at index %d, but the index is out of bounds" CallSite.pp callee_site sink_index ; access_tree_acc - | _ -> - access_tree_acc in IntSet.fold add_sink_to_actual (TraceDomain.Sink.indexes sink) access_tree let apply_summary ret_opt (actuals: HilExp.t list) summary caller_access_tree (proc_data: extras ProcData.t) callee_site = - let get_caller_ap formal_ap = + let get_caller_ap_node_opt formal_ap access_tree = let apply_return ret_ap = match ret_opt with | Some base_var -> @@ -310,11 +322,6 @@ module Make (TaintSpecification : TaintSpec.S) = struct AccessPath.Abs.pp ret_ap Typ.Procname.pp (CallSite.pname callee_site) ; None in - let get_actual_ap formal_index = - Option.value_map - ~f:(function HilExp.AccessPath access_path -> Some access_path | _ -> None) - ~default:None (List.nth actuals formal_index) - in let project ~formal_ap ~actual_ap = let projected_ap = AccessPath.append actual_ap (snd (AccessPath.Abs.extract formal_ap)) @@ -325,36 +332,38 @@ module Make (TaintSpecification : TaintSpec.S) = struct let base_var, _ = fst (AccessPath.Abs.extract formal_ap) in match base_var with | Var.ProgramVar pvar -> - if Pvar.is_return pvar then apply_return formal_ap else Some formal_ap + let projected_ap_opt = + if Pvar.is_return pvar then apply_return formal_ap else Some formal_ap + in + let caller_node_opt = + match projected_ap_opt with + | Some projected_ap -> + access_path_get_node projected_ap access_tree proc_data + | None -> + None + in + (projected_ap_opt, Option.value ~default:TaintDomain.empty_node caller_node_opt) | Var.LogicalVar id when Ident.is_footprint id -> ( - match - (* summaries store the index of the formal parameter in the ident stamp *) - get_actual_ap (Ident.get_stamp id) - with - | Some actual_ap -> + match List.nth actuals (Ident.get_stamp id) with + | Some HilExp.AccessPath actual_ap -> let projected_ap = project ~formal_ap ~actual_ap in - Some projected_ap - | None -> - None ) + let caller_node_opt = access_path_get_node projected_ap access_tree proc_data in + (Some projected_ap, Option.value ~default:TaintDomain.empty_node caller_node_opt) + | Some exp -> + let caller_node_opt = hil_exp_get_node exp access_tree proc_data in + (None, Option.value ~default:TaintDomain.empty_node caller_node_opt) + | _ -> + (None, TaintDomain.empty_node) ) | _ -> - None - in - let get_caller_ap_node_opt ap access_tree = - let get_caller_node caller_ap = - let caller_node_opt = access_path_get_node caller_ap access_tree proc_data in - let caller_node = Option.value ~default:TaintDomain.empty_node caller_node_opt in - (caller_ap, caller_node) - in - Option.map (get_caller_ap ap) ~f:get_caller_node + (None, TaintDomain.empty_node) in let replace_footprint_sources callee_trace caller_trace access_tree = let replace_footprint_source acc footprint_access_path (is_mem, _) = if is_mem then - match get_caller_ap_node_opt footprint_access_path access_tree with - | Some (_, (caller_ap_trace, _)) -> - TraceDomain.join caller_ap_trace acc - | None -> - acc + let _, (caller_ap_trace, _) = + get_caller_ap_node_opt footprint_access_path access_tree + in + TraceDomain.join caller_ap_trace acc else acc in let {TraceDomain.Sources.footprint} = TraceDomain.sources callee_trace in @@ -368,18 +377,20 @@ module Make (TaintSpecification : TaintSpec.S) = struct appended_trace in let add_to_caller_tree access_tree_acc callee_ap callee_trace = - match get_caller_ap_node_opt callee_ap access_tree_acc with - | Some (caller_ap, (caller_trace, caller_tree)) -> - let trace = instantiate_and_report callee_trace caller_trace access_tree_acc in - let pruned_trace = - if TraceDomain.Sources.Footprint.is_empty (TraceDomain.sources trace).footprint then - (* empty footprint; nothing else can flow into these sinks. so don't track them *) - TraceDomain.update_sinks trace TraceDomain.Sinks.empty - else trace - in + let caller_ap_opt, (caller_trace, caller_tree) = + get_caller_ap_node_opt callee_ap access_tree_acc + in + let trace = instantiate_and_report callee_trace caller_trace access_tree_acc in + let pruned_trace = + if TraceDomain.Sources.Footprint.is_empty (TraceDomain.sources trace).footprint then + (* empty footprint; nothing else can flow into these sinks. so don't track them *) + TraceDomain.update_sinks trace TraceDomain.Sinks.empty + else trace + in + match caller_ap_opt with + | Some caller_ap -> TaintDomain.add_node caller_ap (pruned_trace, caller_tree) access_tree_acc | None -> - ignore (instantiate_and_report callee_trace TraceDomain.empty access_tree_acc) ; access_tree_acc in TaintDomain.trace_fold add_to_caller_tree summary caller_access_tree diff --git a/infer/tests/codetoanalyze/cpp/quandary/expressions.cpp b/infer/tests/codetoanalyze/cpp/quandary/expressions.cpp new file mode 100644 index 000000000..cbec54efb --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/quandary/expressions.cpp @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2017 - 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. + */ + +extern int __infer_taint_source(); +extern void __infer_taint_sink(int); + +namespace expressions { + +void propagate_via_unop1_bad() { + int source = __infer_taint_source(); + int laundered = ~source; + __infer_taint_sink(laundered); +} + +void propagate_via_unop2_bad() { + int source = __infer_taint_source(); + __infer_taint_sink(~source); +} + +void propagate_via_binop1_bad() { + int source = __infer_taint_source(); + int laundered = 5 + source % 7; + __infer_taint_sink(laundered); +} + +void propagate_via_binop2_bad() { + int source1 = __infer_taint_source(); + int source2 = __infer_taint_source(); + int laundered = 1 + source1 / 2 + source2 * 7; + // should report twice + __infer_taint_sink(laundered); +} + +void propagate_via_binop3_bad() { + int source = __infer_taint_source(); + __infer_taint_sink(source - 3); +} + +void call_sink_nested(int formal) { __infer_taint_sink(formal); } + +void propagate_via_binop_nested1_bad() { + int source = ~(__infer_taint_source()); + call_sink_nested(source); +} + +void propagate_via_binop_nested2_bad() { + int source = __infer_taint_source(); + call_sink_nested(~source); +} + +} // namespace expressions diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index 8c98ce16b..b2792d8c5 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -57,6 +57,14 @@ codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 35, SHELL_INJECTION, [ codetoanalyze/cpp/quandary/execs.cpp, execs::exec_flag_bad, 0, SHELL_INJECTION, [Return from __global_access,Call to execl] codetoanalyze/cpp/quandary/execs.cpp, execs::exec_flag_interproc_bad, 2, SHELL_INJECTION, [Return from __global_access with tainted data &return,Return from execs::return_global,Call to execl] codetoanalyze/cpp/quandary/execs.cpp, execs::sql_on_env_var_bad, 2, SQL_INJECTION, [Return from getenv,Call to __infer_sql_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop1_bad, 3, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop2_bad, 5, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop2_bad, 5, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop3_bad, 2, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop_nested1_bad, 2, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to expressions::call_sink_nested,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_binop_nested2_bad, 2, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to expressions::call_sink_nested,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_unop1_bad, 3, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink] +codetoanalyze/cpp/quandary/expressions.cpp, expressions::propagate_via_unop2_bad, 2, QUANDARY_TAINT_ERROR, [Return from __infer_taint_source,Call to __infer_taint_sink] codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad1, 5, SHELL_INJECTION, [Return from std::basic_istream>_read,Call to execle] codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad2, 5, SHELL_INJECTION, [Return from std::basic_istream>_readsome,Call to execle] codetoanalyze/cpp/quandary/files.cpp, files::read_file_call_exec_bad3, 5, SHELL_INJECTION, [Return from std::basic_istream>_getline,Call to execle] diff --git a/infer/tests/codetoanalyze/cpp/quandary/unknown_code.cpp b/infer/tests/codetoanalyze/cpp/quandary/unknown_code.cpp index dbc1ff964..a3f6f14ec 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/unknown_code.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/unknown_code.cpp @@ -7,6 +7,8 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +#include +#include #include extern std::string __infer_taint_source(); @@ -15,6 +17,8 @@ extern std::string skip_value(std::string); extern std::string* skip_pointer(std::string); extern void skip_by_ref(std::string, std::string&); +extern int of_string(std::string); + namespace unknown_code { void direct_bad() { @@ -54,4 +58,5 @@ void FN_via_skip_by_ref_bad() { skip_by_ref(source, laundered_by_ref); __infer_taint_sink(laundered_by_ref); } + }