From patchwork Sun Nov 13 23:03:09 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 60560 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0490538582BE for ; Sun, 13 Nov 2022 23:03:52 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0490538582BE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1668380632; bh=UhUwzqEVGM0eRc2N9nYP7AtyGgpq0hE2giFvrXoyGTo=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=U+5OKdt+Tp+UCgAVIVQ+nCu/42QrH0wb15kINUJUkrl7IoIRoeG8Fvqg4C/rDa4Rw x/+MFa8Mmy3r+VWuS302hD8pCbPZ7fNONdfJGhdsn1eSrDepiaHDI5l7u8YHGHGnAU mLSwA4uIGvTJ9PfVMHQkXgJasSh3oh2xlxDdeHWk= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id A04033857B9B for ; Sun, 13 Nov 2022 23:03:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A04033857B9B Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-527-xQPR7jB-MSOpv3LjN5xYHw-1; Sun, 13 Nov 2022 18:03:13 -0500 X-MC-Unique: xQPR7jB-MSOpv3LjN5xYHw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id C42B3886462 for ; Sun, 13 Nov 2022 23:03:12 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.17.189]) by smtp.corp.redhat.com (Postfix) with ESMTP id 869454022D7; Sun, 13 Nov 2022 23:03:12 +0000 (UTC) To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] analyzer: new warning: -Wanalyzer-tainted-assertion [PR106235] Date: Sun, 13 Nov 2022 18:03:09 -0500 Message-Id: <20221113230309.2819841-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: David Malcolm via Gcc-patches From: David Malcolm Reply-To: David Malcolm Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" This patch adds a new -Wanalyzer-tainted-assertion warning to -fanalyzer's "taint" mode (which also requires -fanalyzer-checker=taint). It complains about attacker-controlled values being used in assertions, or in any expression affecting control flow that guards a "noreturn" function. As noted in the docs part of the patch, in such cases: - when assertion-checking is enabled: an attacker could trigger a denial of service by injecting an assertion failure - when assertion-checking is disabled, such as by defining NDEBUG, an attacker could inject data that subverts the process, since it presumably violates a precondition that is being assumed by the code. For example, given: #include int __attribute__((tainted_args)) test_tainted_assert (int n) { assert (n > 0); return n * n; } compiling with -fanalyzer -fanalyzer-checker=taint gives: t.c: In function 'test_tainted_assert': t.c:6:3: warning: use of attacked-controlled value in condition for assertion [CWE-617] [-Wanalyzer-tainted-assertion] 6 | assert (n > 0); | ^~~~~~ 'test_tainted_assert': event 1 | | 4 | test_tainted_assert (int n) | | ^~~~~~~~~~~~~~~~~~~ | | | | | (1) function 'test_tainted_assert' marked with '__attribute__((tainted_args))' | +--> 'test_tainted_assert': event 2 | | 4 | test_tainted_assert (int n) | | ^~~~~~~~~~~~~~~~~~~ | | | | | (2) entry to 'test_tainted_assert' | 'test_tainted_assert': events 3-6 | |/usr/include/assert.h:106:10: | 106 | if (expr) \ | | ^ | | | | | (3) use of attacker-controlled value for control flow | | (4) following 'false' branch (when 'n <= 0')... |...... | 109 | __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION); \ | | ~~~~~~~~~~~~~ | | | | | (5) ...to here | | (6) treating '__assert_fail' as an assertion failure handler due to '__attribute__((__noreturn__))' | The testcases have various examples for BUG and BUG_ON from the Linux kernel; there, the diagnostic treats "panic" as an assertion failure handler, due to '__attribute__((__noreturn__))'. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-3947-gd777b38cde91a8. gcc/analyzer/ChangeLog: PR analyzer/106235 * analyzer.opt (Wanalyzer-tainted-assertion): New. * checker-path.cc (checker_path::fixup_locations): Pass false to pending_diagnostic::fixup_location. * diagnostic-manager.cc (get_emission_location): Pass true to pending_diagnostic::fixup_location. * pending-diagnostic.cc (pending_diagnostic::fixup_location): Add bool param. * pending-diagnostic.h (pending_diagnostic::fixup_location): Add bool param to decl. * sm-taint.cc (taint_state_machine::m_tainted_control_flow): New. (taint_diagnostic::describe_state_change): Drop "final". (class tainted_assertion): New. (taint_state_machine::taint_state_machine): Initialize m_tainted_control_flow. (taint_state_machine::alt_get_inherited_state): Support comparisons being tainted, based on their arguments. (is_assertion_failure_handler_p): New. (taint_state_machine::on_stmt): Complain about calls to assertion failure handlers guarded by an attacker-controller conditional. Detect attacker-controlled gcond conditionals and gswitch index values. (taint_state_machine::check_control_flow_arg_for_taint): New. gcc/ChangeLog: PR analyzer/106235 * doc/gcc/gcc-command-options/option-summary.rst: Add -Wno-analyzer-tainted-assertion. * doc/gcc/gcc-command-options/options-that-control-static-analysis.rst: Add -Wno-analyzer-tainted-assertion. gcc/testsuite/ChangeLog: PR analyzer/106235 * gcc.dg/analyzer/taint-assert-BUG_ON.c: New test. * gcc.dg/analyzer/taint-assert-macro-expansion.c: New test. * gcc.dg/analyzer/taint-assert.c: New test. * gcc.dg/analyzer/taint-assert-system-header.c: New test. * gcc.dg/analyzer/test-assert.h: New header. * gcc.dg/plugin/analyzer_gil_plugin.c (gil_diagnostic::fixup_location): Add bool param. Signed-off-by: David Malcolm --- gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/checker-path.cc | 2 +- gcc/analyzer/diagnostic-manager.cc | 2 +- gcc/analyzer/pending-diagnostic.cc | 2 +- gcc/analyzer/pending-diagnostic.h | 8 +- gcc/analyzer/sm-taint.cc | 183 ++++++++- .../gcc-command-options/option-summary.rst | 1 + .../options-that-control-static-analysis.rst | 60 +++ .../gcc.dg/analyzer/taint-assert-BUG_ON.c | 76 ++++ .../analyzer/taint-assert-macro-expansion.c | 96 +++++ .../analyzer/taint-assert-system-header.c | 52 +++ gcc/testsuite/gcc.dg/analyzer/taint-assert.c | 346 ++++++++++++++++++ gcc/testsuite/gcc.dg/analyzer/test-assert.h | 7 + .../gcc.dg/plugin/analyzer_gil_plugin.c | 3 +- 14 files changed, 821 insertions(+), 21 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-assert-BUG_ON.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-assert-macro-expansion.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-assert-system-header.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/taint-assert.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/test-assert.h diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 518a5d422ff..49448697046 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -174,6 +174,10 @@ Wanalyzer-tainted-array-index Common Var(warn_analyzer_tainted_array_index) Init(1) Warning Warn about code paths in which an unsanitized value is used as an array index. +Wanalyzer-tainted-assertion +Common Var(warn_analyzer_tainted_assertion) Init(1) Warning +Warn about code paths in which an 'assert()' is made involving an unsanitized value. + Wanalyzer-tainted-divisor Common Var(warn_analyzer_tainted_divisor) Init(1) Warning Warn about code paths in which an unsanitized value is used as a divisor. diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc index 39de7453f51..e548ede3821 100644 --- a/gcc/analyzer/checker-path.cc +++ b/gcc/analyzer/checker-path.cc @@ -1316,7 +1316,7 @@ void checker_path::fixup_locations (pending_diagnostic *pd) { for (checker_event *e : m_events) - e->set_location (pd->fixup_location (e->get_location ())); + e->set_location (pd->fixup_location (e->get_location (), false)); } /* Return true if there is a (start_cfg_edge_event, end_cfg_edge_event) pair diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index 9b319c3a3f5..1b19e58201b 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -933,7 +933,7 @@ get_emission_location (const gimple *stmt, function *fun, location_t loc = get_stmt_location (stmt, fun); /* Allow the pending_diagnostic to fix up the location. */ - loc = pd.fixup_location (loc); + loc = pd.fixup_location (loc, true); return loc; } diff --git a/gcc/analyzer/pending-diagnostic.cc b/gcc/analyzer/pending-diagnostic.cc index 9216a22e64d..53cab2065dd 100644 --- a/gcc/analyzer/pending-diagnostic.cc +++ b/gcc/analyzer/pending-diagnostic.cc @@ -153,7 +153,7 @@ fixup_location_in_macro_p (cpp_hashnode *macro) Don't unwind inside macros for which fixup_location_in_macro_p is true. */ location_t -pending_diagnostic::fixup_location (location_t loc) const +pending_diagnostic::fixup_location (location_t loc, bool) const { if (linemap_location_from_macro_expansion_p (line_table, loc)) { diff --git a/gcc/analyzer/pending-diagnostic.h b/gcc/analyzer/pending-diagnostic.h index 0e91e71a208..5d5d126b342 100644 --- a/gcc/analyzer/pending-diagnostic.h +++ b/gcc/analyzer/pending-diagnostic.h @@ -214,10 +214,10 @@ class pending_diagnostic diagnostic deduplication. */ static bool same_tree_p (tree t1, tree t2); - /* A vfunc for fixing up locations (both the primary location for the - diagnostic, and for events in their paths), e.g. to avoid unwinding - inside specific macros. */ - virtual location_t fixup_location (location_t loc) const; + /* Vfunc for fixing up locations, e.g. to avoid unwinding + inside specific macros. PRIMARY is true for the primary location + for the diagnostic, and FALSE for events in their paths. */ + virtual location_t fixup_location (location_t loc, bool primary) const; /* For greatest precision-of-wording, the various following "describe_*" virtual functions give the pending diagnostic a way to describe events diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc index d4ee6c435b8..a2b442a4ef2 100644 --- a/gcc/analyzer/sm-taint.cc +++ b/gcc/analyzer/sm-taint.cc @@ -109,6 +109,10 @@ public: state_t combine_states (state_t s0, state_t s1) const; private: + void check_control_flow_arg_for_taint (sm_context *sm_ctxt, + const gimple *stmt, + tree expr) const; + void check_for_tainted_size_arg (sm_context *sm_ctxt, const supernode *node, const gcall *call, @@ -130,6 +134,9 @@ public: /* Stop state, for a value we don't want to track any more. */ state_t m_stop; + + /* Global state, for when the last condition had tainted arguments. */ + state_t m_tainted_control_flow; }; /* Class for diagnostics relating to taint_state_machine. */ @@ -149,8 +156,7 @@ public: && m_has_bounds == other.m_has_bounds); } - label_text describe_state_change (const evdesc::state_change &change) - final override + label_text describe_state_change (const evdesc::state_change &change) override { if (change.m_new_state == m_sm.m_tainted) { @@ -761,6 +767,100 @@ private: enum memory_space m_mem_space; }; +/* Concrete taint_diagnostic subclass for reporting attacker-controlled + value being used as part of the condition of an assertion. */ + +class tainted_assertion : public taint_diagnostic +{ +public: + tainted_assertion (const taint_state_machine &sm, tree arg, + tree assert_failure_fndecl) + : taint_diagnostic (sm, arg, BOUNDS_NONE), + m_assert_failure_fndecl (assert_failure_fndecl) + { + gcc_assert (m_assert_failure_fndecl); + } + + const char *get_kind () const final override + { + return "tainted_assertion"; + } + + bool subclass_equal_p (const pending_diagnostic &base_other) const override + { + if (!taint_diagnostic::subclass_equal_p (base_other)) + return false; + const tainted_assertion &other + = (const tainted_assertion &)base_other; + return m_assert_failure_fndecl == other.m_assert_failure_fndecl; + } + + int get_controlling_option () const final override + { + return OPT_Wanalyzer_tainted_assertion; + } + + bool emit (rich_location *rich_loc) final override + { + diagnostic_metadata m; + /* "CWE-617: Reachable Assertion". */ + m.add_cwe (617); + + return warning_meta (rich_loc, m, get_controlling_option (), + "use of attacked-controlled value in" + " condition for assertion"); + } + + location_t fixup_location (location_t loc, + bool primary) const final override + { + if (primary) + /* For the primary location we want to avoid being in e.g. the + system header, since this would suppress the + diagnostic. */ + return expansion_point_location_if_in_system_header (loc); + else if (in_system_header_at (loc)) + /* For events, we want to show the implemenation of the assert + macro when we're describing them. */ + return linemap_resolve_location (line_table, loc, + LRK_SPELLING_LOCATION, + NULL); + else + return pending_diagnostic::fixup_location (loc, primary); + } + + label_text describe_state_change (const evdesc::state_change &change) override + { + if (change.m_new_state == m_sm.m_tainted_control_flow) + return change.formatted_print + ("use of attacker-controlled value for control flow"); + return taint_diagnostic::describe_state_change (change); + } + + label_text describe_final_event (const evdesc::final_event &ev) final override + { + if (mention_noreturn_attribute_p ()) + return ev.formatted_print + ("treating %qE as an assertion failure handler" + " due to %<__attribute__((__noreturn__))%>", + m_assert_failure_fndecl); + else + return ev.formatted_print + ("treating %qE as an assertion failure handler", + m_assert_failure_fndecl); + } + +private: + bool mention_noreturn_attribute_p () const + { + if (fndecl_built_in_p (m_assert_failure_fndecl, BUILT_IN_UNREACHABLE)) + return false; + return true; + } + + tree m_assert_failure_fndecl; +}; + /* taint_state_machine's ctor. */ taint_state_machine::taint_state_machine (logger *logger) @@ -770,6 +870,7 @@ taint_state_machine::taint_state_machine (logger *logger) m_has_lb = add_state ("has_lb"); m_has_ub = add_state ("has_ub"); m_stop = add_state ("stop"); + m_tainted_control_flow = add_state ("tainted-control-flow"); } state_machine::state_t @@ -810,6 +911,15 @@ taint_state_machine::alt_get_inherited_state (const sm_state_map &map, { default: break; + + case EQ_EXPR: + case GE_EXPR: + case LE_EXPR: + case NE_EXPR: + case GT_EXPR: + case LT_EXPR: + case UNORDERED_EXPR: + case ORDERED_EXPR: case PLUS_EXPR: case MINUS_EXPR: case MULT_EXPR: @@ -823,17 +933,6 @@ taint_state_machine::alt_get_inherited_state (const sm_state_map &map, } break; - case EQ_EXPR: - case GE_EXPR: - case LE_EXPR: - case NE_EXPR: - case GT_EXPR: - case LT_EXPR: - case UNORDERED_EXPR: - case ORDERED_EXPR: - /* Comparisons are just booleans. */ - return m_start; - case BIT_AND_EXPR: case RSHIFT_EXPR: return NULL; @@ -844,6 +943,19 @@ taint_state_machine::alt_get_inherited_state (const sm_state_map &map, return NULL; } +/* Return true iff FNDECL should be considered to be an assertion failure + handler by -Wanalyzer-tainted-assertion. */ + +static bool +is_assertion_failure_handler_p (tree fndecl) +{ + // i.e. "noreturn" + if (TREE_THIS_VOLATILE (fndecl)) + return true; + + return false; +} + /* Implementation of state_machine::on_stmt vfunc for taint_state_machine. */ bool @@ -871,6 +983,14 @@ taint_state_machine::on_stmt (sm_context *sm_ctxt, /* External function with "access" attribute. */ if (sm_ctxt->unknown_side_effects_p ()) check_for_tainted_size_arg (sm_ctxt, node, call, callee_fndecl); + + if (is_assertion_failure_handler_p (callee_fndecl) + && sm_ctxt->get_global_state () == m_tainted_control_flow) + { + sm_ctxt->warn (node, call, NULL_TREE, + make_unique (*this, NULL_TREE, + callee_fndecl)); + } } // TODO: ...etc; many other sources of untrusted data @@ -897,9 +1017,46 @@ taint_state_machine::on_stmt (sm_context *sm_ctxt, } } + if (const gcond *cond = dyn_cast (stmt)) + { + /* Reset the state of "tainted-control-flow" before each + control flow statement, so that only the last one before + an assertion-failure-handler counts. */ + sm_ctxt->set_global_state (m_start); + check_control_flow_arg_for_taint (sm_ctxt, cond, gimple_cond_lhs (cond)); + check_control_flow_arg_for_taint (sm_ctxt, cond, gimple_cond_rhs (cond)); + } + + if (const gswitch *switch_ = dyn_cast (stmt)) + { + /* Reset the state of "tainted-control-flow" before each + control flow statement, so that only the last one before + an assertion-failure-handler counts. */ + sm_ctxt->set_global_state (m_start); + check_control_flow_arg_for_taint (sm_ctxt, switch_, + gimple_switch_index (switch_)); + } + return false; } +/* If EXPR is tainted, mark this execution path with the + "tainted-control-flow" global state, in case we're about + to call an assertion-failure-handler. */ + +void +taint_state_machine::check_control_flow_arg_for_taint (sm_context *sm_ctxt, + const gimple *stmt, + tree expr) const +{ + const region_model *old_model = sm_ctxt->get_old_region_model (); + const svalue *sval = old_model->get_rvalue (expr, NULL); + state_t state = sm_ctxt->get_state (stmt, sval); + enum bounds b; + if (get_taint (state, TREE_TYPE (expr), &b)) + sm_ctxt->set_global_state (m_tainted_control_flow); +} + /* Implementation of state_machine::on_condition vfunc for taint_state_machine. Potentially transition state 'tainted' to 'has_ub' or 'has_lb', and states 'has_ub' and 'has_lb' to 'stop'. */ diff --git a/gcc/doc/gcc/gcc-command-options/option-summary.rst b/gcc/doc/gcc/gcc-command-options/option-summary.rst index d068f98feac..b90b6600d70 100644 --- a/gcc/doc/gcc/gcc-command-options/option-summary.rst +++ b/gcc/doc/gcc/gcc-command-options/option-summary.rst @@ -309,6 +309,7 @@ in the following sections. :option:`-Wno-analyzer-shift-count-overflow` |gol| :option:`-Wno-analyzer-stale-setjmp-buffer` |gol| :option:`-Wno-analyzer-tainted-allocation-size` |gol| + :option:`-Wno-analyzer-tainted-assertion` |gol| :option:`-Wno-analyzer-tainted-array-index` |gol| :option:`-Wno-analyzer-tainted-divisor` |gol| :option:`-Wno-analyzer-tainted-offset` |gol| diff --git a/gcc/doc/gcc/gcc-command-options/options-that-control-static-analysis.rst b/gcc/doc/gcc/gcc-command-options/options-that-control-static-analysis.rst index 32a626c16a9..18f73d95e1e 100644 --- a/gcc/doc/gcc/gcc-command-options/options-that-control-static-analysis.rst +++ b/gcc/doc/gcc/gcc-command-options/options-that-control-static-analysis.rst @@ -549,6 +549,66 @@ Options That Control Static Analysis Default setting; overrides :option:`-Wno-analyzer-tainted-allocation-size`. +.. option:: -Wno-analyzer-tainted-assertion + + This warning requires both :option:`-fanalyzer` and + :option:`-fanalyzer-checker=taint` to enable it; + use :option:`-Wno-analyzer-tainted-assertion` to disable it. + + This diagnostic warns for paths through the code in which a value + that could be under an attacker's control is used as part of a + condition without being first sanitized, and that condition guards a + call to a function marked with attribute :fn-attr:`noreturn` + (such as the function ``__builtin_unreachable``). Such functions + typically indicate abnormal termination of the program, such as for + assertion failure handlers. For example: + + .. code-block:: c + + assert (some_tainted_value < SOME_LIMIT); + + In such cases: + + * when assertion-checking is enabled: an attacker could trigger + a denial of service by injecting an assertion failure + + * when assertion-checking is disabled, such as by defining ``NDEBUG``, + an attacker could inject data that subverts the process, since it + presumably violates a precondition that is being assumed by the code. + + Note that when assertion-checking is disabled, the assertions are + typically removed by the preprocessor before the analyzer has a chance + to "see" them, so this diagnostic can only generate warnings on builds + in which assertion-checking is enabled. + + For the purpose of this warning, any function marked with attribute + :fn-attr:`noreturn` is considered as a possible assertion failure + handler, including ``__builtin_unreachable``. Note that these functions + are sometimes removed by the optimizer before the analyzer "sees" them. + Hence optimization should be disabled when attempting to trigger this + diagnostic. + + See `CWE-617: Reachable Assertion `_. + + The warning can also report problematic constructions such as + + .. code-block:: c + + switch (some_tainted_value) { + case 0: + /* [...etc; various valid cases omitted...] */ + break; + + default: + __builtin_unreachable (); /* BUG: attacker can trigger this */ + } + + despite the above not being an assertion failure, strictly speaking. + +.. option:: -Wanalyzer-tainted-assertion + + Default setting; overrides :option:`-Wno-analyzer-tainted-assertion`. + .. option:: -Wno-analyzer-tainted-array-index This warning requires both :option:`-fanalyzer` and diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-assert-BUG_ON.c b/gcc/testsuite/gcc.dg/analyzer/taint-assert-BUG_ON.c new file mode 100644 index 00000000000..8aef0a44a6d --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/taint-assert-BUG_ON.c @@ -0,0 +1,76 @@ +// TODO: remove need for this option +/* { dg-additional-options "-fanalyzer-checker=taint" } */ + +/* We need this, otherwise the warnings are emitted inside the macros, which + makes it hard to write the DejaGnu directives. */ +/* { dg-additional-options " -ftrack-macro-expansion=0" } */ + +/* Adapted from code in the Linux kernel, which has this: */ +/* SPDX-License-Identifier: GPL-2.0 */ + +#define __noreturn __attribute__ ((__noreturn__)) + +void panic(const char *fmt, ...) __noreturn; + +int _printk(const char *fmt, ...); +#define __printk_index_emit(...) do {} while (0) +#define printk_index_wrap(_p_func, _fmt, ...) \ + ({ \ + __printk_index_emit(_fmt, NULL, NULL); \ + _p_func(_fmt, ##__VA_ARGS__); \ + }) +#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) +#define barrier_before_unreachable() do { } while (0) + +#define BUG() do { \ + printk("BUG: failure at %s:%d/%s()!\n", __FILE__, __LINE__, __func__); \ + barrier_before_unreachable(); \ + panic("BUG!"); \ +} while (0) + +#define BUG_ON(condition) do { if (condition) BUG(); } while (0) + +void __attribute__((tainted_args)) +test_BUG(int n) +{ + if (n > 100) /* { dg-message "use of attacker-controlled value for control flow" } */ + BUG(); /* { dg-warning "-Wanalyzer-tainted-assertion" "warning" } */ + /* { dg-message "treating 'panic' as an assertion failure handler due to '__attribute__\\(\\(__noreturn__\\)\\)'" "final event" { target *-*-* } .-1 } */ +} + +void __attribute__((tainted_args)) +test_BUG_ON(int n) +{ + BUG_ON(n > 100); /* { dg-warning "-Wanalyzer-tainted-assertion" "warning" } */ + /* { dg-message "treating 'panic' as an assertion failure handler due to '__attribute__\\(\\(__noreturn__\\)\\)'" "final event" { target *-*-* } .-1 } */ +} + +int __attribute__((tainted_args)) +test_switch_BUG_1(int n) +{ + switch (n) { /* { dg-message "use of attacker-controlled value for control flow" } */ + default: + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + case 42: + BUG (); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ + } +} + +int __attribute__((tainted_args)) +test_switch_BUG(int n) +{ + switch (n) { /* { dg-message "use of attacker-controlled value for control flow" } */ + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + } + BUG (); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-assert-macro-expansion.c b/gcc/testsuite/gcc.dg/analyzer/taint-assert-macro-expansion.c new file mode 100644 index 00000000000..24b175a0973 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/taint-assert-macro-expansion.c @@ -0,0 +1,96 @@ +/* Integration test of how the execution path looks for + -Wanalyzer-tainted-assertion with macro-tracking enabled + (the default). */ + +// TODO: remove need for this option +/* { dg-additional-options "-fanalyzer-checker=taint" } */ + +/* { dg-additional-options "-fdiagnostics-show-path-depths" } */ +/* { dg-additional-options "-fdiagnostics-path-format=inline-events -fdiagnostics-show-caret" } */ + +/* An assertion macro that has a call to a __noreturn__ function. */ + +extern void my_assert_fail (const char *expr, const char *file, int line) + __attribute__ ((__noreturn__)); + +#define MY_ASSERT_1(EXPR) \ + do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) /* { dg-warning "use of attacked-controlled value in condition for assertion \\\[CWE-617\\\] \\\[-Wanalyzer-tainted-assertion\\\]" } */ + +int __attribute__((tainted_args)) +test_tainted_MY_ASSERT_1 (int n) +{ + MY_ASSERT_1 (n > 0); + return n * n; +} + +/* { dg-begin-multiline-output "" } + do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + { dg-end-multiline-output "" } */ +// note: in expansion of macro 'MY_ASSERT_1' +/* { dg-begin-multiline-output "" } + MY_ASSERT_1 (n > 0); + ^~~~~~~~~~~ + 'test_tainted_MY_ASSERT_1': event 1 (depth 0) + | + | test_tainted_MY_ASSERT_1 (int n) + | ^~~~~~~~~~~~~~~~~~~~~~~~ + | | + | (1) function 'test_tainted_MY_ASSERT_1' marked with '__attribute__((tainted_args))' + | + +--> 'test_tainted_MY_ASSERT_1': event 2 (depth 1) + | + | test_tainted_MY_ASSERT_1 (int n) + | ^~~~~~~~~~~~~~~~~~~~~~~~ + | | + | (2) entry to 'test_tainted_MY_ASSERT_1' + | + 'test_tainted_MY_ASSERT_1': event 3 (depth 1) + | + | do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + | ^ + | | + | (3) use of attacker-controlled value for control flow + { dg-end-multiline-output "" } */ +// note: in expansion of macro 'MY_ASSERT_1' +/* { dg-begin-multiline-output "" } + | MY_ASSERT_1 (n > 0); + | ^~~~~~~~~~~ + | + 'test_tainted_MY_ASSERT_1': event 4 (depth 1) + | + | do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + | ^ + | | + | (4) following 'true' branch (when 'n <= 0')... + { dg-end-multiline-output "" } */ +// note: in expansion of macro 'MY_ASSERT_1' +/* { dg-begin-multiline-output "" } + | MY_ASSERT_1 (n > 0); + | ^~~~~~~~~~~ + | + 'test_tainted_MY_ASSERT_1': event 5 (depth 1) + | + | do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + | | + | (5) ...to here + { dg-end-multiline-output "" } */ +// note: in expansion of macro 'MY_ASSERT_1' +/* { dg-begin-multiline-output "" } + | MY_ASSERT_1 (n > 0); + | ^~~~~~~~~~~ + | + 'test_tainted_MY_ASSERT_1': event 6 (depth 1) + | + | do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + | | + | (6) treating 'my_assert_fail' as an assertion failure handler due to '__attribute__((__noreturn__))' + { dg-end-multiline-output "" } */ +// note: in expansion of macro 'MY_ASSERT_1' +/* { dg-begin-multiline-output "" } + | MY_ASSERT_1 (n > 0); + | ^~~~~~~~~~~ + | + { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-assert-system-header.c b/gcc/testsuite/gcc.dg/analyzer/taint-assert-system-header.c new file mode 100644 index 00000000000..a65853c7886 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/taint-assert-system-header.c @@ -0,0 +1,52 @@ +/* Integration test of how the execution path looks for + -Wanalyzer-tainted-assertion with macro-tracking enabled + (the default), where the assertion macro is defined in + a system header. */ + +// TODO: remove need for this option +/* { dg-additional-options "-fanalyzer-checker=taint" } */ + +/* { dg-additional-options "-fdiagnostics-show-path-depths" } */ +/* { dg-additional-options "-fdiagnostics-path-format=inline-events -fdiagnostics-show-caret" } */ + +/* An assertion macro that has a call to a __noreturn__ function. */ + +/* This is marked as a system header. */ +#include "test-assert.h" + +int __attribute__((tainted_args)) +test_tainted_assert (int n) +{ + assert (n > 0); /* { dg-warning "use of attacked-controlled value in condition for assertion \\\[CWE-617\\\] \\\[-Wanalyzer-tainted-assertion\\\]" } */ + return n * n; +} + +/* { dg-begin-multiline-output "" } + assert (n > 0); + ^~~~~~ + 'test_tainted_assert': event 1 (depth 0) + | + | test_tainted_assert (int n) + | ^~~~~~~~~~~~~~~~~~~ + | | + | (1) function 'test_tainted_assert' marked with '__attribute__((tainted_args))' + | + +--> 'test_tainted_assert': event 2 (depth 1) + | + | test_tainted_assert (int n) + | ^~~~~~~~~~~~~~~~~~~ + | | + | (2) entry to 'test_tainted_assert' + | + 'test_tainted_assert': events 3-6 (depth 1) + | + | + | do { if (!(EXPR)) __assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + | ^ ~~~~~~~~~~~~~ + | | | + | | (5) ...to here + | | (6) treating '__assert_fail' as an assertion failure handler due to '__attribute__((__noreturn__))' + | (3) use of attacker-controlled value for control flow + | (4) following 'true' branch (when 'n <= 0')... + | + { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/taint-assert.c b/gcc/testsuite/gcc.dg/analyzer/taint-assert.c new file mode 100644 index 00000000000..b09f8c51a16 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/taint-assert.c @@ -0,0 +1,346 @@ +// TODO: remove need for this option +/* { dg-additional-options "-fanalyzer-checker=taint" } */ + +/* We need this, otherwise the warnings are emitted inside the macros, which + makes it hard to write the DejaGnu directives. */ +/* { dg-additional-options " -ftrack-macro-expansion=0" } */ + +#include "analyzer-decls.h" + +/* An assertion macro that has a call to a __noreturn__ function. */ + +extern void my_assert_fail (const char *expr, const char *file, int line) + __attribute__ ((__noreturn__)); + +#define MY_ASSERT_1(EXPR) \ + do { if (!(EXPR)) my_assert_fail (#EXPR, __FILE__, __LINE__); } while (0) + +int +test_not_tainted_MY_ASSERT_1 (int n) +{ + MY_ASSERT_1 (n > 0); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return n * n; +} + +int __attribute__((tainted_args)) +test_tainted_MY_ASSERT_1 (int n) +{ + MY_ASSERT_1 (n > 0); /* { dg-warning "use of attacked-controlled value in condition for assertion \\\[CWE-617\\\] \\\[-Wanalyzer-tainted-assertion\\\]" "warning" } */ + /* { dg-message "treating 'my_assert_fail' as an assertion failure handler due to '__attribute__\\(\\(__noreturn__\\)\\)'" "final event" { target *-*-* } .-1 } */ + return n * n; +} + + +/* An assertion macro that has a call to __builtin_unreachable. */ + +#define MY_ASSERT_2(EXPR) \ + do { if (!(EXPR)) __builtin_unreachable (); } while (0) + +int +test_not_tainted_MY_ASSERT_2 (int n) +{ + MY_ASSERT_2 (n > 0); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return n * n; +} + +int __attribute__((tainted_args)) +test_tainted_MY_ASSERT_2 (int n) +{ + MY_ASSERT_2 (n > 0); /* { dg-warning "-Wanalyzer-tainted-assertion" "warning" } */ + /* { dg-message "treating '__builtin_unreachable' as an assertion failure handler" "final event" { target *-*-* } .-1 } */ + return n * n; +} + + +/* An assertion macro that's preprocessed away. + The analyzer doesn't see this, so can't warn. */ + +#define MY_ASSERT_3(EXPR) do { } while (0) + +int +test_not_tainted_MY_ASSERT_3 (int n) +{ + MY_ASSERT_3 (n > 0); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return n * n; +} + +int __attribute__((tainted_args)) +test_tainted_MY_ASSERT_3 (int n) +{ + MY_ASSERT_3 (n > 0); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return n * n; +} + + +/* A macro that isn't an assertion. */ + +extern void do_something_benign (); + +#define NOT_AN_ASSERT(EXPR) \ + do { if (!(EXPR)) do_something_benign (); } while (0) + +int +test_not_tainted_NOT_AN_ASSERT (int n) +{ + NOT_AN_ASSERT (n > 0); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return n * n; +} + +int __attribute__((tainted_args)) +test_tainted_NOT_AN_ASSERT (int n) +{ + NOT_AN_ASSERT (n > 0); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return n * n; +} + + +/* A condition that isn't an assertion. */ + +int __attribute__((tainted_args)) +test_tainted_condition (int n) +{ + if (n > 0) /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + return 1; + else + return -1; +} + + +/* More complicated expressions in assertions. */ + +int g; + +void __attribute__((tainted_args)) +test_compound_condition_in_assert_1 (int n) +{ + MY_ASSERT_1 ((n * 2) < (g + 3)); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_compound_condition_in_assert_2 (int x, int y) +{ + MY_ASSERT_1 (x < 100 && y < 100); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_compound_condition_in_assert_3 (int x, int y) +{ + MY_ASSERT_1 (x < 100 || y < 100); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_sanitized_expression_in_assert (int n) +{ + __analyzer_dump_state ("taint", n); /* { dg-warning "state: 'tainted'" } */ + if (n < 0 || n >= 100) + return; + __analyzer_dump_state ("taint", n); /* { dg-warning "state: 'stop'" } */ + MY_ASSERT_1 (n < 200); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_sanitization_then_ok_assertion (unsigned n) +{ + if (n >= 100) + return; + + /* Shouldn't warn here, as g isn't attacker-controlled. */ + MY_ASSERT_1 (g > 42); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_good_assert_then_bad_assert (unsigned n) +{ + /* Shouldn't warn here, as g isn't attacker-controlled. */ + MY_ASSERT_1 (g > 42); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ + + /* ...but n is: */ + MY_ASSERT_1 (n < 100); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_bad_assert_then_good_assert (unsigned n) +{ + MY_ASSERT_1 (n < 100); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ + MY_ASSERT_1 (g > 42); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ +} + + +/* */ + +void __attribute__((tainted_args)) +test_zero_MY_ASSERT_1 (unsigned n) +{ + if (n >= 100) + MY_ASSERT_1 (0); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_nonzero_MY_ASSERT_1 (unsigned n) +{ + if (n >= 100) + MY_ASSERT_1 (1); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_zero_MY_ASSERT_2 (unsigned n) +{ + if (n >= 100) + MY_ASSERT_2 (0); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +void __attribute__((tainted_args)) +test_nonzero_MY_ASSERT_2 (unsigned n) +{ + if (n >= 100) + MY_ASSERT_2 (1); /* { dg-bogus "-Wanalyzer-tainted-assertion" } */ +} + + +/* Assertions that call a subroutine to do validity checking. */ + +static int +__analyzer_valid_1 (int x) +{ + return x < 100; +} + +void __attribute__((tainted_args)) +test_assert_calling_valid_1 (int n) +{ + MY_ASSERT_1 (__analyzer_valid_1 (n)); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +static int +__analyzer_valid_2 (int x) +{ + return x < 100; +} + +void __attribute__((tainted_args)) +test_assert_calling_valid_2 (int n) +{ + MY_ASSERT_1 (__analyzer_valid_2 (n)); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} + +static int +__analyzer_valid_3 (int x, int y) +{ + if (x >= 100) + return 0; + if (y >= 100) + return 0; + return 1; +} + +void __attribute__((tainted_args)) +test_assert_calling_valid_3 (int a, int b) +{ + MY_ASSERT_1 (__analyzer_valid_3 (a, b)); /* { dg-warning "-Wanalyzer-tainted-assertion" "TODO" { xfail *-*-* } } */ +} + + +/* 'switch' statements with supposedly unreachable cases/defaults. */ + +int __attribute__((tainted_args)) +test_switch_default (int n) +{ + switch (n) /* { dg-message "use of attacker-controlled value for control flow" "why" } */ + /* { dg-message "following 'default:' branch" "dest" { target *-*-* } .-1 } */ + { + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + default: + /* The wording is rather inaccurate here. */ + __builtin_unreachable (); /* { dg-warning "use of attacked-controlled value in condition for assertion" } */ + } +} + +int __attribute__((tainted_args)) +test_switch_unhandled_case (int n) +{ + switch (n) /* { dg-message "use of attacker-controlled value for control flow" "why" } */ + /* { dg-message "following 'default:' branch" "dest" { target *-*-* } .-1 } */ + { + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + } + + /* The wording is rather inaccurate here. */ + __builtin_unreachable (); /* { dg-warning "use of attacked-controlled value in condition for assertion" } */ +} + +int __attribute__((tainted_args)) +test_switch_bogus_case_MY_ASSERT_1 (int n) +{ + switch (n) + { + default: + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + case 42: + MY_ASSERT_1 (0); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ + } +} + +int __attribute__((tainted_args)) +test_switch_bogus_case_MY_ASSERT_2 (int n) +{ + switch (n) + { + default: + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + case 42: + MY_ASSERT_2 (0); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ + } +} + +int __attribute__((tainted_args)) +test_switch_bogus_case_unreachable (int n) +{ + switch (n) + { + default: + case 0: + return 5; + case 1: + return 22; + case 2: + return -1; + case 42: + /* This case gets optimized away before we see it. */ + __builtin_unreachable (); + } +} + + +/* Contents of a struct. */ + +struct s +{ + int x; + int y; +}; + +int __attribute__((tainted_args)) +test_assert_struct (struct s *p) +{ + MY_ASSERT_1 (p->x < p->y); /* { dg-warning "-Wanalyzer-tainted-assertion" } */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/test-assert.h b/gcc/testsuite/gcc.dg/analyzer/test-assert.h new file mode 100644 index 00000000000..4a24b720ac8 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/test-assert.h @@ -0,0 +1,7 @@ +#pragma GCC system_header + +extern void __assert_fail (const char *expr, const char *file, int line) + __attribute__ ((__noreturn__)); + +#define assert(EXPR) \ + do { if (!(EXPR)) __assert_fail (#EXPR, __FILE__, __LINE__); } while (0) diff --git a/gcc/testsuite/gcc.dg/plugin/analyzer_gil_plugin.c b/gcc/testsuite/gcc.dg/plugin/analyzer_gil_plugin.c index b72856bf6f6..e494315bb34 100644 --- a/gcc/testsuite/gcc.dg/plugin/analyzer_gil_plugin.c +++ b/gcc/testsuite/gcc.dg/plugin/analyzer_gil_plugin.c @@ -89,7 +89,8 @@ public: return 0; } - location_t fixup_location (location_t loc) const final override + location_t fixup_location (location_t loc, + bool) const final override { /* Ideally we'd check for specific macros here, and only resolve certain macros. */