Messages in this thread | | | Date | Wed, 2 May 2012 15:54:12 +0100 | From | Matthew Garrett <> | Subject | Re: [PATCH 2/2] efi: Validate UEFI boot variables |
| |
On Wed, May 02, 2012 at 04:55:06AM +0100, Ben Hutchings wrote:
> This validation is crap; you're not accounting for the size of the node > or invalid lengths. Try:
Good catch.
> 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:
Mm. Yeah, I guess that should probably be permitted.
> 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?
The spec doesn't permit any such names, so I don't think it makes any difference in practice. But in theory, we should probably either explicitly permit or reject them rather than treating them as if they're boot variables.
> > + /* 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.
Yes, I guess that could trip things up.
> > + 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.
Sure.
-- Matthew Garrett | mjg59@srcf.ucam.org
| |