Fix potential bugs that may cause resolver to retry endlessly
[1] If DNS-over-TLS validation success, but server does not
respond to DNS-over-TLS query after a while (the server can still
respond to DNS-over-UDP). Then, query a non-exist domain.
[2] If DNS-over-TLS validation success, but server does not
respond to DNS-over-TLS query after a while. Also, the server always
responds RCODE=RES_F_EDNS0ERR on DNS-over-UDP.
These strange network behaviors should not happen. However, once they
happen (maybe by bogus servers), the resolver should be able to handle
it gracefully.
Bug: 120910570
Test: runtests.sh pass
Change-Id: I7e3044e012303a7991b04e7d38e55340e2a5db1a
diff --git a/resolv/dns_responder/dns_responder.cpp b/resolv/dns_responder/dns_responder.cpp
index 13726cb..3da5f70 100644
--- a/resolv/dns_responder/dns_responder.cpp
+++ b/resolv/dns_responder/dns_responder.cpp
@@ -776,11 +776,17 @@
return makeErrorResponse(&header, ns_rcode::ns_r_formerr, response,
response_len);
}
+
+ if (edns_ == Edns::FORMERR_UNCOND) {
+ ALOGI("force to return RCODE FORMERR");
+ return makeErrorResponse(&header, ns_rcode::ns_r_formerr, response, response_len);
+ }
+
if (!header.additionals.empty() && edns_ != Edns::ON) {
ALOGI("DNS request has an additional section (assumed EDNS). "
"Simulating an ancient (pre-EDNS) server, and returning %s",
- edns_ == Edns::FORMERR ? "RCODE FORMERR." : "no response.");
- if (edns_ == Edns::FORMERR) {
+ edns_ == Edns::FORMERR_ON_EDNS ? "RCODE FORMERR." : "no response.");
+ if (edns_ == Edns::FORMERR_ON_EDNS) {
return makeErrorResponse(&header, ns_rcode::ns_r_formerr, response, response_len);
}
// No response.
diff --git a/resolv/dns_responder/dns_responder.h b/resolv/dns_responder/dns_responder.h
index 028d75a..edf3bbf 100644
--- a/resolv/dns_responder/dns_responder.h
+++ b/resolv/dns_responder/dns_responder.h
@@ -49,8 +49,9 @@
enum class Edns : uint8_t {
ON,
- FORMERR, // DNS server not supporting EDNS will reply FORMERR.
- DROP // DNS server not supporting EDNS will not do any response.
+ FORMERR_ON_EDNS, // DNS server not supporting EDNS will reply FORMERR.
+ FORMERR_UNCOND, // DNS server reply FORMERR unconditionally
+ DROP // DNS server not supporting EDNS will not do any response.
};
void addMapping(const std::string& name, ns_type type, const std::string& addr);
@@ -126,9 +127,9 @@
// Control how the DNS server behaves when it receives the requests containing OPT RR.
// If it's set Edns::ON, the server can recognize and reply the response; if it's set
- // Edns::FORMERR, the server behaves like an old DNS server that doesn't support EDNS0, and
- // replying FORMERR; if it's Edns::DROP, the server doesn't support EDNS0 either, and ignoring
- // the requests.
+ // Edns::FORMERR_ON_EDNS, the server behaves like an old DNS server that doesn't support EDNS0,
+ // and replying FORMERR; if it's Edns::DROP, the server doesn't support EDNS0 either, and
+ // ignoring the requests.
std::atomic<Edns> edns_ = Edns::ON;
// Mappings from (name, type) to registered response and the
diff --git a/resolv/getaddrinfo.cpp b/resolv/getaddrinfo.cpp
index 4a46c1a..7369b9b 100644
--- a/resolv/getaddrinfo.cpp
+++ b/resolv/getaddrinfo.cpp
@@ -1606,11 +1606,9 @@
for (t = target; t; t = t->next) {
u_char* answer;
int anslen;
- u_int oflags;
hp = (HEADER*) (void*) t->answer;
- oflags = res->_flags;
-
+ bool retried = false;
again:
hp->rcode = NOERROR; /* default */
@@ -1620,16 +1618,15 @@
answer = t->answer;
anslen = t->anslen;
#ifdef DEBUG
- if (res->options & RES_DEBUG) printf(";; res_nquery(%s, %d, %d)\n", name, cl, type);
+ if (res->options & RES_DEBUG) printf(";; res_queryN(%s, %d, %d)\n", name, cl, type);
#endif
n = res_nmkquery(res, QUERY, name, cl, type, NULL, 0, NULL, buf, sizeof(buf));
- if (n > 0 && (res->_flags & RES_F_EDNS0ERR) == 0 &&
- (res->options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) != 0)
+ if (n > 0 && (res->options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) != 0 && !retried)
n = res_nopt(res, n, buf, sizeof(buf), anslen);
if (n <= 0) {
#ifdef DEBUG
- if (res->options & RES_DEBUG) printf(";; res_nquery: mkquery failed\n");
+ if (res->options & RES_DEBUG) printf(";; res_queryN: mkquery failed\n");
#endif
*herrno = NO_RECOVERY;
return n;
@@ -1642,11 +1639,11 @@
if (rcode != RCODE_TIMEOUT) rcode = hp->rcode; /* record most recent error */
/* if the query choked with EDNS0, retry without EDNS0 */
if ((res->options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) != 0 &&
- ((oflags ^ res->_flags) & RES_F_EDNS0ERR) != 0) {
- res->_flags |= RES_F_EDNS0ERR;
+ (res->_flags & RES_F_EDNS0ERR) && !retried) {
#ifdef DEBUG
- if (res->options & RES_DEBUG) printf(";; res_nquery: retry without EDNS0\n");
+ if (res->options & RES_DEBUG) printf(";; res_queryN: retry without EDNS0\n");
#endif
+ retried = true;
goto again;
}
#ifdef DEBUG
diff --git a/resolv/res_query.cpp b/resolv/res_query.cpp
index 4ae13bf..acf6160 100644
--- a/resolv/res_query.cpp
+++ b/resolv/res_query.cpp
@@ -117,10 +117,8 @@
u_char buf[MAXPACKET];
HEADER* hp = (HEADER*) (void*) answer;
int n;
- u_int oflags;
int rcode = NOERROR;
-
- oflags = statp->_flags;
+ bool retried = false;
again:
hp->rcode = NOERROR; /* default */
@@ -130,8 +128,7 @@
#endif
n = res_nmkquery(statp, QUERY, name, cl, type, NULL, 0, NULL, buf, sizeof(buf));
- if (n > 0 && (statp->_flags & RES_F_EDNS0ERR) == 0 &&
- (statp->options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) != 0U)
+ if (n > 0 && (statp->options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) != 0U && !retried)
n = res_nopt(statp, n, buf, sizeof(buf), anslen);
if (n <= 0) {
#ifdef DEBUG
@@ -144,9 +141,9 @@
if (n < 0) {
/* if the query choked with EDNS0, retry without EDNS0 */
if ((statp->options & (RES_USE_EDNS0 | RES_USE_DNSSEC)) != 0U &&
- ((oflags ^ statp->_flags) & RES_F_EDNS0ERR) != 0) {
- statp->_flags |= RES_F_EDNS0ERR;
+ (statp->_flags & RES_F_EDNS0ERR) && !retried) {
if (statp->options & RES_DEBUG) printf(";; res_nquery: retry without EDNS0\n");
+ retried = true;
goto again;
}
#ifdef DEBUG
diff --git a/resolv/resolver_test.cpp b/resolv/resolver_test.cpp
index 598aaba..0f32fb9 100644
--- a/resolv/resolver_test.cpp
+++ b/resolv/resolver_test.cpp
@@ -1945,9 +1945,9 @@
}
// This test checks that the resolver should not generate the request containing OPT RR when using
-// cleartext DNS. If we query the DNS server not supporting EDNS0 and it reponds with FORMERR, we
-// will fallback to no EDNS0 and try again. If the server does no response, we won't retry so that
-// we get no answer.
+// cleartext DNS. If we query the DNS server not supporting EDNS0 and it reponds with
+// FORMERR_ON_EDNS, we will fallback to no EDNS0 and try again. If the server does no response, we
+// won't retry so that we get no answer.
TEST_F(ResolverTest, BrokenEdns) {
typedef test::DNSResponder::Edns Edns;
enum ExpectResult { EXPECT_FAILURE, EXPECT_SUCCESS };
@@ -1982,7 +1982,7 @@
case Edns::ON:
ednsString = "ednsOn";
break;
- case Edns::FORMERR:
+ case Edns::FORMERR_ON_EDNS:
ednsString = "ednsFormerr";
break;
case Edns::DROP:
@@ -2004,10 +2004,10 @@
{OPPORTUNISTIC_UDP, GETHOSTBYNAME, Edns::ON, EXPECT_SUCCESS},
{OPPORTUNISTIC_TLS, GETHOSTBYNAME, Edns::ON, EXPECT_SUCCESS},
{STRICT, GETHOSTBYNAME, Edns::ON, EXPECT_SUCCESS},
- {OFF, GETHOSTBYNAME, Edns::FORMERR, EXPECT_SUCCESS},
- {OPPORTUNISTIC_UDP, GETHOSTBYNAME, Edns::FORMERR, EXPECT_SUCCESS},
- {OPPORTUNISTIC_TLS, GETHOSTBYNAME, Edns::FORMERR, EXPECT_FAILURE},
- {STRICT, GETHOSTBYNAME, Edns::FORMERR, EXPECT_FAILURE},
+ {OFF, GETHOSTBYNAME, Edns::FORMERR_ON_EDNS, EXPECT_SUCCESS},
+ {OPPORTUNISTIC_UDP, GETHOSTBYNAME, Edns::FORMERR_ON_EDNS, EXPECT_SUCCESS},
+ {OPPORTUNISTIC_TLS, GETHOSTBYNAME, Edns::FORMERR_ON_EDNS, EXPECT_FAILURE},
+ {STRICT, GETHOSTBYNAME, Edns::FORMERR_ON_EDNS, EXPECT_FAILURE},
{OFF, GETHOSTBYNAME, Edns::DROP, EXPECT_SUCCESS},
{OPPORTUNISTIC_UDP, GETHOSTBYNAME, Edns::DROP, EXPECT_SUCCESS},
//{OPPORTUNISTIC_TLS, GETHOSTBYNAME, Edns::DROP, EXPECT_FAILURE},
@@ -2016,10 +2016,10 @@
{OPPORTUNISTIC_UDP, GETADDRINFO, Edns::ON, EXPECT_SUCCESS},
{OPPORTUNISTIC_TLS, GETADDRINFO, Edns::ON, EXPECT_SUCCESS},
{STRICT, GETADDRINFO, Edns::ON, EXPECT_SUCCESS},
- {OFF, GETADDRINFO, Edns::FORMERR, EXPECT_SUCCESS},
- {OPPORTUNISTIC_UDP, GETADDRINFO, Edns::FORMERR, EXPECT_SUCCESS},
- {OPPORTUNISTIC_TLS, GETADDRINFO, Edns::FORMERR, EXPECT_FAILURE},
- {STRICT, GETADDRINFO, Edns::FORMERR, EXPECT_FAILURE},
+ {OFF, GETADDRINFO, Edns::FORMERR_ON_EDNS, EXPECT_SUCCESS},
+ {OPPORTUNISTIC_UDP, GETADDRINFO, Edns::FORMERR_ON_EDNS, EXPECT_SUCCESS},
+ {OPPORTUNISTIC_TLS, GETADDRINFO, Edns::FORMERR_ON_EDNS, EXPECT_FAILURE},
+ {STRICT, GETADDRINFO, Edns::FORMERR_ON_EDNS, EXPECT_FAILURE},
{OFF, GETADDRINFO, Edns::DROP, EXPECT_SUCCESS},
{OPPORTUNISTIC_UDP, GETADDRINFO, Edns::DROP, EXPECT_SUCCESS},
//{OPPORTUNISTIC_TLS, GETADDRINFO, Edns::DROP, EXPECT_FAILURE},
@@ -2092,6 +2092,72 @@
dns.stopServer();
}
+// DNS-over-TLS validation success, but server does not respond to TLS query after a while.
+// Resolver should have a reasonable number of retries instead of spinning forever. We don't have
+// an efficient way to know if resolver is stuck in an infinite loop. However, test case will be
+// failed due to timeout.
+TEST_F(ResolverTest, UnstableTls) {
+ const char CLEARTEXT_ADDR[] = "127.0.0.53";
+ const char CLEARTEXT_PORT[] = "53";
+ const char TLS_PORT[] = "853";
+ const char* host_name1 = "nonexistent1.example.com.";
+ const char* host_name2 = "nonexistent2.example.com.";
+ const std::vector<std::string> servers = {CLEARTEXT_ADDR};
+
+ test::DNSResponder dns(CLEARTEXT_ADDR, CLEARTEXT_PORT, 250, ns_rcode::ns_r_servfail);
+ ASSERT_TRUE(dns.startServer());
+ dns.setEdns(test::DNSResponder::Edns::FORMERR_ON_EDNS);
+ test::DnsTlsFrontend tls(CLEARTEXT_ADDR, TLS_PORT, CLEARTEXT_ADDR, CLEARTEXT_PORT);
+ ASSERT_TRUE(tls.startServer());
+ ASSERT_TRUE(SetResolversWithTls(servers, mDefaultSearchDomains, mDefaultParams_Binder, "", {}));
+ // Wait for validation complete.
+ EXPECT_TRUE(tls.waitForQueries(1, 5000));
+ // Shutdown TLS server to get an error. It's similar to no response case but without waiting.
+ tls.stopServer();
+
+ const hostent* h_result = gethostbyname(host_name1);
+ EXPECT_EQ(1U, GetNumQueries(dns, host_name1));
+ ASSERT_TRUE(h_result == nullptr);
+ ASSERT_EQ(HOST_NOT_FOUND, h_errno);
+
+ addrinfo hints = {.ai_family = AF_INET, .ai_socktype = SOCK_DGRAM};
+ ScopedAddrinfo ai_result = safe_getaddrinfo(host_name2, nullptr, &hints);
+ EXPECT_TRUE(ai_result == nullptr);
+ EXPECT_EQ(1U, GetNumQueries(dns, host_name2));
+}
+
+// DNS-over-TLS validation success, but server does not respond to TLS query after a while.
+// Moreover, server responds RCODE=FORMERR even on non-EDNS query.
+TEST_F(ResolverTest, BogusDnsServer) {
+ const char CLEARTEXT_ADDR[] = "127.0.0.53";
+ const char CLEARTEXT_PORT[] = "53";
+ const char TLS_PORT[] = "853";
+ const char* host_name1 = "nonexistent1.example.com.";
+ const char* host_name2 = "nonexistent2.example.com.";
+ const std::vector<std::string> servers = {CLEARTEXT_ADDR};
+
+ test::DNSResponder dns(CLEARTEXT_ADDR, CLEARTEXT_PORT, 250, ns_rcode::ns_r_servfail);
+ ASSERT_TRUE(dns.startServer());
+ test::DnsTlsFrontend tls(CLEARTEXT_ADDR, TLS_PORT, CLEARTEXT_ADDR, CLEARTEXT_PORT);
+ ASSERT_TRUE(tls.startServer());
+ ASSERT_TRUE(SetResolversWithTls(servers, mDefaultSearchDomains, mDefaultParams_Binder, "", {}));
+ // Wait for validation complete.
+ EXPECT_TRUE(tls.waitForQueries(1, 5000));
+ // Shutdown TLS server to get an error. It's similar to no response case but without waiting.
+ tls.stopServer();
+ dns.setEdns(test::DNSResponder::Edns::FORMERR_UNCOND);
+
+ const hostent* h_result = gethostbyname(host_name1);
+ EXPECT_EQ(0U, GetNumQueries(dns, host_name1));
+ ASSERT_TRUE(h_result == nullptr);
+ ASSERT_EQ(HOST_NOT_FOUND, h_errno);
+
+ addrinfo hints = {.ai_family = AF_INET, .ai_socktype = SOCK_DGRAM};
+ ScopedAddrinfo ai_result = safe_getaddrinfo(host_name2, nullptr, &hints);
+ EXPECT_TRUE(ai_result == nullptr);
+ EXPECT_EQ(0U, GetNumQueries(dns, host_name2));
+}
+
TEST_F(ResolverTest, GetAddrInfo_Dns64Synthesize) {
constexpr char listen_addr[] = "::1";
constexpr char listen_addr2[] = "127.0.0.5";