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

Even if the value is sane, the current code overwrites the stack of the caller? This becomes visible when the stack protector is enabled. Why don't you want to fix it?

Best regards

Heinrich

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