From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1k0pU8-0004Db-5R for mharc-grub-devel@gnu.org; Wed, 29 Jul 2020 13:03:20 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41476) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k0pU6-0004AO-PQ for grub-devel@gnu.org; Wed, 29 Jul 2020 13:03:18 -0400 Received: from userp2130.oracle.com ([156.151.31.86]:55154) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k0pU3-0000rQ-RG for grub-devel@gnu.org; Wed, 29 Jul 2020 13:03:18 -0400 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 06TH2rpu116551; Wed, 29 Jul 2020 17:03:08 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : date : message-id : in-reply-to : references; s=corp-2020-01-29; bh=UuLbGrr90mnWUn+LzdiDtsfnjGDOHGkWk9eLbl2xnsQ=; b=SzeuhNaxORxhoOVxDyPu7mcNWwUOVftBRn2HgpTn1EjTNGkLFJGcqr8kerfOZgm1jZgE 1r5EeWKpYaKepYlxK0UWFag2ZU6JENn6r+kekv6jUxj+536TTmz1d8bORGdFhj7CXM+W ZI8IYb/LnYpjY2oLwflAwXiVsnHI7R4LwN3MOLBZImyfU79ks5L0co1BHzT5lezBBBvE lufG8E78Xk4nabXAZ2UMiNhovzm2OUCnPIYfmtdiJLvrkvmNXrld6WTq4rE9p/5XzVzh Z5hkzsu4ULX1IpsCM578XjaovKB9a9a7eJbC1vz9vucYqk1qwo9BoW9Y//a8jMPnrUoJ Yg== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2130.oracle.com with ESMTP id 32hu1jevjs-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 29 Jul 2020 17:03:08 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 06TGvPsV035453; Wed, 29 Jul 2020 17:03:08 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by userp3030.oracle.com with ESMTP id 32hu5v5jpw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 29 Jul 2020 17:03:08 +0000 Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 06TH36vI016449; Wed, 29 Jul 2020 17:03:06 GMT Received: from tomti.i.net-space.pl (/10.175.200.191) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 29 Jul 2020 10:03:05 -0700 From: Daniel Kiper To: grub-devel@gnu.org Cc: 93sam@debian.org, alexander.burmashev@oracle.com, amakhalov@vmware.com, chris.coulson@canonical.com, cjwatson@debian.org, cperry@redhat.com, darren.kenny@oracle.com, darren.moffat@oracle.com, dave.miner@oracle.com, degranit@microsoft.com, eric.snowberg@oracle.com, ilya.okomin@oracle.com, jan.setjeeilers@oracle.com, jerecox@microsoft.com, jesse@eclypsium.com, john.haxby@oracle.com, kanth.ghatraju@oracle.com, konrad.wilk@oracle.com, mbenatto@redhat.com, mickey@eclypsium.com, msrc57813grub@microsoft.com, phcoder@gmail.com, pjones@redhat.com, sajacobu@microsoft.com, todd.vierling@oracle.com, xnox@ubuntu.com Subject: [SECURITY PATCH 24/28] efi: Fix some malformed device path arithmetic errors Date: Wed, 29 Jul 2020 19:00:37 +0200 Message-Id: <20200729170041.14082-25-daniel.kiper@oracle.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20200729170041.14082-1-daniel.kiper@oracle.com> References: <20200729170041.14082-1-daniel.kiper@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9697 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999 mlxscore=0 suspectscore=1 bulkscore=0 malwarescore=0 spamscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2007290115 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9697 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 clxscore=1015 malwarescore=0 spamscore=0 suspectscore=1 bulkscore=0 priorityscore=1501 phishscore=0 mlxlogscore=999 lowpriorityscore=0 impostorscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2007290116 Received-SPF: pass client-ip=156.151.31.86; envelope-from=daniel.kiper@oracle.com; helo=userp2130.oracle.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/07/29 13:01:45 X-ACL-Warn: Detected OS = Linux 3.1-3.10 [fuzzy] X-Spam_score_int: -63 X-Spam_score: -6.4 X-Spam_bar: ------ X-Spam_report: (-6.4 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-1, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jul 2020 17:03:19 -0000 From: Peter Jones Several places we take the length of a device path and subtract 4 from it, without ever checking that it's >= 4. There are also cases where this kind of malformation will result in unpredictable iteration, including treating the length from one dp node as the type in the next node. These are all errors, no matter where the data comes from. This patch adds a checking macro, GRUB_EFI_DEVICE_PATH_VALID(), which can be used in several places, and makes GRUB_EFI_NEXT_DEVICE_PATH() return NULL and GRUB_EFI_END_ENTIRE_DEVICE_PATH() evaluate as true when the length is too small. Additionally, it makes several places in the code check for and return errors in these cases. Signed-off-by: Peter Jones Reviewed-by: Daniel Kiper --- grub-core/kern/efi/efi.c | 64 ++++++++++++++++++++++++++++++++------ grub-core/loader/efi/chainloader.c | 13 ++++++-- grub-core/loader/i386/xnu.c | 9 +++--- include/grub/efi/api.h | 14 ++++++--- 4 files changed, 79 insertions(+), 21 deletions(-) diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c index b2d07a3fa..535e29424 100644 --- a/grub-core/kern/efi/efi.c +++ b/grub-core/kern/efi/efi.c @@ -342,7 +342,7 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0) dp = dp0; - while (1) + while (dp) { grub_efi_uint8_t type = GRUB_EFI_DEVICE_PATH_TYPE (dp); grub_efi_uint8_t subtype = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp); @@ -352,9 +352,15 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0) if (type == GRUB_EFI_MEDIA_DEVICE_PATH_TYPE && subtype == GRUB_EFI_FILE_PATH_DEVICE_PATH_SUBTYPE) { - grub_efi_uint16_t len; - len = ((GRUB_EFI_DEVICE_PATH_LENGTH (dp) - 4) - / sizeof (grub_efi_char16_t)); + grub_efi_uint16_t len = GRUB_EFI_DEVICE_PATH_LENGTH (dp); + + if (len < 4) + { + grub_error (GRUB_ERR_OUT_OF_RANGE, + "malformed EFI Device Path node has length=%d", len); + return NULL; + } + len = (len - 4) / sizeof (grub_efi_char16_t); filesize += GRUB_MAX_UTF8_PER_UTF16 * len + 2; } @@ -370,7 +376,7 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0) if (!name) return NULL; - while (1) + while (dp) { grub_efi_uint8_t type = GRUB_EFI_DEVICE_PATH_TYPE (dp); grub_efi_uint8_t subtype = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp); @@ -386,8 +392,15 @@ grub_efi_get_filename (grub_efi_device_path_t *dp0) *p++ = '/'; - len = ((GRUB_EFI_DEVICE_PATH_LENGTH (dp) - 4) - / sizeof (grub_efi_char16_t)); + len = GRUB_EFI_DEVICE_PATH_LENGTH (dp); + if (len < 4) + { + grub_error (GRUB_ERR_OUT_OF_RANGE, + "malformed EFI Device Path node has length=%d", len); + return NULL; + } + + len = (len - 4) / sizeof (grub_efi_char16_t); fp = (grub_efi_file_path_device_path_t *) dp; /* According to EFI spec Path Name is NULL terminated */ while (len > 0 && fp->path_name[len - 1] == 0) @@ -462,7 +475,26 @@ grub_efi_duplicate_device_path (const grub_efi_device_path_t *dp) ; p = GRUB_EFI_NEXT_DEVICE_PATH (p)) { - total_size += GRUB_EFI_DEVICE_PATH_LENGTH (p); + grub_size_t len = GRUB_EFI_DEVICE_PATH_LENGTH (p); + + /* + * In the event that we find a node that's completely garbage, for + * example if we get to 0x7f 0x01 0x02 0x00 ... (EndInstance with a size + * of 2), GRUB_EFI_END_ENTIRE_DEVICE_PATH() will be true and + * GRUB_EFI_NEXT_DEVICE_PATH() will return NULL, so we won't continue, + * and neither should our consumers, but there won't be any error raised + * even though the device path is junk. + * + * This keeps us from passing junk down back to our caller. + */ + if (len < 4) + { + grub_error (GRUB_ERR_OUT_OF_RANGE, + "malformed EFI Device Path node has length=%d", len); + return NULL; + } + + total_size += len; if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (p)) break; } @@ -507,7 +539,7 @@ dump_vendor_path (const char *type, grub_efi_vendor_device_path_t *vendor) void grub_efi_print_device_path (grub_efi_device_path_t *dp) { - while (1) + while (GRUB_EFI_DEVICE_PATH_VALID (dp)) { grub_efi_uint8_t type = GRUB_EFI_DEVICE_PATH_TYPE (dp); grub_efi_uint8_t subtype = GRUB_EFI_DEVICE_PATH_SUBTYPE (dp); @@ -919,7 +951,10 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1, /* Return non-zero. */ return 1; - while (1) + if (dp1 == dp2) + return 0; + + while (GRUB_EFI_DEVICE_PATH_VALID (dp1) && GRUB_EFI_DEVICE_PATH_VALID (dp2)) { grub_efi_uint8_t type1, type2; grub_efi_uint8_t subtype1, subtype2; @@ -955,5 +990,14 @@ grub_efi_compare_device_paths (const grub_efi_device_path_t *dp1, dp2 = (grub_efi_device_path_t *) ((char *) dp2 + len2); } + /* + * There's no "right" answer here, but we probably don't want to call a valid + * dp and an invalid dp equal, so pick one way or the other. + */ + if (GRUB_EFI_DEVICE_PATH_VALID (dp1) && !GRUB_EFI_DEVICE_PATH_VALID (dp2)) + return 1; + else if (!GRUB_EFI_DEVICE_PATH_VALID (dp1) && GRUB_EFI_DEVICE_PATH_VALID (dp2)) + return -1; + return 0; } diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c index daf8c6b54..a8d7b9155 100644 --- a/grub-core/loader/efi/chainloader.c +++ b/grub-core/loader/efi/chainloader.c @@ -156,9 +156,18 @@ make_file_path (grub_efi_device_path_t *dp, const char *filename) size = 0; d = dp; - while (1) + while (d) { - size += GRUB_EFI_DEVICE_PATH_LENGTH (d); + grub_size_t len = GRUB_EFI_DEVICE_PATH_LENGTH (d); + + if (len < 4) + { + grub_error (GRUB_ERR_OUT_OF_RANGE, + "malformed EFI Device Path node has length=%d", len); + return NULL; + } + + size += len; if ((GRUB_EFI_END_ENTIRE_DEVICE_PATH (d))) break; d = GRUB_EFI_NEXT_DEVICE_PATH (d); diff --git a/grub-core/loader/i386/xnu.c b/grub-core/loader/i386/xnu.c index e9e119259..a70093607 100644 --- a/grub-core/loader/i386/xnu.c +++ b/grub-core/loader/i386/xnu.c @@ -515,14 +515,15 @@ grub_cmd_devprop_load (grub_command_t cmd __attribute__ ((unused)), devhead = buf; buf = devhead + 1; - dpstart = buf; + dp = dpstart = buf; - do + while (GRUB_EFI_DEVICE_PATH_VALID (dp) && buf < bufend) { - dp = buf; buf = (char *) buf + GRUB_EFI_DEVICE_PATH_LENGTH (dp); + if (GRUB_EFI_END_ENTIRE_DEVICE_PATH (dp)) + break; + dp = buf; } - while (!GRUB_EFI_END_ENTIRE_DEVICE_PATH (dp) && buf < bufend); dev = grub_xnu_devprop_add_device (dpstart, (char *) buf - (char *) dpstart); diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h index 937058d68..1dcaa12f5 100644 --- a/include/grub/efi/api.h +++ b/include/grub/efi/api.h @@ -630,6 +630,7 @@ typedef struct grub_efi_device_path grub_efi_device_path_protocol_t; #define GRUB_EFI_DEVICE_PATH_TYPE(dp) ((dp)->type & 0x7f) #define GRUB_EFI_DEVICE_PATH_SUBTYPE(dp) ((dp)->subtype) #define GRUB_EFI_DEVICE_PATH_LENGTH(dp) ((dp)->length) +#define GRUB_EFI_DEVICE_PATH_VALID(dp) ((dp) != NULL && GRUB_EFI_DEVICE_PATH_LENGTH (dp) >= 4) /* The End of Device Path nodes. */ #define GRUB_EFI_END_DEVICE_PATH_TYPE (0xff & 0x7f) @@ -638,13 +639,16 @@ typedef struct grub_efi_device_path grub_efi_device_path_protocol_t; #define GRUB_EFI_END_THIS_DEVICE_PATH_SUBTYPE 0x01 #define GRUB_EFI_END_ENTIRE_DEVICE_PATH(dp) \ - (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_END_DEVICE_PATH_TYPE \ - && (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp) \ - == GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)) + (!GRUB_EFI_DEVICE_PATH_VALID (dp) || \ + (GRUB_EFI_DEVICE_PATH_TYPE (dp) == GRUB_EFI_END_DEVICE_PATH_TYPE \ + && (GRUB_EFI_DEVICE_PATH_SUBTYPE (dp) \ + == GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE))) #define GRUB_EFI_NEXT_DEVICE_PATH(dp) \ - ((grub_efi_device_path_t *) ((char *) (dp) \ - + GRUB_EFI_DEVICE_PATH_LENGTH (dp))) + (GRUB_EFI_DEVICE_PATH_VALID (dp) \ + ? ((grub_efi_device_path_t *) \ + ((char *) (dp) + GRUB_EFI_DEVICE_PATH_LENGTH (dp))) \ + : NULL) /* Hardware Device Path. */ #define GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE 1 -- 2.11.0