Re: [PATCH 2/2] efi: Validate UEFI boot variables

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 3055 bytes --]

On Mon, 2012-04-30 at 16:11 -0400, Matthew Garrett wrote:
> A common flaw in UEFI systems is a refusal to POST triggered by a malformed
> boot variable. Once in this state, machines may only be restored by
> reflashing their firmware with an external hardware device. While this is
> obviously a firmware bug, the serious nature of the outcome suggests that
> operating systems should filter their variable writes in order to prevent
> a malicious user from rendering the machine unusable.
[...]
> +static bool
> +validate_device_path(struct efi_variable *var, int match, u8 *buffer,
> int len)
> +{
> +       struct efi_generic_dev_path *node;
> +       int offset = 0;
> +
> +       node = (struct efi_generic_dev_path *)buffer;
> +
> +       while (offset < len) {
> +               offset += node->length;
> +
> +               if (offset > len)
> +                       return false;
> +
> +               if ((node->type == EFI_DEV_END_PATH ||
> +                    node->type == EFI_DEV_END_PATH2) &&
> +                   node->sub_type == EFI_DEV_END_ENTIRE)
> +                       return true;
> +
> +               node = (struct efi_generic_dev_path *)(buffer + offset);
> +       }

This validation is crap; you're not accounting for the size of the node
or invalid lengths.  Try:

	while (offset <= len - sizeof(*node) &&
	       node->length >= sizeof(*node) &&
	       node->length <= len - offset) {
		offset += node->length;

		if ((node->type == EFI_DEV_END_PATH ||
		     node->type == EFI_DEV_END_PATH2) &&
		    node->sub_type == EFI_DEV_END_ENTIRE)
			return true;

		node = (struct efi_generic_dev_path *)(buffer + offset);
	}

[...]
> +static bool
> +validate_load_option(struct efi_variable *var, int match, u8 *buffer, int len)
> +{
> +	u16 filepathlength;
> +	int i, desclength = 0;
> +
> +	/* Either "Boot" or "Driver" followed by four digits of hex */
> +	for (i = match; i < match+4; i++) {
> +		if (hex_to_bin(var->VariableName[i] & 0xff) < 0)
> +			return true;
> +	}

Isn't this slightly too restrictive?  The '& 0xff' results in many
non-ASCII characters aliasing hex digits and potentially causes a
variable to be validated as if it was special.  I would think the
correct condition is:

		if (var->VariableName[i] > 127 ||
		    hex_to_bin(var->VariableName[i]) < 0)

Presumably the variable should also be ignored if there are any more
characters after the 4 hex digits?

> +	/* A valid entry must be at least 6 bytes */
> +	if (len < 6)
> +		return false;

Surely 8 bytes - otherwise you don't even have space for the
description's null terminator.

> +	filepathlength = buffer[4] | buffer[5] << 8;
> +
> +	/*
> +	 * There's no stored length for the description, so it has to be
> +	 * found by hand
> +	 */
> +	desclength = utf16_strsize((efi_char16_t *)(buffer + 6), len) + 2;
[...]

Second argument should be len - 6.

Ben.

-- 
Ben Hutchings
Design a system any fool can use, and only a fool will want to use it.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]