From 6bd858269e91b3966c569f5d18a6fd3932b65112 Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Fri, 20 Sep 2013 11:29:23 -0500 Subject: [PATCH 1/7] Pass the right arguments to EFI_PXE_BASE_CODE_TFTP_READ_FILE A wrong pointer was being passed to EFI_PXE_BASE_CODE_TFTP_READ_FILE, preventing us from getting the file size back from the tftp call, ensuring that we don't have enough information to properly secureboot-validate the retrieved image. --- netboot.c | 4 ++-- shim.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/netboot.c b/netboot.c index d569048..f7a6a1a 100644 --- a/netboot.c +++ b/netboot.c @@ -328,7 +328,7 @@ EFI_STATUS parseNetbootinfo(EFI_HANDLE image_handle) return rc; } -EFI_STATUS FetchNetbootimage(EFI_HANDLE image_handle, VOID **buffer, UINTN *bufsiz) +EFI_STATUS FetchNetbootimage(EFI_HANDLE image_handle, VOID **buffer, UINT64 *bufsiz) { EFI_STATUS rc; EFI_PXE_BASE_CODE_TFTP_OPCODE read = EFI_PXE_BASE_CODE_TFTP_READ_FILE; @@ -346,7 +346,7 @@ EFI_STATUS FetchNetbootimage(EFI_HANDLE image_handle, VOID **buffer, UINTN *bufs try_again: rc = uefi_call_wrapper(pxe->Mtftp, 10, pxe, read, *buffer, overwrite, - &bufsiz, &blksz, &tftp_addr, full_path, NULL, nobuffer); + bufsiz, &blksz, &tftp_addr, full_path, NULL, nobuffer); if (rc == EFI_BUFFER_TOO_SMALL) { /* try again, doubling buf size */ diff --git a/shim.c b/shim.c index 47e3812..c1bb85f 100644 --- a/shim.c +++ b/shim.c @@ -1193,7 +1193,7 @@ EFI_STATUS start_image(EFI_HANDLE image_handle, CHAR16 *ImagePath) EFI_DEVICE_PATH *path; CHAR16 *PathName = NULL; void *sourcebuffer = NULL; - UINTN sourcesize = 0; + UINT64 sourcesize = 0; void *data = NULL; int datasize; -- 1.8.1.4 From b1fa932c45038fbe280420b88f0103610fff48aa Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Fri, 20 Sep 2013 13:03:57 -0500 Subject: [PATCH 2/7] Fix nul termination errors in filenames passed to tftp Fix various errors in the tftp string handling, to ensure we always have properly nul-terminated strings. --- netboot.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/netboot.c b/netboot.c index f7a6a1a..b31e71c 100644 --- a/netboot.c +++ b/netboot.c @@ -54,7 +54,7 @@ static inline unsigned short int __swap16(unsigned short int x) static EFI_PXE_BASE_CODE *pxe; static EFI_IP_ADDRESS tftp_addr; -static char *full_path; +static UINT8 *full_path; typedef struct { @@ -112,7 +112,7 @@ try_again: for (i=0; i < (bs / sizeof(EFI_HANDLE)); i++) { status = uefi_call_wrapper(BS->OpenProtocol, 6, hbuf[i], &pxe_base_code_protocol, - &pxe, image_handle, NULL, + (void **)&pxe, image_handle, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL); if (status != EFI_SUCCESS) { @@ -228,15 +228,15 @@ static UINT8 *str2ip6(char *str) static BOOLEAN extract_tftp_info(char *url) { - char *start, *end; + CHAR8 *start, *end; char ip6str[128]; - char *template = DEFAULT_LOADER; + CHAR8 *template = (CHAR8 *)DEFAULT_LOADER; if (strncmp((UINT8 *)url, (UINT8 *)"tftp://", 7)) { Print(L"URLS MUST START WITH tftp://\n"); return FALSE; } - start = url + 7; + start = (CHAR8 *)url + 7; if (*start != '[') { Print(L"TFTP SERVER MUST BE ENCLOSED IN [..]\n"); return FALSE; @@ -251,21 +251,19 @@ static BOOLEAN extract_tftp_info(char *url) Print(L"TFTP SERVER MUST BE ENCLOSED IN [..]\n"); return FALSE; } - *end = '\0'; memset(ip6str, 0, 128); - memcpy(ip6str, start, strlen((UINT8 *)start)); - *end = ']'; + memcpy(ip6str, start, end + 1 - start); end++; memcpy(&tftp_addr.v6, str2ip6(ip6str), 16); - full_path = AllocatePool(strlen((UINT8 *)end)+strlen((UINT8 *)template)+1); + full_path = AllocateZeroPool(strlen(end)+strlen(template)+1); if (!full_path) return FALSE; - memset(full_path, 0, strlen((UINT8 *)end)+strlen((UINT8 *)template)); - memcpy(full_path, end, strlen((UINT8 *)end)); - end = strrchr(full_path, '/'); + memcpy(full_path, end, strlen(end)); + end = (CHAR8 *)strrchr((char *)full_path, '/'); if (!end) - end = full_path; - memcpy(end, template, strlen((UINT8 *)template)); + end = (CHAR8 *)full_path; + memcpy(end, template, strlen(template)); + end[strlen(template)] = '\0'; return TRUE; } @@ -286,20 +284,15 @@ static EFI_STATUS parseDhcp6() static EFI_STATUS parseDhcp4() { - char *template = DEFAULT_LOADER; - char *tmp; - int len = strlen((CHAR8 *)template); + CHAR8 *template = (CHAR8 *)DEFAULT_LOADER; + full_path = AllocateZeroPool(strlen(template)+1); - tmp = AllocatePool(len+1); - - if (!tmp) + if (!full_path) return EFI_OUT_OF_RESOURCES; - memcpy(&tftp_addr.v4, pxe->Mode->DhcpAck.Dhcpv4.BootpSiAddr, 4); - memcpy(tmp, template, len+1); - full_path = tmp; + memcpy(full_path, template, strlen(template)); /* Note we don't capture the filename option here because we know its shim.efi * We instead assume the filename at the end of the path is going to be grubx64.efi -- 1.8.1.4 From a68d8233dcc76094813e5c235a80fb6c7ec6ad7c Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Fri, 20 Sep 2013 17:06:33 -0500 Subject: [PATCH 3/7] Fix an off-by-one error We don't need to add one because our end pointer is already off the end of the string we want to copy. --- netboot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netboot.c b/netboot.c index b31e71c..15dbdf7 100644 --- a/netboot.c +++ b/netboot.c @@ -252,7 +252,7 @@ static BOOLEAN extract_tftp_info(char *url) return FALSE; } memset(ip6str, 0, 128); - memcpy(ip6str, start, end + 1 - start); + memcpy(ip6str, start, end - start); end++; memcpy(&tftp_addr.v6, str2ip6(ip6str), 16); full_path = AllocateZeroPool(strlen(end)+strlen(template)+1); -- 1.8.1.4 From bbaa1df5dcc6570dc29544dbcc00353f925a1128 Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Sun, 22 Sep 2013 22:21:49 -0700 Subject: [PATCH 4/7] Misc allocation cleanups --- netboot.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/netboot.c b/netboot.c index 15dbdf7..c81e28e 100644 --- a/netboot.c +++ b/netboot.c @@ -160,10 +160,9 @@ static char *get_v6_bootfile_url(EFI_PXE_BASE_CODE_DHCPV6_PACKET *pkt) if (ntohs(option->OpCode) == 59) { /* This is the bootfile url option */ urllen = ntohs(option->Length); - url = AllocatePool(urllen+2); + url = AllocateZeroPool(urllen+1); if (!url) return NULL; - memset(url, 0, urllen+2); memcpy(url, option->Data, urllen); return url; } @@ -275,10 +274,13 @@ static EFI_STATUS parseDhcp6() bootfile_url = get_v6_bootfile_url(packet); - if (extract_tftp_info(bootfile_url) == FALSE) - return EFI_NOT_FOUND; if (!bootfile_url) return EFI_NOT_FOUND; + if (extract_tftp_info(bootfile_url) == FALSE) { + FreePool(bootfile_url); + return EFI_NOT_FOUND; + } + FreePool(bootfile_url); return EFI_SUCCESS; } -- 1.8.1.4 From 4b1e7425479a111553f1055757429249bc389d28 Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Sun, 22 Sep 2013 22:25:47 -0700 Subject: [PATCH 5/7] More consistent types, fewer casts --- netboot.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/netboot.c b/netboot.c index c81e28e..dab1f5c 100644 --- a/netboot.c +++ b/netboot.c @@ -142,11 +142,11 @@ try_again: return rc; } -static char *get_v6_bootfile_url(EFI_PXE_BASE_CODE_DHCPV6_PACKET *pkt) +static CHAR8 *get_v6_bootfile_url(EFI_PXE_BASE_CODE_DHCPV6_PACKET *pkt) { void *optr; EFI_DHCP6_PACKET_OPTION *option; - char *url; + CHAR8 *url; UINT32 urllen; optr = pkt->DhcpOptions; @@ -225,7 +225,7 @@ static UINT8 *str2ip6(char *str) return (UINT8 *)ip; } -static BOOLEAN extract_tftp_info(char *url) +static BOOLEAN extract_tftp_info(CHAR8 *url) { CHAR8 *start, *end; char ip6str[128]; @@ -235,7 +235,7 @@ static BOOLEAN extract_tftp_info(char *url) Print(L"URLS MUST START WITH tftp://\n"); return FALSE; } - start = (CHAR8 *)url + 7; + start = url + 7; if (*start != '[') { Print(L"TFTP SERVER MUST BE ENCLOSED IN [..]\n"); return FALSE; @@ -270,8 +270,7 @@ static BOOLEAN extract_tftp_info(char *url) static EFI_STATUS parseDhcp6() { EFI_PXE_BASE_CODE_DHCPV6_PACKET *packet = (EFI_PXE_BASE_CODE_DHCPV6_PACKET *)&pxe->Mode->DhcpAck.Raw; - char *bootfile_url; - + CHAR8 *bootfile_url; bootfile_url = get_v6_bootfile_url(packet); if (!bootfile_url) -- 1.8.1.4 From 12cd90c232301efe7d262a33c471a6af1282ae03 Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Sun, 22 Sep 2013 22:45:26 -0700 Subject: [PATCH 6/7] Correct limits on the length of ipv6 addresses The maximum length of a string representation of an ipv6 address is 39 characters (8 groups of 4 hex chars, with 7 colons in between). So don't allocate more room than this - and more importantly, don't blindly accept strings from the server that are longer than our buffer... --- netboot.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/netboot.c b/netboot.c index dab1f5c..61777a2 100644 --- a/netboot.c +++ b/netboot.c @@ -228,7 +228,7 @@ static UINT8 *str2ip6(char *str) static BOOLEAN extract_tftp_info(CHAR8 *url) { CHAR8 *start, *end; - char ip6str[128]; + char ip6str[40]; CHAR8 *template = (CHAR8 *)DEFAULT_LOADER; if (strncmp((UINT8 *)url, (UINT8 *)"tftp://", 7)) { @@ -245,12 +245,16 @@ static BOOLEAN extract_tftp_info(CHAR8 *url) end = start; while ((*end != '\0') && (*end != ']')) { end++; + if (end - start > 39) { + Print(L"TFTP URL includes malformed IPv6 address\n"); + return FALSE; + } } if (end == '\0') { Print(L"TFTP SERVER MUST BE ENCLOSED IN [..]\n"); return FALSE; } - memset(ip6str, 0, 128); + memset(ip6str, 0, 40); memcpy(ip6str, start, end - start); end++; memcpy(&tftp_addr.v6, str2ip6(ip6str), 16); -- 1.8.1.4 From 0c3bd9d9ea5261cfdf5c1d6feb2f42d17ba4ca8a Mon Sep 17 00:00:00 2001 From: Steve Langasek Date: Sun, 22 Sep 2013 23:11:26 -0700 Subject: [PATCH 7/7] Fix a memory leak --- netboot.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/netboot.c b/netboot.c index 61777a2..927445d 100644 --- a/netboot.c +++ b/netboot.c @@ -356,6 +356,8 @@ try_again: goto try_again; } + if (rc != EFI_SUCCESS && *buffer) { + FreePool(*buffer); + } return rc; - } -- 1.8.1.4