From 9dc7e3d66ffee6d81e0439f29f39c743493af7c4 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Mon, 8 May 2017 11:42:57 -0700 Subject: [PATCH] [quandary] handle return value passed by reference in sources Summary: Needed because this is how the Clang frontend translates returns of non-POD, non pointer values (I think)? Will handle the more general case of pass by reference soon. Reviewed By: jvillard Differential Revision: D5017653 fbshipit-source-id: 1fbcea5 --- infer/src/quandary/ClangTaintAnalysis.ml | 10 +++++-- infer/src/quandary/TaintAnalysis.ml | 28 ++++++++++++++----- .../codetoanalyze/cpp/quandary/.inferconfig | 9 ++++++ .../codetoanalyze/cpp/quandary/basics.cpp | 9 ++++++ .../codetoanalyze/cpp/quandary/issues.exp | 1 + 5 files changed, 48 insertions(+), 9 deletions(-) diff --git a/infer/src/quandary/ClangTaintAnalysis.ml b/infer/src/quandary/ClangTaintAnalysis.ml index 34a4d9d9b..33cb4b848 100644 --- a/infer/src/quandary/ClangTaintAnalysis.ml +++ b/infer/src/quandary/ClangTaintAnalysis.ml @@ -23,8 +23,14 @@ include | QuandarySummary.AccessTree.Clang tree -> tree | _ -> assert false - let handle_unknown_call _ _ _ _ = - [] + let handle_unknown_call _ ret_typ_opt actuals _ = match ret_typ_opt, actuals with + | Some _, _ -> + (* propagate taint from actuals to return value *) + [TaintSpec.Propagate_to_return] + | None, [] -> [] + | None, _ -> + (* no return value; propagate taint from actuals to receiver *) + [TaintSpec.Propagate_to_receiver] let is_taintable_type _ = true end) diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 233423fa7..913706036 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -355,13 +355,27 @@ module Make (TaintSpecification : TaintSpec.S) = struct let source = TraceDomain.Source.get call_site proc_data.tenv in let astate_with_source = match source, ret_opt with - | Some source, Some ret_exp -> - add_source source ret_exp astate_with_sink - | Some _, None -> - L.err - "Warning: %a is marked as a source, but has no return value" - Typ.Procname.pp callee_pname; - astate_with_sink + | Some source, Some ret_base -> + add_source source ret_base astate_with_sink + | Some source, None -> + let warn_invalid_source () = + L.stderr + "Warning: %a is marked as a source, but has no return value" + Typ.Procname.pp callee_pname; + astate_with_sink in + if Typ.Procname.is_c_method called_pname + then + (* the C++ frontend handles returns of non-pointers by adding a dummy + pass-by-reference variable as the last actual, then returning the value + by assigning to it. make sure we understand this pattern *) + match List.last actuals with + | Some (HilExp.AccessPath ((Var.ProgramVar pvar, _) as ret_base, [])) + when Pvar.is_frontend_tmp pvar -> + add_source source ret_base astate_with_sink + | _ -> + warn_invalid_source () + else + warn_invalid_source () | None, _ -> astate_with_sink in diff --git a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig index 8569ad047..895c8bfdb 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/.inferconfig +++ b/infer/tests/codetoanalyze/cpp/quandary/.inferconfig @@ -15,6 +15,10 @@ { "procedure": "basics::template_source", "kind": "Other" + }, + { + "procedure": "basics::Obj::string_source", + "kind": "Other" } ], "quandary-sinks": [ @@ -32,6 +36,11 @@ "procedure": "basics::Obj::static_sink", "kind": "Other", "index": "0" + }, + { + "procedure": "basics::Obj::string_sink", + "kind": "Other", + "index": "1" } ] } diff --git a/infer/tests/codetoanalyze/cpp/quandary/basics.cpp b/infer/tests/codetoanalyze/cpp/quandary/basics.cpp index 5fae91906..af46c37a2 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/basics.cpp +++ b/infer/tests/codetoanalyze/cpp/quandary/basics.cpp @@ -7,6 +7,8 @@ * of patent rights can be found in the PATENTS file in the same directory. */ +#include + extern void* __infer_taint_source(); extern void __infer_taint_sink(void*); @@ -18,6 +20,8 @@ class Obj { void method_sink(void*) {} static void* static_source() { return (void*)0; } static void static_sink(void*) {} + std::string string_source(int i) { return ""; } + void string_sink(std::string) {} }; void* returnSource() { return __infer_taint_source(); } @@ -68,4 +72,9 @@ void template_source_bad() { void* source = template_source(); __infer_taint_sink(source); } + +void string_source_bad(Obj obj) { + std::string source = obj.string_source(5); + obj.string_sink(source); +} } diff --git a/infer/tests/codetoanalyze/cpp/quandary/issues.exp b/infer/tests/codetoanalyze/cpp/quandary/issues.exp index fdf7f43f8..ef72ccee6 100644 --- a/infer/tests/codetoanalyze/cpp/quandary/issues.exp +++ b/infer/tests/codetoanalyze/cpp/quandary/issues.exp @@ -4,6 +4,7 @@ codetoanalyze/cpp/quandary/basics.cpp, basics::returnSourceToSinkBad, 2, QUANDAR codetoanalyze/cpp/quandary/basics.cpp, basics::sourceThenCallSinkBad, 2, QUANDARY_TAINT_ERROR, [return from __infer_taint_source,call to basics::callSink,call to __infer_taint_sink] codetoanalyze/cpp/quandary/basics.cpp, basics::sourceToSinkDirectBad, 2, QUANDARY_TAINT_ERROR, [return from __infer_taint_source,call to __infer_taint_sink] codetoanalyze/cpp/quandary/basics.cpp, basics::static_source_sink_bad, 2, QUANDARY_TAINT_ERROR, [return from basics::Obj_static_source,call to basics::Obj_static_sink] +codetoanalyze/cpp/quandary/basics.cpp, basics::string_source_bad, 2, QUANDARY_TAINT_ERROR, [return from basics::Obj_string_source,call to basics::Obj_string_sink] codetoanalyze/cpp/quandary/basics.cpp, basics::template_source_bad, 2, QUANDARY_TAINT_ERROR, [return from basics::template_source,call to __infer_taint_sink] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 6, QUANDARY_TAINT_ERROR, [return from getenv,call to execl] codetoanalyze/cpp/quandary/execs.cpp, execs::callExecBad, 8, QUANDARY_TAINT_ERROR, [return from getenv,call to execl]