Commit 2e67d65
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 93f947a commit 2e67d65
1 file changed
Lines changed: 10 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1066,29 +1066,31 @@ int ParseSoaReply(Environment* env, | |||
| 1066 | 1066 | // Can't use ares_parse_soa_reply() here which can only parse single record | |
| 1067 | 1067 | const unsigned int ancount = cares_get_16bit(buf + 6); | |
| 1068 | 1068 | unsigned char* ptr = buf + NS_HFIXEDSZ; | |
| 1069 | - char* name_temp; | ||
| 1069 | + char* name_temp = nullptr; | ||
| 1070 | 1070 | long temp_len; // NOLINT(runtime/int) | |
| 1071 | 1071 | int status = ares_expand_name(ptr, buf, len, &name_temp, &temp_len); | |
| 1072 | - const ares_unique_ptr name(name_temp); | ||
| 1073 | 1072 | if (status != ARES_SUCCESS) { | |
| 1074 | 1073 | // returns EBADRESP in case of invalid input | |
| 1075 | 1074 | return status == ARES_EBADNAME ? ARES_EBADRESP : status; | |
| 1076 | 1075 | } | |
| 1077 | 1076 | ||
| 1077 | + const ares_unique_ptr name(name_temp); | ||
| 1078 | + | ||
| 1078 | 1079 | if (ptr + temp_len + NS_QFIXEDSZ > buf + len) { | |
| 1079 | 1080 | return ARES_EBADRESP; | |
| 1080 | 1081 | } | |
| 1081 | 1082 | ptr += temp_len + NS_QFIXEDSZ; | |
| 1082 | 1083 | ||
| 1083 | 1084 | for (unsigned int i = 0; i < ancount; i++) { | |
| 1084 | - char* rr_name_temp; | ||
| 1085 | + char* rr_name_temp = nullptr; | ||
| 1085 | 1086 | long rr_temp_len; // NOLINT(runtime/int) | |
| 1086 | 1087 | int status2 = ares_expand_name(ptr, buf, len, &rr_name_temp, &rr_temp_len); | |
| 1087 | - const ares_unique_ptr rr_name(rr_name_temp); | ||
| 1088 | 1088 | ||
| 1089 | 1089 | if (status2 != ARES_SUCCESS) | |
| 1090 | 1090 | return status2 == ARES_EBADNAME ? ARES_EBADRESP : status2; | |
| 1091 | 1091 | ||
| 1092 | + const ares_unique_ptr rr_name(rr_name_temp); | ||
| 1093 | + | ||
| 1092 | 1094 | ptr += rr_temp_len; | |
| 1093 | 1095 | if (ptr + NS_RRFIXEDSZ > buf + len) { | |
| 1094 | 1096 | return ARES_EBADRESP; | |
@@ -1100,27 +1102,27 @@ int ParseSoaReply(Environment* env, | |||
| 1100 | 1102 | ||
| 1101 | 1103 | // only need SOA | |
| 1102 | 1104 | if (rr_type == ns_t_soa) { | |
| 1103 | - char* nsname_temp; | ||
| 1105 | + char* nsname_temp = nullptr; | ||
| 1104 | 1106 | long nsname_temp_len; // NOLINT(runtime/int) | |
| 1105 | 1107 | ||
| 1106 | 1108 | int status3 = ares_expand_name(ptr, buf, len, | |
| 1107 | 1109 | &nsname_temp, | |
| 1108 | 1110 | &nsname_temp_len); | |
| 1109 | - const ares_unique_ptr nsname(nsname_temp); | ||
| 1110 | 1111 | if (status3 != ARES_SUCCESS) { | |
| 1111 | 1112 | return status3 == ARES_EBADNAME ? ARES_EBADRESP : status3; | |
| 1112 | 1113 | } | |
| 1114 | + const ares_unique_ptr nsname(nsname_temp); | ||
| 1113 | 1115 | ptr += nsname_temp_len; | |
| 1114 | 1116 | ||
| 1115 | - char* hostmaster_temp; | ||
| 1117 | + char* hostmaster_temp = nullptr; | ||
| 1116 | 1118 | long hostmaster_temp_len; // NOLINT(runtime/int) | |
| 1117 | 1119 | int status4 = ares_expand_name(ptr, buf, len, | |
| 1118 | 1120 | &hostmaster_temp, | |
| 1119 | 1121 | &hostmaster_temp_len); | |
| 1120 | - const ares_unique_ptr hostmaster(hostmaster_temp); | ||
| 1121 | 1122 | if (status4 != ARES_SUCCESS) { | |
| 1122 | 1123 | return status4 == ARES_EBADNAME ? ARES_EBADRESP : status4; | |
| 1123 | 1124 | } | |
| 1125 | + const ares_unique_ptr hostmaster(hostmaster_temp); | ||
| 1124 | 1126 | ptr += hostmaster_temp_len; | |
| 1125 | 1127 | ||
| 1126 | 1128 | if (ptr + 5 * 4 > buf + len) { | |
0 commit comments