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

[AKASHI Takahiro]

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