Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 0a015535 authored by Ken Chen's avatar Ken Chen
Browse files

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
parent 981ee9a6

dns_responder/dns_responder.cpp

+8 −2
Original line number Diff line number Diff line
@@ -776,11 +776,17 @@ bool DNSResponder::handleDNSRequest(const char* buffer, ssize_t len, 
        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.

dns_responder/dns_responder.h

+6 −5
Original line number Diff line number Diff line
@@ -49,7 +49,8 @@ class DNSResponder { 


    enum class Edns : uint8_t {

        ON,

        FORMERR,  // DNS server not supporting EDNS will reply FORMERR.

        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.

    };
@@ -126,9 +127,9 @@ class DNSResponder { 


    // 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

getaddrinfo.cpp

+7 −10
Original line number Diff line number Diff line
@@ -1606,11 +1606,9 @@ static int res_queryN(const char* name, res_target* target, res_state res, int* 
    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 @@ static int res_queryN(const char* name, res_target* target, res_state res, int* 
        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 @@ static int res_queryN(const char* name, res_target* target, res_state res, int* 
            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

res_query.cpp

+4 −7
Original line number Diff line number Diff line
@@ -117,10 +117,8 @@ int res_nquery(res_state statp, const char* name, // domain name 
    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 @@ again: 
#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 @@ again: 
    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

resolver_test.cpp

+78 −12
Original line number Diff line number Diff line
@@ -1945,9 +1945,9 @@ TEST_F(ResolverTest, Async_NoRetryFlag) { 
}



// 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 @@ TEST_F(ResolverTest, BrokenEdns) { 
                case Edns::ON:

                    ednsString = "ednsOn";

                    break;

                case Edns::FORMERR:

                case Edns::FORMERR_ON_EDNS:

                    ednsString = "ednsFormerr";

                    break;

                case Edns::DROP:
@@ -2004,10 +2004,10 @@ TEST_F(ResolverTest, BrokenEdns) { 
            {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 @@ TEST_F(ResolverTest, BrokenEdns) { 
            {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 @@ TEST_F(ResolverTest, BrokenEdns) { 
    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";