translate_slashes(): don't write to string literals

Currently, all three invocations of the translate_slashes() function may
lead to writes to the string literal that is #defined with the
DEFAULT_LOADER_CHAR macro. According to ISO C99 6.4.5p6, this is undefined
behavior ("If the program attempts to modify such an array, the behavior
is undefined").

This bug crashes shim on e.g. the 64-bit ArmVirtQemu platform ("Data
abort: Permission fault"), where the platform firmware maps the .text
section (which contains the string literal) read-only.

Modify translate_slashes() so that it copies and translates characters
from an input array of "char" to an output array of "CHAR8".

While at it, fix another bug. Before this patch, if translate_slashes()
ever encountered a double backslash (translating it to a single forward
slash), then the output would end up shorter than the input. However, the
output was not NUL-terminated in-place, therefore the original string
length (and according trailing garbage) would be preserved. After this
patch, the NUL-termination on contraction is automatic, as the output
array's contents are indeterminate when entering the function, and so we
must NUL-terminate it anyway.

Fixes: 8e9124227d
Fixes: e62b69a5b0
Fixes: 3d79bcb265
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1795654
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Upstream-commit-id: 9813e8bc8b3
This commit is contained in:
Laszlo Ersek 2020-01-28 23:33:46 +01:00 committed by Peter Jones
parent 959f5e4e99
commit c6bedd5b83
3 changed files with 21 additions and 13 deletions

View File

@ -743,14 +743,14 @@ httpboot_fetch_buffer (EFI_HANDLE image, VOID **buffer, UINT64 *buf_size)
{
EFI_STATUS efi_status;
EFI_HANDLE nic;
CHAR8 *next_loader = NULL;
CHAR8 next_loader[sizeof DEFAULT_LOADER_CHAR];
CHAR8 *next_uri = NULL;
CHAR8 *hostname = NULL;
if (!uri)
return EFI_NOT_READY;
next_loader = translate_slashes(DEFAULT_LOADER_CHAR);
translate_slashes(next_loader, DEFAULT_LOADER_CHAR);
/* Create the URI for the next loader based on the original URI */
efi_status = generate_next_uri(uri, next_loader, &next_uri);

View File

@ -45,21 +45,23 @@ strcata(CHAR8 *dest, const CHAR8 *src)
static inline
__attribute__((unused))
CHAR8 *
translate_slashes(char *str)
translate_slashes(CHAR8 *out, const char *str)
{
int i;
int j;
if (str == NULL)
return (CHAR8 *)str;
if (str == NULL || out == NULL)
return NULL;
for (i = 0, j = 0; str[i] != '\0'; i++, j++) {
if (str[i] == '\\') {
str[j] = '/';
out[j] = '/';
if (str[i+1] == '\\')
i++;
}
} else
out[j] = str[i];
}
return (CHAR8 *)str;
out[j] = '\0';
return out;
}
#endif /* SHIM_STR_H */

View File

@ -189,7 +189,9 @@ static BOOLEAN extract_tftp_info(CHAR8 *url)
CHAR8 *start, *end;
CHAR8 ip6str[40];
CHAR8 ip6inv[16];
CHAR8 *template = (CHAR8 *)translate_slashes(DEFAULT_LOADER_CHAR);
CHAR8 template[sizeof DEFAULT_LOADER_CHAR];
translate_slashes(template, DEFAULT_LOADER_CHAR);
// to check against str2ip6() errors
memset(ip6inv, 0, sizeof(ip6inv));
@ -254,10 +256,14 @@ static EFI_STATUS parseDhcp6()
static EFI_STATUS parseDhcp4()
{
CHAR8 *template = (CHAR8 *)translate_slashes(DEFAULT_LOADER_CHAR);
INTN template_len = strlen(template) + 1;
CHAR8 template[sizeof DEFAULT_LOADER_CHAR];
INTN template_len;
UINTN template_ofs = 0;
EFI_PXE_BASE_CODE_DHCPV4_PACKET* pkt_v4 = (EFI_PXE_BASE_CODE_DHCPV4_PACKET *)&pxe->Mode->DhcpAck.Dhcpv4;
translate_slashes(template, DEFAULT_LOADER_CHAR);
template_len = strlen(template) + 1;
if(pxe->Mode->ProxyOfferReceived) {
/*
* Proxy should not have precedence. Check if DhcpAck
@ -288,8 +294,8 @@ static EFI_STATUS parseDhcp4()
full_path[dir_len-1] = '\0';
}
if (dir_len == 0 && dir[0] != '/' && template[0] == '/')
template++;
strcata(full_path, template);
template_ofs++;
strcata(full_path, template + template_ofs);
memcpy(&tftp_addr.v4, pkt_v4->BootpSiAddr, 4);
return EFI_SUCCESS;