lkml.org 
[lkml]   [2012]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] efi: Validate UEFI boot variables
    From
    Date
    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]
    \
     
     \ /
      Last update: 2012-05-02 06:21    [W:0.028 / U:30.144 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site