From 7de986dcca0ab68c9f22f00658afda080961ce98 Mon Sep 17 00:00:00 2001 From: Daiyuu Nobori Date: Mon, 15 Jan 2018 10:25:10 +0900 Subject: [PATCH] 7 missing memory boundaries checks and similar memory problems. There are no risk of arbitrary code execution or intrusion on these bugs in my analysis. However, these problems may lead to crash the running server process. So these bugs must be fixed. Buffer overread in ParseL2TPPacket() Memory corruption in IcmpParseResult Missing bounds check in ParseUDP() can lead to invalid memory access Out-of-bounds read in IPsec_PPP.c (unterminated string buffer) Overlapping parameters to memcpy() via StrToIp6() PACK ReadValue() crash vulnerability Potential use of uninitialized memory via IPToInAddr6() 4 memory leaks. While the amount of leakage is very small per time, these bugs can finally cause process crash by out of memory. So these bugs must be fixed. Memory leak in NnReadDnsRecord Memory leak in RadiusLogin() Memory leak via ParsePacketIPv4WithDummyMacHeader Remote memory leak in OpenVPN server code 1 coding improvement. This is not a bug, however, I fixed the code to avoid furture misunderstanding. RecvAll can return success on failure (leading to use of uninitialized memory) Contributors for this bugfix: - Max Planck Institute for Molecular Genetics - Guido Vranken --- src/Cedar/IPsec_L2TP.c | 6 +++++ src/Cedar/IPsec_PPP.c | 4 ++-- src/Cedar/Interop_OpenVPN.c | 2 +- src/Cedar/Radius.c | 7 ++++++ src/Cedar/Virtual.c | 1 + src/Mayaqua/Memory.c | 15 ++++++++++++ src/Mayaqua/Memory.h | 1 + src/Mayaqua/Network.c | 12 ++++++++-- src/Mayaqua/Pack.c | 2 +- src/Mayaqua/Str.c | 48 +++++++++++++++++++++++++++++++++++++ src/Mayaqua/Str.h | 1 + src/Mayaqua/TcpIp.c | 18 ++++++++++---- 12 files changed, 106 insertions(+), 11 deletions(-) diff --git a/src/Cedar/IPsec_L2TP.c b/src/Cedar/IPsec_L2TP.c index 3a930f37..4c9f1e7b 100644 --- a/src/Cedar/IPsec_L2TP.c +++ b/src/Cedar/IPsec_L2TP.c @@ -792,6 +792,12 @@ L2TP_PACKET *ParseL2TPPacket(UDPPACKET *p) size -= 2; a.DataSize = a.Length - 6; + + if (a.DataSize > size) + { + goto LABEL_ERROR; + } + a.Data = Clone(buf, a.DataSize); buf += a.DataSize; diff --git a/src/Cedar/IPsec_PPP.c b/src/Cedar/IPsec_PPP.c index 5482c53e..d2b7cb18 100644 --- a/src/Cedar/IPsec_PPP.c +++ b/src/Cedar/IPsec_PPP.c @@ -291,7 +291,7 @@ void PPPThread(THREAD *thread, void *param) ReadBuf(b, client_response_buffer, 49); Zero(username_tmp, sizeof(username_tmp)); - ReadBuf(b, username_tmp, sizeof(username_tmp)); + ReadBuf(b, username_tmp, sizeof(username_tmp) - 1); Debug("First MS-CHAPv2: id=%s\n", username_tmp); @@ -977,7 +977,7 @@ PPP_PACKET *PPPProcessRequestPacket(PPP_SESSION *p, PPP_PACKET *req) ReadBuf(b, client_response_buffer, 49); Zero(username_tmp, sizeof(username_tmp)); - ReadBuf(b, username_tmp, sizeof(username_tmp)); + ReadBuf(b, username_tmp, sizeof(username_tmp) - 1); client_challenge_16 = client_response_buffer + 0; client_response_24 = client_response_buffer + 16 + 8; diff --git a/src/Cedar/Interop_OpenVPN.c b/src/Cedar/Interop_OpenVPN.c index 91715729..aecac52e 100644 --- a/src/Cedar/Interop_OpenVPN.c +++ b/src/Cedar/Interop_OpenVPN.c @@ -2845,7 +2845,7 @@ bool OvsPerformTcpServer(CEDAR *cedar, SOCK *sock) { void *ptr = FifoPtr(tcp_recv_fifo); USHORT packet_size = READ_USHORT(ptr); - if (packet_size <= OPENVPN_TCP_MAX_PACKET_SIZE) + if (packet_size != 0 && packet_size <= OPENVPN_TCP_MAX_PACKET_SIZE) { UINT total_len = (UINT)packet_size + sizeof(USHORT); if (r >= total_len) diff --git a/src/Cedar/Radius.c b/src/Cedar/Radius.c index c4a41df7..34c0838c 100644 --- a/src/Cedar/Radius.c +++ b/src/Cedar/Radius.c @@ -1827,6 +1827,13 @@ bool RadiusLogin(CONNECTION *c, char *server, UINT port, UCHAR *secret, UINT sec if (encrypted_password == NULL) { // Encryption failure + + // Release the ip_list + for(i = 0; i < LIST_NUM(ip_list); i++) + { + IP *tmp_ip = LIST_DATA(ip_list, i); + Free(tmp_ip); + } ReleaseList(ip_list); return false; } diff --git a/src/Cedar/Virtual.c b/src/Cedar/Virtual.c index f7f326d4..f3ea20c3 100644 --- a/src/Cedar/Virtual.c +++ b/src/Cedar/Virtual.c @@ -2250,6 +2250,7 @@ BUF *NnReadDnsRecord(BUF *buf, bool answer, USHORT *ret_type, USHORT *ret_class) data = Malloc(data_len); if (ReadBuf(buf, data, data_len) != data_len) { + Free(data); return false; } diff --git a/src/Mayaqua/Memory.c b/src/Mayaqua/Memory.c index 31268061..92b7efa5 100644 --- a/src/Mayaqua/Memory.c +++ b/src/Mayaqua/Memory.c @@ -4313,6 +4313,21 @@ void Copy(void *dst, void *src, UINT size) memcpy(dst, src, size); } +// Memory move +void Move(void *dst, void *src, UINT size) +{ + // Validate arguments + if (dst == NULL || src == NULL || size == 0 || dst == src) + { + return; + } + + // KS + KS_INC(KS_COPY_COUNT); + + memmove(dst, src, size); +} + // Memory comparison int Cmp(void *p1, void *p2, UINT size) { diff --git a/src/Mayaqua/Memory.h b/src/Mayaqua/Memory.h index 693386ba..28babd8b 100644 --- a/src/Mayaqua/Memory.h +++ b/src/Mayaqua/Memory.h @@ -284,6 +284,7 @@ void *InternalReAlloc(void *addr, UINT size); void InternalFree(void *addr); void Copy(void *dst, void *src, UINT size); +void Move(void *dst, void *src, UINT size); int Cmp(void *p1, void *p2, UINT size); int CmpCaseIgnore(void *p1, void *p2, UINT size); void ZeroMem(void *addr, UINT size); diff --git a/src/Mayaqua/Network.c b/src/Mayaqua/Network.c index df057213..a1d5c83c 100644 --- a/src/Mayaqua/Network.c +++ b/src/Mayaqua/Network.c @@ -7373,7 +7373,7 @@ bool StrToIP6(IP *ip, char *str) if (StartWith(tmp, "[") && EndWith(tmp, "]")) { // If the string is enclosed in square brackets, remove brackets - StrCpy(tmp, sizeof(tmp), &tmp[1]); + StrCpyAllowOverlap(tmp, sizeof(tmp), &tmp[1]); if (StrLen(tmp) >= 1) { @@ -12691,6 +12691,14 @@ bool RecvAll(SOCK *sock, void *data, UINT size, bool secure) { return false; } + if (ret == SOCK_LATER) + { + // I suppose that this is safe because the RecvAll() function is used only + // if the sock->AsyncMode == true. And the Recv() function may return + // SOCK_LATER only if the sock->AsyncMode == false. Therefore the call of + // Recv() function in the RecvAll() function never returns SOCK_LATER. + return false; + } recv_size += ret; if (recv_size >= size) { @@ -17597,7 +17605,7 @@ void IPToInAddr6(struct in6_addr *addr, IP *ip) return; } - Zero(addr, sizeof(struct in_addr)); + Zero(addr, sizeof(struct in6_addr)); if (IsIP6(ip)) { diff --git a/src/Mayaqua/Pack.c b/src/Mayaqua/Pack.c index 64ae8ce8..7cc67058 100644 --- a/src/Mayaqua/Pack.c +++ b/src/Mayaqua/Pack.c @@ -354,7 +354,7 @@ VALUE *ReadValue(BUF *b, UINT type) break; case VALUE_STR: // ANSI string len = ReadBufInt(b); - if ((len + 1) > MAX_VALUE_SIZE) + if (len > (MAX_VALUE_SIZE - 1)) { // Size over break; diff --git a/src/Mayaqua/Str.c b/src/Mayaqua/Str.c index 66e7acc3..482f9822 100644 --- a/src/Mayaqua/Str.c +++ b/src/Mayaqua/Str.c @@ -3346,6 +3346,54 @@ UINT StrCpy(char *dst, UINT size, char *src) return len; } +UINT StrCpyAllowOverlap(char *dst, UINT size, char *src) +{ + UINT len; + // Validate arguments + if (dst == src) + { + return StrLen(src); + } + if (dst == NULL || src == NULL) + { + if (src == NULL && dst != NULL) + { + if (size >= 1) + { + dst[0] = '\0'; + } + } + return 0; + } + if (size == 1) + { + dst[0] = '\0'; + return 0; + } + if (size == 0) + { + // Ignore the length + size = 0x7fffffff; + } + + // Check the length + len = StrLen(src); + if (len <= (size - 1)) + { + Move(dst, src, len + 1); + } + else + { + len = size - 1; + Move(dst, src, len); + dst[len] = '\0'; + } + + // KS + KS_INC(KS_STRCPY_COUNT); + + return len; +} // Check whether the string buffer is within the specified size bool StrCheckSize(char *str, UINT size) diff --git a/src/Mayaqua/Str.h b/src/Mayaqua/Str.h index f5ad7769..92da8dc8 100644 --- a/src/Mayaqua/Str.h +++ b/src/Mayaqua/Str.h @@ -135,6 +135,7 @@ UINT StrSize(char *str); bool StrCheckLen(char *str, UINT len); bool StrCheckSize(char *str, UINT size); UINT StrCpy(char *dst, UINT size, char *src); +UINT StrCpyAllowOverlap(char *dst, UINT size, char *src); UINT StrCat(char *dst, UINT size, char *src); UINT StrCatLeft(char *dst, UINT size, char *src); char ToLower(char c); diff --git a/src/Mayaqua/TcpIp.c b/src/Mayaqua/TcpIp.c index c838f9f6..00b16cc8 100644 --- a/src/Mayaqua/TcpIp.c +++ b/src/Mayaqua/TcpIp.c @@ -174,14 +174,14 @@ ICMP_RESULT *IcmpParseResult(IP *dest_ip, USHORT src_id, USHORT src_seqno, UCHAR if (true) { UINT ip_header_size = GetIpHeaderSize(recv_buffer, i); - if (ip_header_size >= sizeof(IPV4_HEADER)) + if (ip_header_size >= sizeof(IPV4_HEADER) && (ip_header_size <= i)) { IPV4_HEADER *ipv4 = (IPV4_HEADER *)recv_buffer; if ((IPV4_GET_VERSION(ipv4) == 4) && (ipv4->Protocol == IP_PROTO_ICMPV4)) { UINT ip_total_len = (UINT)Endian16(ipv4->TotalLength); - if ((ip_total_len >= sizeof(IPV4_HEADER)) && (ip_total_len <= i)) + if ((ip_total_len >= sizeof(IPV4_HEADER)) && (ip_total_len <= i) && (ip_total_len >= ip_header_size)) { UINT icmp_packet_size = ip_total_len - ip_header_size; ICMP_HEADER *icmp = (ICMP_HEADER *)(recv_buffer + ip_header_size); @@ -1957,7 +1957,7 @@ void CorrectChecksum(PKT *p) { udp->Checksum = 0; - if ((IPV4_GET_FLAGS(v4) & 0x01) == 0) + if ((IPV4_GET_FLAGS(v4) & 0x01) == 0 && (p->IPv4PayloadSize >= udp_len)) { // Calculate the checksum correctly based on the data in case of a non-fragmented packet udp->Checksum = CalcChecksumForIPv4(v4->SrcIP, v4->DstIP, IP_PROTO_UDP, udp, udp_len, 0); @@ -2023,7 +2023,7 @@ void CorrectChecksum(PKT *p) { udp->Checksum = 0; - if (v6info->FragmentHeader == NULL || ((IPV6_GET_FLAGS(v6info->FragmentHeader) & IPV6_FRAGMENT_HEADER_FLAG_MORE_FRAGMENTS) == 0)) + if ((v6info->FragmentHeader == NULL || ((IPV6_GET_FLAGS(v6info->FragmentHeader) & IPV6_FRAGMENT_HEADER_FLAG_MORE_FRAGMENTS) == 0)) && (v6info->PayloadSize >= udp_len)) { // If the packet is not fragmented, recalculate the checksum udp->Checksum = CalcChecksumForIPv6(&v6->SrcAddress, &v6->DestAddress, IP_PROTO_UDP, udp, udp_len, 0); @@ -2868,6 +2868,7 @@ PKT *ParsePacketIPv4WithDummyMacHeader(UCHAR *buf, UINT size) { UCHAR *tmp; UINT tmp_size; + PKT *ret; // Validate arguments if (buf == NULL) { @@ -2880,7 +2881,14 @@ PKT *ParsePacketIPv4WithDummyMacHeader(UCHAR *buf, UINT size) WRITE_USHORT(tmp + 12, MAC_PROTO_IPV4); Copy(tmp + 14, buf, size); - return ParsePacket(tmp, tmp_size); + ret = ParsePacket(tmp, tmp_size); + + if (ret == NULL) + { + Free(tmp); + } + + return ret; } // IPv4 parsing