From 26d3da40748377d96ad7ceb9016f9d65d28c19a2 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Thu, 2 May 2019 09:58:06 +0100 Subject: [PATCH] uefi: Fix a libasan failure when reading a UEFI variable It seems some older versions of libefi var incorrectly build the 'length' value in the DP returned from efi_generate_file_device_path(). This means we copy past the end of the allocated buffer when parsing the efi_update_info_t structure. This bug seems fixed in efivar git master, and this fix is only going to help people with older efivar versions. It's probably a good thing to be a bit more paranoid about EFI variable data anyway. DEBUG: UpdateInfo: ? 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f ???????????????????????????????????????????????????????????????????????????????????????????????????????? 0x0000 ? 07 00 00 00 20 d9 7b 69 cf 12 a9 4d 83 85 99 69 09 bc 65 59 00 00 05 00 00 00 00 00 00 00 00 00 0x0020 ? 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00 00 00 04 01 2a 00 01 00 00 00 00 08 00 00 0x0040 ? 00 00 00 00 00 40 06 00 00 00 00 00 5a aa 97 5a 10 d5 7e 49 99 0b ca 8d 35 4d c8 6d 02 02 04 04 0x0060 ? 86 00 5c 00 45 00 46 00 49 00 5c 00 66 00 65 00 64 00 6f 00 72 00 61 00 5c 00 66 00 77 00 5c 00 0x0080 ? 66 00 77 00 75 00 70 00 64 00 2d 00 36 00 39 00 37 00 62 00 64 00 39 00 32 00 30 00 2d 00 31 00 0x00a0 ? 32 00 63 00 66 00 2d 00 34 00 64 00 61 00 39 00 2d 00 38 00 33 00 38 00 35 00 2d 00 39 00 39 00 0x00c0 ? 36 00 39 00 30 00 39 00 62 00 63 00 36 00 35 00 35 00 39 00 2e 00 63 00 61 00 70 00 00 00 7f ff 0x00e0 ? 04 00 DEBUG: DP type:0x04 subtype:0x01 size:0x002a DEBUG: DP type:0x04 subtype:0x04 size:0x0086 DEBUG: found END_ENTIRE at 0x00aa DEBUG: DP length invalid! Truncating from 0x0086 to 0x0080 DEBUG: DP type:0x7f subtype:0xff size:0x0004 --- plugins/uefi/fu-self-test.c | 1 + plugins/uefi/fu-uefi-device.c | 10 +++ plugins/uefi/fu-uefi-devpath.c | 128 +++++++++++++++++++++++++++++ plugins/uefi/fu-uefi-devpath.h | 28 +++++++ plugins/uefi/fu-uefi-update-info.c | 55 +++++++------ plugins/uefi/meson.build | 3 + 6 files changed, 199 insertions(+), 26 deletions(-) create mode 100644 plugins/uefi/fu-uefi-devpath.c create mode 100644 plugins/uefi/fu-uefi-devpath.h diff --git a/plugins/uefi/fu-self-test.c b/plugins/uefi/fu-self-test.c index da71326a9..e3a561f1f 100644 --- a/plugins/uefi/fu-self-test.c +++ b/plugins/uefi/fu-self-test.c @@ -319,6 +319,7 @@ main (int argc, char **argv) /* only critical and error are fatal */ g_log_set_fatal_mask (NULL, G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL); + g_setenv ("G_MESSAGES_DEBUG", "all", TRUE); /* tests go here */ g_test_add_func ("/uefi/pcrs1.2", fu_uefi_pcrs_1_2_func); diff --git a/plugins/uefi/fu-uefi-device.c b/plugins/uefi/fu-uefi-device.c index dc4964a5c..63700aaac 100644 --- a/plugins/uefi/fu-uefi-device.c +++ b/plugins/uefi/fu-uefi-device.c @@ -15,6 +15,7 @@ #include "fu-uefi-common.h" #include "fu-uefi-device.h" +#include "fu-uefi-devpath.h" #include "fu-uefi-bootmgr.h" #include "fu-uefi-pcrs.h" #include "fu-uefi-vars.h" @@ -237,6 +238,7 @@ fu_uefi_device_build_dp_buf (const gchar *path, gsize *bufsz, GError **error) gssize req; gssize sz; g_autofree guint8 *dp_buf = NULL; + g_autoptr(GPtrArray) dps = NULL; /* get the size of the path first */ req = efi_generate_file_device_path (NULL, 0, path, @@ -275,6 +277,14 @@ fu_uefi_device_build_dp_buf (const gchar *path, gsize *bufsz, GError **error) return NULL; } + /* parse what we got back from efivar */ + dps = fu_uefi_devpath_parse (dp_buf, (gsize) sz, + FU_UEFI_DEVPATH_PARSE_FLAG_NONE, error); + if (dps == NULL) { + fu_common_dump_raw (G_LOG_DOMAIN, "dp_buf", dp_buf, (gsize) sz); + return NULL; + } + /* success */ if (bufsz != NULL) *bufsz = sz; diff --git a/plugins/uefi/fu-uefi-devpath.c b/plugins/uefi/fu-uefi-devpath.c new file mode 100644 index 000000000..a5a9ebc0a --- /dev/null +++ b/plugins/uefi/fu-uefi-devpath.c @@ -0,0 +1,128 @@ +/* + * Copyright (C) 2018-2019 Richard Hughes + * + * SPDX-License-Identifier: LGPL-2.1+ + */ + +#include "config.h" + +#include + +#include "fu-common.h" +#include "fu-uefi-devpath.h" + +#include "fwupd-error.h" + +typedef struct { + guint8 type; + guint8 subtype; + GBytes *data; +} FuUefiDevPath; + +static void +fu_uefi_efi_dp_free (FuUefiDevPath *dp) +{ + if (dp->data != NULL) + g_bytes_unref (dp->data); + g_free (dp); +} + +GBytes * +fu_uefi_devpath_find_data (GPtrArray *dps, guint8 type, guint8 subtype, GError **error) +{ + for (guint i = 0; i < dps->len; i++) { + FuUefiDevPath *dp = g_ptr_array_index (dps, i); + if (dp->type == type && dp->subtype == subtype) + return dp->data; + } + g_set_error (error, + FWUPD_ERROR, + FWUPD_ERROR_INTERNAL, + "no DP with type 0x%02x and subtype 0x%02x", + type, subtype); + return NULL; +} + +GPtrArray * +fu_uefi_devpath_parse (const guint8 *buf, gsize sz, + FuUefiDevpathParseFlags flags, GError **error) +{ + guint16 offset = 0; + g_autoptr(GPtrArray) dps = NULL; + + /* sanity check */ + if (sz < sizeof(efidp_header)) { + g_set_error_literal (error, + FWUPD_ERROR, + FWUPD_ERROR_INTERNAL, + "const_efidp is corrupt"); + return NULL; + } + + dps = g_ptr_array_new_with_free_func ((GDestroyNotify) fu_uefi_efi_dp_free); + while (1) { + FuUefiDevPath *dp; + const efidp_header *hdr = (efidp_header *) (buf + offset); + guint16 hdr_length = GUINT16_FROM_LE(hdr->length); + + /* check if last entry */ + g_debug ("DP type:0x%02x subtype:0x%02x size:0x%04x", + hdr->type, hdr->subtype, hdr->length); + if (hdr->type == EFIDP_END_TYPE && hdr->subtype == EFIDP_END_ENTIRE) + break; + + /* work around a bug in efi_va_generate_file_device_path_from_esp */ + if (offset + sizeof(efidp_header) + hdr->length > sz) { + hdr_length = 0; + fu_common_dump_full (G_LOG_DOMAIN, "efidp", + buf + offset, sz - offset, 32, + FU_DUMP_FLAGS_SHOW_ADDRESSES); + for (guint16 i = offset + 4; i <= sz - 4; i++) { + if (memcmp (buf + i, "\x7f\xff\x04\x00", 4) == 0) { + hdr_length = i - offset; + g_debug ("found END_ENTIRE at 0x%04x", + (guint) (i - offset)); + break; + } + } + if (hdr_length == 0) { + g_set_error_literal (error, + FWUPD_ERROR, + FWUPD_ERROR_INTERNAL, + "DP length invalid and no END_ENTIRE " + "found, possibly data truncation?"); + return NULL; + } + if ((flags & FU_UEFI_DEVPATH_PARSE_FLAG_REPAIR) == 0) { + g_set_error (error, + FWUPD_ERROR, + FWUPD_ERROR_INTERNAL, + "DP length invalid, reported 0x%04x, maybe 0x%04x", + hdr->length, hdr_length); + return NULL; + } + g_debug ("DP length invalid! Truncating from 0x%04x to 0x%04x", + hdr->length, hdr_length); + } + + /* add new DP */ + dp = g_new0 (FuUefiDevPath, 1); + dp->type = hdr->type; + dp->subtype = hdr->subtype; + if (hdr_length > 0) + dp->data = g_bytes_new (buf + offset + 4, hdr_length); + g_ptr_array_add (dps, dp); + + /* advance to next DP */ + offset += hdr_length; + if (offset + sizeof(efidp_header) > sz) { + g_set_error_literal (error, + FWUPD_ERROR, + FWUPD_ERROR_INTERNAL, + "DP length invalid after fixing"); + return NULL; + } + + } + return g_steal_pointer (&dps); +} diff --git a/plugins/uefi/fu-uefi-devpath.h b/plugins/uefi/fu-uefi-devpath.h new file mode 100644 index 000000000..e160d731d --- /dev/null +++ b/plugins/uefi/fu-uefi-devpath.h @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2018-2019 Richard Hughes + * + * SPDX-License-Identifier: LGPL-2.1+ + */ + +#pragma once + +#include + +G_BEGIN_DECLS + +typedef enum { + FU_UEFI_DEVPATH_PARSE_FLAG_NONE = 0, + FU_UEFI_DEVPATH_PARSE_FLAG_REPAIR = 1 << 0, + FU_UEFI_DEVPATH_PARSE_FLAG_LAST +} FuUefiDevpathParseFlags; + +GPtrArray *fu_uefi_devpath_parse (const guint8 *buf, + gsize sz, + FuUefiDevpathParseFlags flags, + GError **error); +GBytes *fu_uefi_devpath_find_data (GPtrArray *dps, + guint8 type, + guint8 subtype, + GError **error); + +G_END_DECLS diff --git a/plugins/uefi/fu-uefi-update-info.c b/plugins/uefi/fu-uefi-update-info.c index 7be701169..9454844ce 100644 --- a/plugins/uefi/fu-uefi-update-info.c +++ b/plugins/uefi/fu-uefi-update-info.c @@ -7,6 +7,7 @@ #include "config.h" #include "fu-common.h" +#include "fu-uefi-devpath.h" #include "fu-uefi-update-info.h" #include "fu-uefi-common.h" #include "fu-ucs2.h" @@ -36,42 +37,41 @@ fu_uefi_update_info_status_to_string (FuUefiUpdateInfoStatus status) } static gchar * -fu_uefi_update_info_parse_dp (const guint8 *buf, gsize sz) +fu_uefi_update_info_parse_dp (const guint8 *buf, gsize sz, GError **error) { - const_efidp idp; - guint16 ucs2sz = 0; + GBytes *dp_data; + const gchar *data; + gsize ucs2sz = 0; g_autofree gchar *relpath = NULL; g_autofree guint16 *ucs2file = NULL; + g_autoptr(GPtrArray) dps = NULL; g_return_val_if_fail (buf != NULL, NULL); g_return_val_if_fail (sz != 0, NULL); - /* find UCS2 string */ - idp = (const_efidp) buf; - while (1) { - if (efidp_type (idp) == EFIDP_END_TYPE && - efidp_subtype (idp) == EFIDP_END_ENTIRE) - break; - if (efidp_type(idp) != EFIDP_MEDIA_TYPE || - efidp_subtype (idp) != EFIDP_MEDIA_FILE) { - if (efidp_next_node (idp, &idp) < 0) - break; - continue; - } - ucs2sz = efidp_node_size (idp) - 4; - ucs2file = g_new0 (guint16, (ucs2sz / 2) + 1); - memcpy (ucs2file, (guint8 *)idp + 4, ucs2sz); - break; - } - - /* nothing found */ - if (ucs2file == NULL || ucs2sz == 0) + /* get all headers */ + dps = fu_uefi_devpath_parse (buf, sz, FU_UEFI_DEVPATH_PARSE_FLAG_REPAIR, error); + if (dps == NULL) + return NULL; + dp_data = fu_uefi_devpath_find_data (dps, + EFIDP_MEDIA_TYPE, + EFIDP_MEDIA_FILE, + error); + if (dp_data == NULL) return NULL; - /* convert to something sane */ + /* convert to UTF-8 */ + data = g_bytes_get_data (dp_data, &ucs2sz); + ucs2file = g_new0 (guint16, (ucs2sz / 2) + 1); + memcpy (ucs2file, data, ucs2sz); relpath = fu_ucs2_to_uft8 (ucs2file, ucs2sz / sizeof (guint16)); - if (relpath == NULL) + if (relpath == NULL) { + g_set_error_literal (error, + FWUPD_ERROR, + FWUPD_ERROR_INTERNAL, + "cannot convert to UTF-8"); return NULL; + } g_strdelimit (relpath, "\\", '/'); return g_steal_pointer (&relpath); } @@ -106,7 +106,10 @@ fu_uefi_update_info_parse (FuUefiUpdateInfo *self, const guint8 *buf, gsize sz, } if (sz > sizeof(efi_update_info_t)) { self->capsule_fn = fu_uefi_update_info_parse_dp (buf + sizeof(efi_update_info_t), - sz - sizeof(efi_update_info_t)); + sz - sizeof(efi_update_info_t), + error); + if (self->capsule_fn == NULL) + return FALSE; } return TRUE; } diff --git a/plugins/uefi/meson.build b/plugins/uefi/meson.build index ac9f5dd83..b78e3cfc1 100644 --- a/plugins/uefi/meson.build +++ b/plugins/uefi/meson.build @@ -20,6 +20,7 @@ shared_module('fu_plugin_uefi', 'fu-uefi-bootmgr.c', 'fu-uefi-common.c', 'fu-uefi-device.c', + 'fu-uefi-devpath.c', 'fu-uefi-pcrs.c', 'fu-uefi-update-info.c', 'fu-uefi-vars.c', @@ -53,6 +54,7 @@ executable( 'fu-uefi-bootmgr.c', 'fu-uefi-common.c', 'fu-uefi-device.c', + 'fu-uefi-devpath.c', 'fu-uefi-pcrs.c', 'fu-uefi-update-info.c', 'fu-uefi-vars.c', @@ -94,6 +96,7 @@ if get_option('tests') 'fu-uefi-bootmgr.c', 'fu-uefi-common.c', 'fu-uefi-device.c', + 'fu-uefi-devpath.c', 'fu-uefi-pcrs.c', 'fu-uefi-update-info.c', 'fu-uefi-vars.c',