From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 9A5823858C52 for ; Tue, 16 Jan 2024 00:05:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9A5823858C52 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9A5823858C52 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705363556; cv=none; b=f8wODni21ih0I/zkU8YbUCRKX2mlNFFeZnDkYzLJP8S8s36dXIV+oP0uzvt688JFpoj3uqNQu4hSy5CLBeiz8Ivzp+TRUwlfO7ca1pmtj3Cez490S6eaq4PQUhT3uy30+P1EmG3byzUbWOABj+vlEH4VkvXEyApMLBI4atNJFwA= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705363556; c=relaxed/simple; bh=QqdpfSUVBn1a9qO5imR/uFWWcHV4y1jCvfnx7/AzlWQ=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=R6+97AssOha+EPg5nEsonKz6HxcjUG+17ems6yoQt4OG0n/YURHSZQLTd7P6gDEvoqmgN3grJNpzzs4+zko0WxREvI6kwz90Sx48CNyNURW0LcQabSoPiP3h4jZvwlp4INTuY8yPvWHKl347V0pxB9YMKwb749ZqEVObDz/CXww= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1705363553; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=3Q3wRLmxRfWuGBAS2k5oONZ7RZDbAv6TprttluXYh8k=; b=H3k9YJ3Cu7SBtlCsxy8mMmysWMXdL5PjMBYem3/4vUJxMX/fIyX5SPdjALgIke/rsVTp4A XS64m1J+6e+GX5Ec4jeeEZ7K4yEFrc8CtqEOgYyMwP3HzVarXcVVLQvq4jYbHuZL3OwkRu gMGpFwzB1125RPN9UsvJxbNrn4SlAvI= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-187-LUxWz4k7OfiKN27NdZSkxA-1; Mon, 15 Jan 2024 19:05:51 -0500 X-MC-Unique: LUxWz4k7OfiKN27NdZSkxA-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7FB4A1C05AB1 for ; Tue, 16 Jan 2024 00:05:51 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.22.16.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 564D9492BFA; Tue, 16 Jan 2024 00:05:51 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [pushed] analyzer: fix false +ves from -Wanalyzer-tainted-array-index with unsigned char index [PR106229] Date: Mon, 15 Jan 2024 19:05:50 -0500 Message-Id: <20240116000550.1054419-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Successful run of analyzer integration tests on x86_64-pc-linux-gnu. Pushed to trunk as r14-7266-gce27b66d952127. gcc/analyzer/ChangeLog: PR analyzer/106229 * analyzer.h (compare_constants): New decl. * constraint-manager.cc (compare_constants): Make non-static. * sm-taint.cc: Add include "fold-const.h". (class concrete_range): New. (get_possible_range): New. (index_can_be_out_of_bounds_p): New. (region_model::check_region_for_taint): Reject -Wanalyzer-tainted-array-index if the type of the value makes it impossible for it to be out-of-bounds of the array. gcc/testsuite/ChangeLog: PR analyzer/106229 * c-c++-common/analyzer/taint-index-pr106229.c: New test. Signed-off-by: David Malcolm --- gcc/analyzer/analyzer.h | 3 + gcc/analyzer/constraint-manager.cc | 2 +- gcc/analyzer/sm-taint.cc | 114 +++++++++++++++++- .../analyzer/taint-index-pr106229.c | 109 +++++++++++++++++ 4 files changed, 223 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/analyzer/taint-index-pr106229.c diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 8dec9649f2fb..23e3f71df0af 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -427,6 +427,9 @@ bit_offset_to_json (const bit_offset_t &offset); extern json::value * byte_offset_to_json (const byte_offset_t &offset); +extern tristate +compare_constants (tree lhs_const, enum tree_code op, tree rhs_const); + } // namespace ana extern bool is_special_named_call_p (const gcall *call, const char *funcname, diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc index 2db6c1734638..e8bcabeb0cd5 100644 --- a/gcc/analyzer/constraint-manager.cc +++ b/gcc/analyzer/constraint-manager.cc @@ -54,7 +54,7 @@ along with GCC; see the file COPYING3. If not see namespace ana { -static tristate +tristate compare_constants (tree lhs_const, enum tree_code op, tree rhs_const) { tree comparison diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc index 3f7e5cd55837..dc4b078c411f 100644 --- a/gcc/analyzer/sm-taint.cc +++ b/gcc/analyzer/sm-taint.cc @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3. If not see #include "digraph.h" #include "stringpool.h" #include "attribs.h" +#include "fold-const.h" #include "analyzer/supergraph.h" #include "analyzer/call-string.h" #include "analyzer/program-point.h" @@ -1369,6 +1370,104 @@ make_taint_state_machine (logger *logger) return new taint_state_machine (logger); } +/* A closed concrete range. */ + +class concrete_range +{ +public: + /* Return true iff THIS is fully within OTHER + i.e. + - m_min must be >= OTHER.m_min + - m_max must be <= OTHER.m_max. */ + bool within_p (const concrete_range &other) const + { + if (compare_constants (m_min, GE_EXPR, other.m_min).is_true ()) + if (compare_constants (m_max, LE_EXPR, other.m_max).is_true ()) + return true; + return false; + } + + tree m_min; + tree m_max; +}; + +/* Attempt to get a closed concrete range for SVAL based on types. + If found, write to *OUT and return true. + Otherwise return false. */ + +static bool +get_possible_range (const svalue *sval, concrete_range *out) +{ + if (const svalue *inner = sval->maybe_undo_cast ()) + { + concrete_range inner_range; + if (!get_possible_range (inner, &inner_range)) + return false; + + if (sval->get_type () + && inner->get_type () + && INTEGRAL_TYPE_P (sval->get_type ()) + && INTEGRAL_TYPE_P (inner->get_type ()) + && TYPE_UNSIGNED (inner->get_type ()) + && (TYPE_PRECISION (sval->get_type ()) + > TYPE_PRECISION (inner->get_type ()))) + { + /* We have a cast from an unsigned type to a wider integral type. + Assuming this is zero-extension, we can inherit the range from + the inner type. */ + enum tree_code op = ((const unaryop_svalue *)sval)->get_op (); + out->m_min = fold_unary (op, sval->get_type (), inner_range.m_min); + out->m_max = fold_unary (op, sval->get_type (), inner_range.m_max); + return true; + } + } + + if (sval->get_type () + && INTEGRAL_TYPE_P (sval->get_type ())) + { + out->m_min = TYPE_MIN_VALUE (sval->get_type ()); + out->m_max = TYPE_MAX_VALUE (sval->get_type ()); + return true; + } + + return false; +} + +/* Determine if it's possible for tainted array access ELEMENT_REG to + actually be a problem. + + Check here for index being from e.g. unsigned char when the array + contains >= 255 elements. + + Return true if out-of-bounds is possible, false if it's impossible + (for suppressing false positives). */ + +static bool +index_can_be_out_of_bounds_p (const element_region *element_reg) +{ + const svalue *index = element_reg->get_index (); + const region *array_reg = element_reg->get_parent_region (); + + if (array_reg->get_type () + && TREE_CODE (array_reg->get_type ()) == ARRAY_TYPE + && TYPE_DOMAIN (array_reg->get_type ()) + && INTEGRAL_TYPE_P (TYPE_DOMAIN (array_reg->get_type ()))) + { + concrete_range valid_index_range; + valid_index_range.m_min + = TYPE_MIN_VALUE (TYPE_DOMAIN (array_reg->get_type ())); + valid_index_range.m_max + = TYPE_MAX_VALUE (TYPE_DOMAIN (array_reg->get_type ())); + + concrete_range possible_index_range; + if (get_possible_range (index, &possible_index_range)) + if (possible_index_range.within_p (valid_index_range)) + return false; + } + + return true; +} + /* Complain to CTXT if accessing REG leads could lead to arbitrary memory access under an attacker's control (due to taint). */ @@ -1415,10 +1514,17 @@ region_model::check_region_for_taint (const region *reg, gcc_assert (state); enum bounds b; if (taint_sm.get_taint (state, index->get_type (), &b)) - { - tree arg = get_representative_tree (index); - ctxt->warn (make_unique (taint_sm, arg, b)); - } + { + if (index_can_be_out_of_bounds_p (element_reg)) + { + tree arg = get_representative_tree (index); + ctxt->warn (make_unique (taint_sm, + arg, b)); + } + else if (ctxt->get_logger ()) + ctxt->get_logger ()->log ("rejecting tainted_array_index as" + " out of bounds is not possible"); + } } break; diff --git a/gcc/testsuite/c-c++-common/analyzer/taint-index-pr106229.c b/gcc/testsuite/c-c++-common/analyzer/taint-index-pr106229.c new file mode 100644 index 000000000000..76dca630a038 --- /dev/null +++ b/gcc/testsuite/c-c++-common/analyzer/taint-index-pr106229.c @@ -0,0 +1,109 @@ +#include + +/* Attacker-controlled 8 bit values where the array isn't + necessarily big enough. We should warn about these. */ + +struct st_s8_field_255_elements +{ + int8_t idx; + char buf[255]; +}; + +char __attribute__((tainted_args)) +test_s8_field_255_elements (struct st_s8_field_255_elements s) +{ + return s.buf[s.idx]; /* { dg-warning "tainted-array-index" } */ +} + +struct st_u8_field_255_elements +{ + uint8_t idx; + char buf[255]; +}; + +char __attribute__((tainted_args)) +test_u8_field_255_elements (struct st_u8_field_255_elements s) +{ + return s.buf[s.idx]; /* { dg-warning "tainted-array-index" } */ +} + +/* Attacker-controlled 8 bit values where the array is + big enough, but where the value might be signed. */ + +struct st_s8_field_256_elements +{ + int8_t idx; + char buf[256]; +}; + +char __attribute__((tainted_args)) +test_s8_field_256_elements (struct st_s8_field_256_elements s) +{ + return s.buf[s.idx]; /* { dg-warning "tainted-array-index" } */ +} + +struct st_u8_field_256_elements +{ + uint8_t idx; + char buf[256]; +}; + +char __attribute__((tainted_args)) +test_u8_field_256_elements (struct st_u8_field_256_elements s) +{ + return s.buf[s.idx]; /* { dg-bogus "tainted-array-index" } */ +} + +/* Attacker-controlled 16 bit values where the array isn't + necessarily big enough. We should warn about these. */ + +struct st_s16_field_256_elements +{ + int16_t idx; + char buf[256]; +}; + +char __attribute__((tainted_args)) +test_s16_field_256_elements (struct st_s16_field_256_elements s) +{ + return s.buf[s.idx]; /* { dg-warning "tainted-array-index" } */ +} + +struct st_u16_field_256_elements +{ + uint16_t idx; + char buf[256]; +}; + +char __attribute__((tainted_args)) +test_u16_field_256_elements (struct st_u16_field_256_elements s) +{ + return s.buf[s.idx]; /* { dg-warning "tainted-array-index" } */ +} + +/* Attacker-controlled 16 bit values where the array is + big enough, but where the value might be signed. */ + +struct st_s16_field_65536_elements +{ + int16_t idx; + char buf[65536]; +}; + +char __attribute__((tainted_args)) +test_s16_field_65536_elements (struct st_s16_field_65536_elements s) +{ + return s.buf[s.idx]; /* { dg-warning "tainted-array-index" } */ +} + +struct st_u16_field_65536_elements +{ + uint16_t idx; + char buf[65536]; +}; + +char __attribute__((tainted_args)) +test_u16_field_65536_elements (struct st_u16_field_65536_elements s) +{ + return s.buf[s.idx]; /* { dg-bogus "tainted-array-index" } */ +} -- 2.26.3