Am 10. Februar 2021 01:38:38 MEZ schrieb AKASHI Takahiro
<takahiro.aka...@linaro.org>:
>On Tue, Feb 09, 2021 at 09:37:42PM +0100, Heinrich Schuchardt wrote:
>> fix get_last_capsule() leads to writes beyond the stack allocated
>buffer.
>> This was indicated when enabling the stack protector.
>>
>> utf16_utf8_strcpy() only stops copying when reaching '\0'. The
>current
>> invocation always writes beyond the end of value[].
>>
>> The output length of utf16_utf8_strcpy() may be longer than the
>number of
>> UTF-16 tokens. E.g has "CapsuleКиев" has 11 UTF-16 tokens but 15
>UTF-8
>
>First of all, "CapsuleLast" is a read-only variable from user's
>viewpoint,
>and is maintained solely by efi code. Then its value is expected to
>always
>be sane.
>The case you suggested above is quite unlikely.
What happens if the user tries to create a variable of the same name, e.g. in
the variable store on disk?
>
>Although I don't think we need this patch,
Why do you think the patch is not necessary given that the current code is
writing ouside buffers?
>
>> tokens. Hence, using utf16_utf8_strcpy() without checking the input
>may
>> lead to further writes beyond value[].
>>
>> The current invocation of strict_strtoul() reads beyond the end of
>value[].
>>
>> A non-hexadecimal value after "Capsule" (e.g. "CapsuleZZZZ") must
>result in
>> an error. We cat catch this by checking the return value of
>strict_strtoul().
>>
>> A value that is too short after "Capsule" (e.g. "Capsule0") must
>result in
>> an error. We must check the string length of value[].
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>
>> ---
>> lib/efi_loader/efi_capsule.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_capsule.c
>b/lib/efi_loader/efi_capsule.c
>> index d39d731080..0017f0c0db 100644
>> --- a/lib/efi_loader/efi_capsule.c
>> +++ b/lib/efi_loader/efi_capsule.c
>> @@ -42,20 +42,28 @@ static struct efi_file_handle *bootdev_root;
>> static __maybe_unused unsigned int get_last_capsule(void)
>> {
>> u16 value16[11]; /* "CapsuleXXXX": non-null-terminated */
>> - char value[11], *p;
>> + char value[5];
>> efi_uintn_t size;
>> unsigned long index = 0xffff;
>> efi_status_t ret;
>> + int i;
>>
>> size = sizeof(value16);
>> ret = efi_get_variable_int(L"CapsuleLast",
>&efi_guid_capsule_report,
>> NULL, &size, value16, NULL);
>> - if (ret != EFI_SUCCESS || u16_strncmp(value16, L"Capsule", 7))
>> + if (ret != EFI_SUCCESS || size != 22 ||
>> + u16_strncmp(value16, L"Capsule", 7))
>> goto err;
>> + for (i = 0; i < 4; ++i) {
>> + u16 c = value16[i + 7];
>>
>> - p = value;
>> - utf16_utf8_strcpy(&p, value16);
>> - strict_strtoul(&value[7], 16, &index);
>> + if (!c)
>> + goto err;
>
>Is this check necessary assuming size == 22?
Ok. Instead we should check for > 0x7f here to avoid illegal codes.
Best regards
Heinrich
>
>
>> + value[i] = c;
>
>You are implicitly casting the value from u16 to u8 here.
>This may lead to making an illegal code legal.
>
>-Takahiro Akashi
>
>> + }
>> + value[4] = 0;
>> + if (strict_strtoul(value, 16, &index))
>> + index = 0xffff;
>> err:
>> return index;
>> }
>> --
>> 2.30.0
>>
