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

[Heinrich Schuchardt]

Am 10. Februar 2021 07:43:25 MEZ schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
>On Wed, Feb 10, 2021 at 07:05:10AM +0100, Heinrich Schuchardt wrote:
>> 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?
>
>If this is your concern, the best solution would be to prohibit
>users to create system-managed (read-only) variables, like CapsuleLast.
>
>If a user can create such a variable with an arbitrary value,
>it will hurt the system's integrity.
>
>> 
>> >
>> >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?
>
>My assumption here is, as I said,
>> >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.
>
>-Takahiro Akashi
>
>
>> >
>> >> 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?

The value of bytes 19-22 could be zero.


>> 
>> 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
>> >> 
>>