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

@@ -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);

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;

plugins/uefi/fu-uefi-devpath.c

@@ -0,0 +1,128 @@
+/*
+ * Copyright (C) 2018-2019 Richard Hughes <richard@hughsie.com>
+ *
+ * SPDX-License-Identifier: LGPL-2.1+
+ */
+
+#include "config.h"
+
+#include <efivar.h>
+
+#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);
+}

plugins/uefi/fu-uefi-devpath.h

@@ -0,0 +1,28 @@
+/*
+ * Copyright (C) 2018-2019 Richard Hughes <richard@hughsie.com>
+ *
+ * SPDX-License-Identifier: LGPL-2.1+
+ */
+
+#pragma once
+
+#include <glib.h>
+
+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

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;
+	GBytes *dp_data;
-	guint16 ucs2sz = 0;
+	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 */
+	/* get all headers */
-	idp = (const_efidp) buf;
+	dps = fu_uefi_devpath_parse (buf, sz, FU_UEFI_DEVPATH_PARSE_FLAG_REPAIR, error);
-	while (1) {
+	if (dps == NULL)
-		if (efidp_type (idp) == EFIDP_END_TYPE &&
+		return NULL;
-		    efidp_subtype (idp) == EFIDP_END_ENTIRE)
+	dp_data = fu_uefi_devpath_find_data (dps,
-			break;
+					     EFIDP_MEDIA_TYPE,
-		if (efidp_type(idp) != EFIDP_MEDIA_TYPE ||
+					     EFIDP_MEDIA_FILE,
-		    efidp_subtype (idp) != EFIDP_MEDIA_FILE) {
+					     error);
-			if (efidp_next_node (idp, &idp) < 0)
+	if (dp_data == NULL)
-				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)
 		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;
 }

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',