[PATCH 1/1] efi_loader: fix get_last_capsule()

[Heinrich Schuchardt]

Am 10. Februar 2021 01:38:38 MEZ schrieb AKASHI Takahiro <takahiro.akashi at 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.glpk at 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
>>