From 2d29b47855f7e704bd3ff2358692e9ef183aed63 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 24 Oct 2017 17:36:41 -0700 Subject: [PATCH] [traces] allow reported traces to return an issue type Summary: A stepping stone to have descriptive issue types for each kind of flow rather that lumping everything into `QUANDARY_TAINT_ERROR`. Reviewed By: mbouaziz Differential Revision: D6126690 fbshipit-source-id: a7230c0 --- infer/src/base/IssueType.ml | 2 ++ infer/src/base/IssueType.mli | 3 +++ infer/src/checkers/SinkTrace.ml | 2 +- infer/src/checkers/Trace.ml | 35 ++++++++++++++++++++++------- infer/src/checkers/Trace.mli | 12 +++++++--- infer/src/quandary/ClangTrace.ml | 16 ++++++------- infer/src/quandary/JavaTrace.ml | 12 +++++----- infer/src/quandary/TaintAnalysis.ml | 9 ++++---- infer/src/unit/TaintTests.ml | 2 +- infer/src/unit/TraceTests.ml | 12 ++++++---- 10 files changed, 70 insertions(+), 35 deletions(-) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 9e816b765..34cd62833 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -148,6 +148,8 @@ let divide_by_zero = from_string ~enabled:false "DIVIDE_BY_ZERO" let double_lock = from_string "DOUBLE_LOCK" +let do_not_report = from_string "DO_NOT_REPORT" + let empty_vector_access = from_string "EMPTY_VECTOR_ACCESS" let eradicate_condition_redundant = diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index f8d1cc777..87acb5c0a 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -99,6 +99,9 @@ val divide_by_zero : t val double_lock : t +val do_not_report : t +(** an issue type that should never be reported *) + val empty_vector_access : t val eradicate_condition_redundant : t diff --git a/infer/src/checkers/SinkTrace.ml b/infer/src/checkers/SinkTrace.ml index 86634c51d..a3cd535ab 100644 --- a/infer/src/checkers/SinkTrace.ml +++ b/infer/src/checkers/SinkTrace.ml @@ -42,7 +42,7 @@ module Make (TraceElem : TraceElem.S) = struct module Source = Source.Dummy module Sink = MakeSink (TraceElem) - let should_report _ _ = true + let get_report _ _ = Some IssueType.do_not_report end) type sink_path = Passthroughs.t * (Sink.t * Passthroughs.t) list diff --git a/infer/src/checkers/Trace.ml b/infer/src/checkers/Trace.ml index 65e9a016c..c8b175d0f 100644 --- a/infer/src/checkers/Trace.ml +++ b/infer/src/checkers/Trace.ml @@ -16,8 +16,7 @@ module type Spec = sig module Sink : Sink.S - val should_report : Source.t -> Sink.t -> bool - (** should a flow originating at source and entering sink be reported? *) + val get_report : Source.t -> Sink.t -> IssueType.t option end module type S = sig @@ -63,6 +62,12 @@ module type S = sig type path = Passthroughs.t * (path_source * Passthroughs.t) list * (path_sink * Passthroughs.t) list + type report = + { issue: IssueType.t + ; path_source: path_source + ; path_sink: path_sink + ; path_passthroughs: Passthroughs.t } + val empty : t val sources : t -> Sources.t @@ -74,7 +79,7 @@ module type S = sig val passthroughs : t -> Passthroughs.t (** get the passthroughs of the trace *) - val get_reports : ?cur_site:CallSite.t -> t -> (path_source * path_sink * Passthroughs.t) list + val get_reports : ?cur_site:CallSite.t -> t -> report list (** get the reportable source-sink flows in this trace. specifying [cur_site] restricts the reported paths to ones introduced by the call at [cur_site] *) @@ -250,6 +255,12 @@ module Make (Spec : Spec) = struct type path = Passthroughs.t * (path_source * Passthroughs.t) list * (path_sink * Passthroughs.t) list + type report = + { issue: IssueType.t + ; path_source: path_source + ; path_sink: path_sink + ; path_passthroughs: Passthroughs.t } + let pp fmt {sources; sinks; passthroughs} = let pp_passthroughs fmt passthroughs = if not (Passthroughs.is_empty passthroughs) then @@ -297,8 +308,16 @@ module Make (Spec : Spec) = struct (* written to avoid closure allocations in hot code. change with caution. *) let report_source source sinks acc0 = let report_one sink acc = - if Spec.should_report source sink && should_report_at_site source sink then - (Known source, sink, t.passthroughs) :: acc + if should_report_at_site source sink then + match Spec.get_report source sink with + | Some issue -> + { issue + ; path_source= Known source + ; path_sink= sink + ; path_passthroughs= t.passthroughs } + :: acc + | None -> + acc else acc in Sinks.fold report_one sinks acc0 @@ -384,8 +403,8 @@ module Make (Spec : Spec) = struct ([], []) in List.map - ~f:(fun (path_source, sink, passthroughs) -> - let sources_passthroughs, sinks_passthroughs = expand_path path_source sink in + ~f:(fun {path_source; path_sink; path_passthroughs} -> + let sources_passthroughs, sinks_passthroughs = expand_path path_source path_sink in let filtered_passthroughs = let source_site = match path_source with @@ -394,7 +413,7 @@ module Make (Spec : Spec) = struct | Footprint _ -> Option.value ~default:CallSite.dummy cur_site in - filter_passthroughs_ Top_level source_site (Sink.call_site sink) passthroughs + filter_passthroughs_ Top_level source_site (Sink.call_site path_sink) path_passthroughs in (filtered_passthroughs, sources_passthroughs, sinks_passthroughs)) (get_reports ?cur_site t) diff --git a/infer/src/checkers/Trace.mli b/infer/src/checkers/Trace.mli index 5d7b8354f..1fc3439d4 100644 --- a/infer/src/checkers/Trace.mli +++ b/infer/src/checkers/Trace.mli @@ -16,8 +16,8 @@ module type Spec = sig module Sink : Sink.S - val should_report : Source.t -> Sink.t -> bool - (** should a flow originating at source and entering sink be reported? *) + val get_report : Source.t -> Sink.t -> IssueType.t option + (** return Some(issue) if the source and sink match, None otherwise *) end module type S = sig @@ -71,6 +71,12 @@ module type S = sig type path = Passthroughs.t * (path_source * Passthroughs.t) list * (path_sink * Passthroughs.t) list + type report = + { issue: IssueType.t + ; path_source: path_source + ; path_sink: path_sink + ; path_passthroughs: Passthroughs.t } + val empty : t (** the empty trace *) @@ -83,7 +89,7 @@ module type S = sig val passthroughs : t -> Passthroughs.t (** get the passthroughs of the trace *) - val get_reports : ?cur_site:CallSite.t -> t -> (path_source * path_sink * Passthroughs.t) list + val get_reports : ?cur_site:CallSite.t -> t -> report list (** get the reportable source-sink flows in this trace. specifying [cur_site] restricts the reported paths to ones introduced by the call at [cur_site] *) diff --git a/infer/src/quandary/ClangTrace.ml b/infer/src/quandary/ClangTrace.ml index ecd4a2c93..4a85ac505 100644 --- a/infer/src/quandary/ClangTrace.ml +++ b/infer/src/quandary/ClangTrace.ml @@ -277,7 +277,7 @@ include Trace.Make (struct module Source = CppSource module Sink = CppSink - let should_report source sink = + let get_report source sink = (* using this to match custom string wrappers such as folly::StringPiece *) let is_stringy typ = let lowercase_typ = String.lowercase (Typ.to_string (Typ.mk typ)) in @@ -287,23 +287,23 @@ include Trace.Make (struct match (Source.kind source, Sink.kind sink) with | Endpoint _, BufferAccess -> (* untrusted data from an endpoint flowing into a buffer *) - true + Some IssueType.quandary_taint_error | Endpoint (_, typ), (ShellExec | SQL) -> (* untrusted string data flowing to shell exec/SQL *) - is_stringy typ + Option.some_if (is_stringy typ) IssueType.quandary_taint_error | (EnvironmentVariable | File), (BufferAccess | ShellExec | SQL) -> (* untrusted environment var or file data flowing to buffer or code injection *) - true + Some IssueType.quandary_taint_error | (Endpoint _ | EnvironmentVariable | File), Allocation -> (* untrusted data flowing to memory allocation *) - true + Some IssueType.quandary_taint_error | CommandLineFlag _, (Allocation | BufferAccess | Other | ShellExec | SQL) -> (* data controlled by a command line flag flowing somewhere sensitive *) - true + Some IssueType.quandary_taint_error | Other, _ -> (* Other matches everything *) - true + Some IssueType.quandary_taint_error | _, Other -> - true + Some IssueType.quandary_taint_error end) diff --git a/infer/src/quandary/JavaTrace.ml b/infer/src/quandary/JavaTrace.ml index d8af56847..037cf9887 100644 --- a/infer/src/quandary/JavaTrace.ml +++ b/infer/src/quandary/JavaTrace.ml @@ -353,7 +353,7 @@ include Trace.Make (struct module Source = JavaSource module Sink = JavaSink - let should_report source sink = + let get_report source sink = match (Source.kind source, Sink.kind sink) with | PrivateData, Logging (* logging private data issue *) @@ -371,17 +371,17 @@ include Trace.Make (struct (* create file from user-controller URI; potential path-traversal vulnerability *) | UserControlledString, (StartComponent | CreateIntent | JavaScript | CreateFile | HTML) -> (* do something sensitive with a user-controlled string *) - true + Some IssueType.quandary_taint_error | (Intent | UserControlledURI | UserControlledString), Deserialization -> (* shouldn't let anyone external control what we deserialize *) - true + Some IssueType.quandary_taint_error | DrawableResource _, OpenDrawableResource -> (* not a security issue, but useful for debugging flows from resource IDs to inflation *) - true + Some IssueType.quandary_taint_error | Other, _ | _, Other -> (* for testing purposes, Other matches everything *) - true + Some IssueType.quandary_taint_error | _ -> - false + None end) diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index dd6d4f691..924e1ca19 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -136,7 +136,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct final_sink (if is_endpoint original_path_source then ". Note: source is an endpoint." else "") in - let report_one (path_source, sink, _) = + let report_one {TraceDomain.issue; path_source; path_sink} = let open TraceDomain in let rec expand_source source0 ((report_acc, seen_acc) as acc) = let kind = Source.kind source0 in @@ -215,7 +215,9 @@ module Make (TaintSpecification : TaintSpec.S) = struct | Footprint _ -> ([(None, path_source)], CallSite.Set.empty) in - let expanded_sinks, _ = expand_sink sink sink_indexes ([sink], CallSite.Set.empty) in + let expanded_sinks, _ = + expand_sink path_sink sink_indexes ([path_sink], CallSite.Set.empty) + in let source_trace = let pp_access_path_opt fmt = function | None -> @@ -251,12 +253,11 @@ module Make (TaintSpecification : TaintSpec.S) = struct Errlog.make_trace_element 0 (CallSite.loc call_site) desc []) expanded_sinks in - let msg = IssueType.quandary_taint_error.unique_id in let _, original_path_source = List.hd_exn expanded_sources in let final_sink = List.hd_exn expanded_sinks in let trace_str = get_short_trace_string original_path_source final_sink in let ltr = source_trace @ List.rev sink_trace in - let exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in + let exn = Exceptions.Checkers (issue.unique_id, Localise.verbatim_desc trace_str) in Reporting.log_error proc_data.extras.summary ~loc:(CallSite.loc cur_site) ~ltr exn in List.iter ~f:report_one (TraceDomain.get_reports ~cur_site trace) diff --git a/infer/src/unit/TaintTests.ml b/infer/src/unit/TaintTests.ml index 8c741c943..2dce4d83e 100644 --- a/infer/src/unit/TaintTests.ml +++ b/infer/src/unit/TaintTests.ml @@ -39,7 +39,7 @@ module MockTrace = Trace.Make (struct end) - let should_report _ _ = false + let get_report _ _ = None end) module MockTaintAnalysis = TaintAnalysis.Make (struct diff --git a/infer/src/unit/TraceTests.ml b/infer/src/unit/TraceTests.ml index 0834455eb..23a855447 100644 --- a/infer/src/unit/TraceTests.ml +++ b/infer/src/unit/TraceTests.ml @@ -80,8 +80,10 @@ module MockTrace = Trace.Make (struct module Source = MockSource module Sink = MockSink - let should_report source sink = - [%compare.equal : MockTraceElem.t] (Source.kind source) (Sink.kind sink) + let get_report source sink = + if [%compare.equal : MockTraceElem.t] (Source.kind source) (Sink.kind sink) then + Some IssueType.quandary_taint_error + else None end) @@ -111,11 +113,13 @@ let tests = assert_equal (List.length reports) 2 ; assert_bool "Reports should contain source1 -> sink1" (List.exists - ~f:(fun (source, sink, _) -> source_equal source source1 && MockSink.equal sink sink1) + ~f:(fun {MockTrace.path_source; path_sink} -> + source_equal path_source source1 && MockSink.equal path_sink sink1) reports) ; assert_bool "Reports should contain source2 -> sink2" (List.exists - ~f:(fun (source, sink, _) -> source_equal source source2 && MockSink.equal sink sink2) + ~f:(fun {MockTrace.path_source; path_sink} -> + source_equal path_source source2 && MockSink.equal path_sink sink2) reports) in "get_reports" >:: get_reports_