Messages in this thread | | | Subject | Re: [PATCH 2/2] efi: Validate UEFI boot variables | From | Ben Hutchings <> | Date | Wed, 02 May 2012 04:55:06 +0100 |
| |
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. [unhandled content-type:application/pgp-signature] | |