Am 10. Februar 2021 07:43:25 MEZ schrieb AKASHI Takahiro <takahiro.aka...@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.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? > >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.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?
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 >> >> >>