Fix possible heap overflow on read accessing getaddrinfo() result.

PATCH In review Gert Doering 2 participants 3 messages Nov 10, 2015 – Nov 11, 2015

Share & tools

Gert Doering

author

2015-11-10 21:58 UTC #0 lore raw

PATCH 1/1 Fix possible heap overflow on read accessing getaddrinfo() result.

The code always tried to copy-out a "struct sockaddr_in6" even for IPv4 results, which reads more bytes than getaddrinfo() is guaranteed to allocate.

Now, look at ai->ai_family and only copy "struct sockaddr" for IPv4.

Also, reformat this block of code to comply to coding style.

This is a specific 2.3 bug as the code in master (to be 2.4) has been completely rewritten to properly handle dual-stack and multiple responses from getaddrinfo() proper.

Bug found by Daniel Hirche using "gcc -fsanitize=address". No possible exploits are known.

Signed-off-by Gert Doering <[email protected]...>

---
 src/openvpn/socket.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

src/openvpn/socket.c +12 -8

index a143853..0f46bad 100644
@@ -1259,20 +1259,24 @@ resolve_remote (struct link_socket *sock,
 		  ASSERT (0);
 		}
 
-		  /* Temporary fix, this need to be changed for dual stack */
-		  status = openvpn_getaddrinfo(flags, sock->remote_host, retry,
-											  signal_received, af, &ai);
-		  if(status == 0) {
-			  sock->info.lsa->remote.addr.in6 = *((struct sockaddr_in6*)(ai->ai_addr));
-			  freeaddrinfo(ai);
+	      /* Temporary fix, this need to be changed for dual stack */
+	      status = openvpn_getaddrinfo(flags, sock->remote_host, retry,
+					    signal_received, af, &ai);
+	      if(status == 0)
+		{
+		  if ( ai->ai_family == AF_INET6 )
+		    sock->info.lsa->remote.addr.in6 = *((struct sockaddr_in6*)(ai->ai_addr));
+		  else
+		    sock->info.lsa->remote.addr.in4 = *((struct sockaddr_in*)(ai->ai_addr));
+		  freeaddrinfo(ai);
 
-			  dmsg (D_SOCKET_DEBUG, "RESOLVE_REMOTE flags=0x%04x phase=%d rrs=%d sig=%d status=%d",
+		  dmsg (D_SOCKET_DEBUG, "RESOLVE_REMOTE flags=0x%04x phase=%d rrs=%d sig=%d status=%d",
 					flags,
 					phase,
 					retry,
 					signal_received ? *signal_received : -1,
 					status);
-		  }
+		}
 	      if (signal_received)
 		{
 		  if (*signal_received)

Signature · 1 line

2.4.9

Arne Schwabe

2015-11-10 22:23 UTC #1 lore raw

Quoted 6 lines · Gert Doering

This is a specific 2.3 bug as the code in master (to be 2.4) has been completely rewritten to properly handle dual-stack and multiple responses from getaddrinfo() proper.
Bug found by Daniel Hirche using "gcc -fsanitize=address". No possible exploits are known.

ACK.

Arne

Gert Doering

author

2015-11-11 08:19 UTC #2 lore raw

In reply to Gert Doering

Your patch has been applied to the release/2.3 branch.

commit cdbadd00582af84aa8adf588743eec8d01910a0b

Author: Gert Doering
Date:   Tue Nov 10 22:58:39 2015 +0100

     Fix possible heap overflow on read accessing getaddrinfo() result.

     Signed-off-by: Gert Doering <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10479

--
kind regards,

Gert Doering