Fix OOB read in DNS resolver
The remote server specifies resplen, the length of the response it
intends to send. anssiz represents the size of the destination buffer.
If the reported resplen is larger than the anssiz, the code correctly
only reads up to anssiz bytes, but returns resplen. so later functions
will access far out of bounds.
The fix ensures that the length of send_vc return does not exceed the
buffer size.
Bug: 161362564
Test: atest pass on HWAddressSanitizer build.
Merged-In: Id4b5df1be4652e4623847b0b0bad0af65b80fdd5
Change-Id: Id4b5df1be4652e4623847b0b0bad0af65b80fdd5
(cherry picked from commit cf6ee247113426ef4e7365a86d00bb5430186802)
(cherry picked from commit 5214c6bebaadfe307579ee930fc650235b157192)
diff --git a/res_send.cpp b/res_send.cpp
index 2db8220..04421c5 100644
--- a/res_send.cpp
+++ b/res_send.cpp
@@ -829,6 +829,9 @@
else
break;
}
+ LOG(WARNING) << __func__ << ": resplen " << resplen << " exceeds buf size " << anssiz;
+ // return size should never exceed container size
+ resplen = anssiz;
}
/*
* If the calling application has bailed out of
@@ -839,7 +842,7 @@
*/
if (hp->id != anhp->id) {
LOG(DEBUG) << __func__ << ": ld answer (unexpected):";
- res_pquery(ans, (resplen > anssiz) ? anssiz : resplen);
+ res_pquery(ans, resplen);
goto read_len;
}
diff --git a/resolv_unit_test.cpp b/resolv_unit_test.cpp
index 3274ed8..befca9e 100644
--- a/resolv_unit_test.cpp
+++ b/resolv_unit_test.cpp
@@ -123,9 +123,7 @@
dns.clearQueries();
}
- int SetResolvers() {
- return resolv_set_nameservers(TEST_NETID, servers, domains, params);
- }
+ int SetResolvers() { return resolv_set_nameservers(TEST_NETID, servers, domains, params); }
const android_net_context mNetcontext = {
.app_netid = TEST_NETID,
@@ -1139,6 +1137,44 @@
}
}
+// Audit if Resolver read out of bounds, which needs HWAddressSanitizer build to trigger SIGABRT.
+TEST_F(ResolvGetAddrInfoTest, OverlengthResp) {
+ std::vector<std::string> nameList;
+ // Construct a long enough record that exceeds 8192 bytes (the maximum buffer size):
+ // Header: (Transaction ID, Flags, ...) = 12 bytes
+ // Query: 19(Name)+2(Type)+2(Class) = 23 bytes
+ // The 1st answer RR: 19(Name)+2(Type)+2(Class)+4(TTL)+2(Len)+77(CNAME) = 106 bytes
+ // 2nd-50th answer RRs: 49*(77(Name)+2(Type)+2(Class)+4(TTL)+2(Len)+77(CNAME)) = 8036 bytes
+ // The last answer RR: 77(Name)+2(Type)+2(Class)+4(TTL)+2(Len)+4(Address) = 91 bytes
+ // ----------------------------------------------------------------------------------------
+ // Sum: 8268 bytes
+ for (int i = 0; i < 10; i++) {
+ std::string domain(kMaxmiumLabelSize / 2, 'a' + i);
+ for (int j = 0; j < 5; j++) {
+ nameList.push_back(domain + std::string(kMaxmiumLabelSize / 2 + 1, '0' + j) +
+ kExampleComDomain + ".");
+ }
+ }
+ test::DNSResponder dns;
+ dns.addMapping(kHelloExampleCom, ns_type::ns_t_cname, nameList[0]);
+ for (size_t i = 0; i < nameList.size() - 1; i++) {
+ dns.addMapping(nameList[i], ns_type::ns_t_cname, nameList[i + 1]);
+ }
+ dns.addMapping(nameList[nameList.size() - 1], ns_type::ns_t_a, kHelloExampleComAddrV4);
+
+ ASSERT_TRUE(dns.startServer());
+ ASSERT_EQ(0, SetResolvers());
+ addrinfo* result = nullptr;
+ const addrinfo hints = {.ai_family = AF_INET};
+ NetworkDnsEventReported event;
+ int rv = resolv_getaddrinfo("hello", nullptr, &hints, &mNetcontext, &result, &event);
+ ScopedAddrinfo result_cleanup(result);
+ EXPECT_EQ(rv, EAI_FAIL);
+ EXPECT_TRUE(result == nullptr);
+ EXPECT_EQ(GetNumQueriesForProtocol(dns, IPPROTO_UDP, kHelloExampleCom), 2U);
+ EXPECT_EQ(GetNumQueriesForProtocol(dns, IPPROTO_TCP, kHelloExampleCom), 2U);
+}
+
TEST_F(GetHostByNameForNetContextTest, AlphabeticalHostname) {
constexpr char host_name[] = "jiababuei.example.com.";
constexpr char v4addr[] = "1.2.3.4";
diff --git a/tests/dns_responder/dns_responder.cpp b/tests/dns_responder/dns_responder.cpp
index e111eeb..1805cb1 100644
--- a/tests/dns_responder/dns_responder.cpp
+++ b/tests/dns_responder/dns_responder.cpp
@@ -252,7 +252,7 @@
}
std::string DNSQuestion::toString() const {
- char buffer[4096];
+ char buffer[16384];
int len = snprintf(buffer, sizeof(buffer), "Q<%s,%s,%s>", qname.name.c_str(),
dnstype2str(qtype), dnsclass2str(qclass));
return std::string(buffer, len);
@@ -291,7 +291,7 @@
}
std::string DNSRecord::toString() const {
- char buffer[4096];
+ char buffer[16384];
int len = snprintf(buffer, sizeof(buffer), "R<%s,%s,%s>", name.name.c_str(), dnstype2str(rtype),
dnsclass2str(rclass));
return std::string(buffer, len);
@@ -418,7 +418,7 @@
// TODO: convert all callers to this interface, then delete the old one.
bool DNSHeader::write(std::vector<uint8_t>* out) const {
- char buffer[4096];
+ char buffer[16384];
char* end = this->write(buffer, buffer + sizeof buffer);
if (end == nullptr) return false;
out->insert(out->end(), buffer, end);
@@ -896,7 +896,7 @@
bool DNSResponder::makeResponse(DNSHeader* header, int protocol, char* response,
size_t* response_len) const {
- char buffer[4096];
+ char buffer[16384];
size_t buffer_len = sizeof(buffer);
bool ret;
@@ -1059,7 +1059,7 @@
}
void DNSResponder::handleQuery(int protocol) {
- char buffer[4096];
+ char buffer[16384];
sockaddr_storage sa;
socklen_t sa_len = sizeof(sa);
ssize_t len = 0;
@@ -1103,7 +1103,7 @@
}
LOG(DEBUG) << "read " << len << " bytes on " << dnsproto2str(protocol);
std::lock_guard lock(cv_mutex_);
- char response[4096];
+ char response[16384];
size_t response_len = sizeof(response);
// TODO: check whether sending malformed packets to DnsResponder
if (handleDNSRequest(buffer, len, protocol, response, &response_len) && response_len > 0) {