From patchwork Tue Jan 16 00:05:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 188352 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2614:b0:101:6a76:bbe3 with SMTP id mm20csp2011167dyc; Mon, 15 Jan 2024 16:06:50 -0800 (PST) X-Google-Smtp-Source: AGHT+IEvw4A1GrYAbRROWU5C36krrUUt6LiSy6FEebZSjeZ1EB3QzpHz24MI4lxkvqnWRGUKluvT X-Received: by 2002:a54:4e96:0:b0:3bd:6272:1a06 with SMTP id c22-20020a544e96000000b003bd62721a06mr5600135oiy.20.1705363610489; Mon, 15 Jan 2024 16:06:50 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1705363610; cv=pass; d=google.com; s=arc-20160816; b=Ngd+ArYKBmAQw7u6CMehHx6t+P75768lKslenybrkmYxAErrqgn8tgCsZQa+2xuOVR hhQejkPw0vvbeQ1wE4qBgkNmGfXxlFsALPf+RvhCVCWYAvGRN7NDM9yB8+/aqqblF/pT j4t3yiZRRyTVkM4KK2WzsZo0p5ZIvLxwMpx3Sibd/w5EemdyYQ+2a5PoewFAV3crD0Yx uoVAdlw4N/USt1xBfsddr7ly4ftP8vxE2IlrqF9su82Ew/P70XHCmmJZY/X9Y7gJgAXN TDdz68ksEwHBZUJiVITNPBhJ5fE6l+aFT4ajtc7nOOTI7PVejkHVPCjV1K2WrlZXgoA3 u4lA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=errors-to:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:content-transfer-encoding :mime-version:message-id:date:subject:cc:to:from:dkim-signature :arc-filter:dmarc-filter:delivered-to; bh=3Q3wRLmxRfWuGBAS2k5oONZ7RZDbAv6TprttluXYh8k=; fh=NXemEfxTRbZtBxUkxR2ehQUaYlcDfMdzPkO8MChVQE4=; b=AY+3HT8U8HxH4BPAfwNkbmgPBng09l9pOnc6XC8O1PXsdPyByNtI5EiXfnkrk/1ljG 9Fiyi2ty88HGrXh4Uac7HbuZagnsWn6muajWmgiy5UrvjahnB3tby4GHiqnnsjIRjBKv iqoys7GhYDkuko/G10E2BINSFxi8r36kKeQ1T0DTOJiiXyvETkqq00m6yYaS0us/uNag XNbx4K0htFtHSC1UGDGluXiN6cZQVwi+p9XYPsfwDVq9xo64glIsDKChr3sJAH7C7Y2c 9Lhg+5Tsd5bgBhcuOvbsXFUKZKoouQPFn/SxkbVUrXvuO5Yud7JPb4AlbxXJ+UjIpfP5 bveA== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=H3k9YJ3C; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from server2.sourceware.org (server2.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id ow4-20020a05620a820400b007833d03747esi8426472qkn.591.2024.01.15.16.06.50 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 Jan 2024 16:06:50 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=H3k9YJ3C; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2BF853858C53 for ; Tue, 16 Jan 2024 00:06:50 +0000 (GMT) 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 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 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788203353272027664 X-GMAIL-MSGID: 1788203353272027664 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" } */ +}