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

Commit 8c45bfb8 authored by Luke Huang's avatar Luke Huang
Browse files

Skip the legacy stats recording if DNS requests are denied by network policy 

In order to prevent the false INTERNAL_ERROR event which is caused by
network policy fill up the legacy DNS stats, skip adding new legacy DNS
stats when we got errno == EPERM.

Bug: 151166599
Test: atest
Merged-In: I4c1d2a5c9f43650a5346a5ee0bb88eb36f850096

Change-Id: I5486638556c0a8fdfabbc37a1b29ed2b42f9b399
(cherry picked from commit 3991974b)
parent b9a10a82

Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -262,6 +262,7 @@ cc_test { 
    ],

    static_libs: [

        "dnsresolver_aidl_interface-unstable-ndk_platform",

        "netd_aidl_interface-ndk_platform",

        "netd_event_listener_interface-ndk_platform",

        "libcutils",

        "libgmock",

res_send.cpp

+24 −9
Original line number Diff line number Diff line
@@ -411,6 +411,15 @@ static DnsQueryEvent* addDnsQueryEvent(NetworkDnsEventReported* event) { 
    return event->mutable_dns_query_events()->add_dns_query_event();

}



static bool isNetworkRestricted(int terrno) {

    // It's possible that system was in some network restricted mode, which blocked

    // the operation of sending packet and resulted in EPERM errno.

    // It would be no reason to keep retrying on that case.

    // TODO: Check the system status to know if network restricted mode is

    // enabled.

    return (terrno == EPERM);

}



int res_nsend(res_state statp, const uint8_t* buf, int buflen, uint8_t* ans, int anssiz, int* rcode,

              uint32_t flags, std::chrono::milliseconds sleepTimeMs) {

    LOG(DEBUG) << __func__;
@@ -537,14 +546,14 @@ int res_nsend(res_state statp, const uint8_t* buf, int buflen, uint8_t* ans, int 
                    // TCP fallback retry and current server does not support TCP connectin

                    useTcp = false;

                }

                LOG(INFO) << __func__ << ": used send_vc " << resplen;

                LOG(INFO) << __func__ << ": used send_vc " << resplen << " terrno: " << terrno;

            } else {

                // UDP

                resplen = send_dg(statp, &params, buf, buflen, ans, anssiz, &terrno, &actualNs,

                                  &useTcp, &gotsomewhere, &query_time, rcode, &delay);

                fallbackTCP = useTcp ? true : false;

                retry_count_for_event = attempt;

                LOG(INFO) << __func__ << ": used send_dg " << resplen;

                LOG(INFO) << __func__ << ": used send_dg " << resplen << " terrno: " << terrno;

            }



            const IPSockAddr& receivedServerAddr = statp->nsaddrs[actualNs];
@@ -568,13 +577,19 @@ int res_nsend(res_state statp, const uint8_t* buf, int buflen, uint8_t* ans, int 
            // queries that deterministically fail (e.g., a name that always returns

            // SERVFAIL or times out) do not unduly affect the stats.

            if (shouldRecordStats) {

                // (b/151166599): This is a workaround to prevent that DnsResolver calculates the

                // reliability of DNS servers from being broken when network restricted mode is

                // enabled.

                // TODO: Introduce the new server selection instead of skipping stats recording.

                if (!isNetworkRestricted(terrno)) {

                    res_sample sample;

                    res_stats_set_sample(&sample, query_time, *rcode, delay);

                // KeepListening UDP mechanism is incompatible with usable_servers of legacy stats,

                // so keep the old logic for now.

                    // KeepListening UDP mechanism is incompatible with usable_servers of legacy

                    // stats, so keep the old logic for now.

                    // TODO: Replace usable_servers of legacy stats with new one.

                resolv_cache_add_resolver_stats_sample(statp->netid, revision_id, serverSockAddr,

                                                       sample, params.max_samples);

                    resolv_cache_add_resolver_stats_sample(

                            statp->netid, revision_id, serverSockAddr, sample, params.max_samples);

                }

                resolv_stats_add(statp->netid, receivedServerAddr, dnsQueryEvent);

            }

tests/Android.bp

+2 −0
Original line number Diff line number Diff line
@@ -11,8 +11,10 @@ cc_test_library { 
        "libutils",

    ],

    static_libs: [

        "netd_aidl_interface-ndk_platform",

        "libnetd_test_dnsresponder_ndk",

        "libnetdutils",

        "libgmock",

    ],

}

tests/resolv_integration_test.cpp

+36 −31
Original line number Diff line number Diff line
@@ -73,9 +73,6 @@ 
constexpr int TEST_VPN_NETID = 65502;

constexpr int MAXPACKET = (8 * 1024);



// Use maximum reserved appId for applications to avoid conflict with existing uids.

static const int TEST_UID = 99999;



// Semi-public Bionic hook used by the NDK (frameworks/base/native/android/net.c)

// Tested here for convenience.

extern "C" int android_getaddrinfofornet(const char* hostname, const char* servname,
@@ -4004,24 +4001,7 @@ TEST_F(ResolverTest, BlockDnsQueryWithUidRule) { 
    dns1.clearQueries();

    dns2.clearQueries();



    // Add drop rule for TEST_UID. Also enable the standby chain because it might not be enabled.

    // Unfortunately we cannot use FIREWALL_CHAIN_NONE, or custom iptables rules, for this purpose

    // because netd calls fchown() on the DNS query sockets, and "iptables -m owner" matches the

    // UID of the socket creator, not the UID set by fchown().

    //

    // TODO: migrate FIREWALL_CHAIN_NONE to eBPF as well.

    EXPECT_TRUE(netdService->firewallEnableChildChain(INetd::FIREWALL_CHAIN_STANDBY, true).isOk());

    EXPECT_TRUE(netdService

                        ->firewallSetUidRule(INetd::FIREWALL_CHAIN_STANDBY, TEST_UID,

                                             INetd::FIREWALL_RULE_DENY)

                        .isOk());



    // Save uid

    int suid = getuid();



    // Switch to TEST_UID

    EXPECT_TRUE(seteuid(TEST_UID) == 0);



    ScopeBlockedUIDRule scopeBlockUidRule(netdService, TEST_UID);

    // Dns Query

    int fd1 = resNetworkQuery(TEST_NETID, host_name, ns_c_in, ns_t_a, 0);

    int fd2 = resNetworkQuery(TEST_NETID, host_name, ns_c_in, ns_t_aaaa, 0);
@@ -4036,16 +4016,6 @@ TEST_F(ResolverTest, BlockDnsQueryWithUidRule) { 
    memset(buf, 0, MAXPACKET);

    res = getAsyncResponse(fd1, &rcode, buf, MAXPACKET);

    EXPECT_EQ(-ECONNREFUSED, res);



    // Restore uid

    EXPECT_TRUE(seteuid(suid) == 0);



    // Remove drop rule for TEST_UID, and disable the standby chain.

    EXPECT_TRUE(netdService

                        ->firewallSetUidRule(INetd::FIREWALL_CHAIN_STANDBY, TEST_UID,

                                             INetd::FIREWALL_RULE_ALLOW)

                        .isOk());

    EXPECT_TRUE(netdService->firewallEnableChildChain(INetd::FIREWALL_CHAIN_STANDBY, false).isOk());

}



namespace {
@@ -4996,3 +4966,38 @@ TEST_F(ResolverTest, GetAddrInfoParallelLookupSleepTime) { 
    EXPECT_GT(PARALLEL_LOOKUP_SLEEP_TIME_MS, timeTakenMs);

    EXPECT_EQ(0U, GetNumQueries(dns, kHelloExampleCom));

}



TEST_F(ResolverTest, BlockDnsQueryUidDoesNotLeadToBadServer) {

    // This test relies on blocking traffic on loopback, which xt_qtaguid does not do.

    // See aosp/358413 and b/34444781 for why.

    SKIP_IF_BPF_NOT_SUPPORTED;



    constexpr char listen_addr1[] = "127.0.0.4";

    constexpr char listen_addr2[] = "::1";

    test::DNSResponder dns1(listen_addr1);

    test::DNSResponder dns2(listen_addr2);

    StartDns(dns1, {});

    StartDns(dns2, {});



    std::vector<std::string> servers = {listen_addr1, listen_addr2};

    ASSERT_TRUE(mDnsClient.SetResolversForNetwork(servers));

    dns1.clearQueries();

    dns2.clearQueries();

    {

        ScopeBlockedUIDRule scopeBlockUidRule(mDnsClient.netdService(), TEST_UID);

        // Start querying ten times.

        for (int i = 0; i < 10; i++) {

            std::string hostName = fmt::format("blocked{}.com", i);

            const addrinfo hints = {.ai_family = AF_INET, .ai_socktype = SOCK_DGRAM};

            EXPECT_EQ(safe_getaddrinfo(hostName.c_str(), nullptr, &hints), nullptr);

        }

    }

    // Since all query packets are blocked, we should not see any stats of them.

    const std::vector<NameserverStats> expectedEmptyDnsStats = {

            NameserverStats(listen_addr1),

            NameserverStats(listen_addr2),

    };

    expectStatsFromGetResolverInfo(expectedEmptyDnsStats);

    EXPECT_EQ(dns1.queries().size(), 0U);

    EXPECT_EQ(dns2.queries().size(), 0U);

}

tests/resolv_test_utils.h

+37 −0
Original line number Diff line number Diff line
@@ -23,10 +23,45 @@ 
#include <string>

#include <vector>



#include <aidl/android/net/INetd.h>

#include <gtest/gtest.h>

#include <netdutils/InternetAddresses.h>



#include "dns_responder/dns_responder.h"



class ScopeBlockedUIDRule {

    using INetd = aidl::android::net::INetd;



  public:

    ScopeBlockedUIDRule(INetd* netSrv, uid_t testUid)

        : mNetSrv(netSrv), mTestUid(testUid), mSavedUid(getuid()) {

        // Add drop rule for testUid. Also enable the standby chain because it might not be

        // enabled. Unfortunately we cannot use FIREWALL_CHAIN_NONE, or custom iptables rules, for

        // this purpose because netd calls fchown() on the DNS query sockets, and "iptables -m

        // owner" matches the UID of the socket creator, not the UID set by fchown().

        // TODO: migrate FIREWALL_CHAIN_NONE to eBPF as well.

        EXPECT_TRUE(mNetSrv->firewallEnableChildChain(INetd::FIREWALL_CHAIN_STANDBY, true).isOk());

        EXPECT_TRUE(mNetSrv->firewallSetUidRule(INetd::FIREWALL_CHAIN_STANDBY, mTestUid,

                                                INetd::FIREWALL_RULE_DENY)

                            .isOk());

        EXPECT_TRUE(seteuid(mTestUid) == 0);

    };

    ~ScopeBlockedUIDRule() {

        // Restore uid

        EXPECT_TRUE(seteuid(mSavedUid) == 0);

        // Remove drop rule for testUid, and disable the standby chain.

        EXPECT_TRUE(mNetSrv->firewallSetUidRule(INetd::FIREWALL_CHAIN_STANDBY, mTestUid,

                                                INetd::FIREWALL_RULE_ALLOW)

                            .isOk());

        EXPECT_TRUE(mNetSrv->firewallEnableChildChain(INetd::FIREWALL_CHAIN_STANDBY, false).isOk());

    }



  private:

    INetd* mNetSrv;

    const uid_t mTestUid;

    const uid_t mSavedUid;

};



struct DnsRecord {

    std::string host_name;  // host name

    ns_type type;           // record type
@@ -35,6 +70,8 @@ struct DnsRecord { 


// TODO: make this dynamic and stop depending on implementation details.

constexpr int TEST_NETID = 30;

// Use maximum reserved appId for applications to avoid conflict with existing uids.

constexpr int TEST_UID = 99999;



static constexpr char kLocalHost[] = "localhost";

static constexpr char kLocalHostAddr[] = "127.0.0.1";