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.
signature.asc
Description: This is a digitally signed message part
