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 3991974b8cf5671b49f39531a6deb9f6fe81ebf0)
diff --git a/Android.bp b/Android.bp
index b1aaef8..016b112 100644
--- a/Android.bp
+++ b/Android.bp
@@ -262,6 +262,7 @@
],
static_libs: [
"dnsresolver_aidl_interface-unstable-ndk_platform",
+ "netd_aidl_interface-ndk_platform",
"netd_event_listener_interface-ndk_platform",
"libcutils",
"libgmock",
diff --git a/res_send.cpp b/res_send.cpp
index 2b84e82..6646310 100644
--- a/res_send.cpp
+++ b/res_send.cpp
@@ -411,6 +411,15 @@
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 @@
// 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, ¶ms, 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 @@
// queries that deterministically fail (e.g., a name that always returns
// SERVFAIL or times out) do not unduly affect the stats.
if (shouldRecordStats) {
- 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.
- // 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);
+ // (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.
+ // 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_stats_add(statp->netid, receivedServerAddr, dnsQueryEvent);
}
diff --git a/tests/Android.bp b/tests/Android.bp
index efdcc6f..4a258cb 100644
--- a/tests/Android.bp
+++ b/tests/Android.bp
@@ -11,8 +11,10 @@
"libutils",
],
static_libs: [
+ "netd_aidl_interface-ndk_platform",
"libnetd_test_dnsresponder_ndk",
"libnetdutils",
+ "libgmock",
],
}
diff --git a/tests/resolv_integration_test.cpp b/tests/resolv_integration_test.cpp
index a2a694e..bf26174 100644
--- a/tests/resolv_integration_test.cpp
+++ b/tests/resolv_integration_test.cpp
@@ -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 @@
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 @@
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 @@
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);
+}
diff --git a/tests/resolv_test_utils.h b/tests/resolv_test_utils.h
index 2177510..21d2936 100644
--- a/tests/resolv_test_utils.h
+++ b/tests/resolv_test_utils.h
@@ -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 @@
// 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";