From 2d5393bb28fdfe3479c09f492a810277e06dbeb7 Mon Sep 17 00:00:00 2001 From: Aastha Gupta 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 Reviewed-By: Rich Trott --- 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 4a332db71279a4..e21fc2e303897f 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -1067,29 +1067,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; @@ -1101,27 +1103,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) {