From 2e67d650bb7b092ed74a532940d1648388021c11 Mon Sep 17 00:00:00 2001
From: Aastha Gupta <aastha.gupta4104@gmail.com>
Date: Sun, 4 Oct 2020 20:19:51 +0530
Subject: [PATCH] src: fix freeing unintialized pointer bug in ParseSoaReply

ares_expand_name doesn't guarantee that pointer variable is initialized
if return code is ARES_EBADNAME or ARES_ENOMEM. But current usage of the
function in the codebase thinks otherwise.

There seems to be an assumption that pointer is always initialized even
though it is a local variable and we create a unique pointer soon after
calling ares_expand_name. This could potentially crash the program with
an invalid free pointer.

I was able to crash it by poisoning the memory and some manual hooks.

By moving the unique_ptr after checking the return code we can fix the
problem. As the underlying function guarantees that pointer is
initialized when the status is ARES_SUCCESS.

PR-URL: https://github.com/nodejs/node/pull/35502
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 src/cares_wrap.cc | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc
index 73a0ac6b334345..37389e741793be 100644
--- a/src/cares_wrap.cc
+++ b/src/cares_wrap.cc
@@ -1066,29 +1066,31 @@ int ParseSoaReply(Environment* env,
   // Can't use ares_parse_soa_reply() here which can only parse single record
   const unsigned int ancount = cares_get_16bit(buf + 6);
   unsigned char* ptr = buf + NS_HFIXEDSZ;
-  char* name_temp;
+  char* name_temp = nullptr;
   long temp_len;  // NOLINT(runtime/int)
   int status = ares_expand_name(ptr, buf, len, &name_temp, &temp_len);
-  const ares_unique_ptr name(name_temp);
   if (status != ARES_SUCCESS) {
     // returns EBADRESP in case of invalid input
     return status == ARES_EBADNAME ? ARES_EBADRESP : status;
   }
 
+  const ares_unique_ptr name(name_temp);
+
   if (ptr + temp_len + NS_QFIXEDSZ > buf + len) {
     return ARES_EBADRESP;
   }
   ptr += temp_len + NS_QFIXEDSZ;
 
   for (unsigned int i = 0; i < ancount; i++) {
-    char* rr_name_temp;
+    char* rr_name_temp = nullptr;
     long rr_temp_len;  // NOLINT(runtime/int)
     int status2 = ares_expand_name(ptr, buf, len, &rr_name_temp, &rr_temp_len);
-    const ares_unique_ptr rr_name(rr_name_temp);
 
     if (status2 != ARES_SUCCESS)
       return status2 == ARES_EBADNAME ? ARES_EBADRESP : status2;
 
+    const ares_unique_ptr rr_name(rr_name_temp);
+
     ptr += rr_temp_len;
     if (ptr + NS_RRFIXEDSZ > buf + len) {
       return ARES_EBADRESP;
@@ -1100,27 +1102,27 @@ int ParseSoaReply(Environment* env,
 
     // only need SOA
     if (rr_type == ns_t_soa) {
-      char* nsname_temp;
+      char* nsname_temp = nullptr;
       long nsname_temp_len;  // NOLINT(runtime/int)
 
       int status3 = ares_expand_name(ptr, buf, len,
                                      &nsname_temp,
                                      &nsname_temp_len);
-      const ares_unique_ptr nsname(nsname_temp);
       if (status3 != ARES_SUCCESS) {
         return status3 == ARES_EBADNAME ? ARES_EBADRESP : status3;
       }
+      const ares_unique_ptr nsname(nsname_temp);
       ptr += nsname_temp_len;
 
-      char* hostmaster_temp;
+      char* hostmaster_temp = nullptr;
       long hostmaster_temp_len;  // NOLINT(runtime/int)
       int status4 = ares_expand_name(ptr, buf, len,
                                      &hostmaster_temp,
                                      &hostmaster_temp_len);
-      const ares_unique_ptr hostmaster(hostmaster_temp);
       if (status4 != ARES_SUCCESS) {
         return status4 == ARES_EBADNAME ? ARES_EBADRESP : status4;
       }
+      const ares_unique_ptr hostmaster(hostmaster_temp);
       ptr += hostmaster_temp_len;
 
       if (ptr + 5 * 4 > buf + len) {