Skip to content

Commit 2d5393b

Browse files
AasthaGuptaMylesBorins
authored andcommittedNov 16, 2020
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: #35502 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent dec004f commit 2d5393b

File tree

1 file changed

+10
-8
lines changed

1 file changed

+10
-8
lines changed
 

‎src/cares_wrap.cc

+10-8
Original file line numberDiff line numberDiff line change
@@ -1067,29 +1067,31 @@ int ParseSoaReply(Environment* env,
10671067
// Can't use ares_parse_soa_reply() here which can only parse single record
10681068
const unsigned int ancount = cares_get_16bit(buf + 6);
10691069
unsigned char* ptr = buf + NS_HFIXEDSZ;
1070-
char* name_temp;
1070+
char* name_temp = nullptr;
10711071
long temp_len; // NOLINT(runtime/int)
10721072
int status = ares_expand_name(ptr, buf, len, &name_temp, &temp_len);
1073-
const ares_unique_ptr name(name_temp);
10741073
if (status != ARES_SUCCESS) {
10751074
// returns EBADRESP in case of invalid input
10761075
return status == ARES_EBADNAME ? ARES_EBADRESP : status;
10771076
}
10781077

1078+
const ares_unique_ptr name(name_temp);
1079+
10791080
if (ptr + temp_len + NS_QFIXEDSZ > buf + len) {
10801081
return ARES_EBADRESP;
10811082
}
10821083
ptr += temp_len + NS_QFIXEDSZ;
10831084

10841085
for (unsigned int i = 0; i < ancount; i++) {
1085-
char* rr_name_temp;
1086+
char* rr_name_temp = nullptr;
10861087
long rr_temp_len; // NOLINT(runtime/int)
10871088
int status2 = ares_expand_name(ptr, buf, len, &rr_name_temp, &rr_temp_len);
1088-
const ares_unique_ptr rr_name(rr_name_temp);
10891089

10901090
if (status2 != ARES_SUCCESS)
10911091
return status2 == ARES_EBADNAME ? ARES_EBADRESP : status2;
10921092

1093+
const ares_unique_ptr rr_name(rr_name_temp);
1094+
10931095
ptr += rr_temp_len;
10941096
if (ptr + NS_RRFIXEDSZ > buf + len) {
10951097
return ARES_EBADRESP;
@@ -1101,27 +1103,27 @@ int ParseSoaReply(Environment* env,
11011103

11021104
// only need SOA
11031105
if (rr_type == ns_t_soa) {
1104-
char* nsname_temp;
1106+
char* nsname_temp = nullptr;
11051107
long nsname_temp_len; // NOLINT(runtime/int)
11061108

11071109
int status3 = ares_expand_name(ptr, buf, len,
11081110
&nsname_temp,
11091111
&nsname_temp_len);
1110-
const ares_unique_ptr nsname(nsname_temp);
11111112
if (status3 != ARES_SUCCESS) {
11121113
return status3 == ARES_EBADNAME ? ARES_EBADRESP : status3;
11131114
}
1115+
const ares_unique_ptr nsname(nsname_temp);
11141116
ptr += nsname_temp_len;
11151117

1116-
char* hostmaster_temp;
1118+
char* hostmaster_temp = nullptr;
11171119
long hostmaster_temp_len; // NOLINT(runtime/int)
11181120
int status4 = ares_expand_name(ptr, buf, len,
11191121
&hostmaster_temp,
11201122
&hostmaster_temp_len);
1121-
const ares_unique_ptr hostmaster(hostmaster_temp);
11221123
if (status4 != ARES_SUCCESS) {
11231124
return status4 == ARES_EBADNAME ? ARES_EBADRESP : status4;
11241125
}
1126+
const ares_unique_ptr hostmaster(hostmaster_temp);
11251127
ptr += hostmaster_temp_len;
11261128

11271129
if (ptr + 5 * 4 > buf + len) {

0 commit comments

Comments
 (0)
Please sign in to comment.