From b8087363143050d214d48e5620a330776ca95a69 Mon Sep 17 00:00:00 2001 From: Robin Lee Date: Wed, 30 Mar 2016 18:43:08 +0100 Subject: [PATCH 1/1] Server API to only allow networking by VPN apps Secure virtual networks already create rules to route all traffic into theirselves. This depends on the secure network already existing. API creates an ip rule at a priority level below SECURE_VPN which can catch traffic before VPN comes up, if it is a requirement that no traffic ever leaves without first going through VPN. Bug: 26694104 Bug: 26354134 Change-Id: If23df0760c6eb0ad137fc26c5124e48edf23b722 --- server/NetdNativeService.cpp | 28 +++++++++++ server/NetdNativeService.h | 3 +- server/RouteController.cpp | 27 +++++++++++ server/RouteController.h | 5 ++ server/UidRanges.cpp | 9 ++++ server/UidRanges.h | 4 ++ server/binder/android/net/INetd.aidl | 25 ++++++++++ tests/binder_test.cpp | 91 ++++++++++++++++++++++++++++++++++-- 8 files changed, 186 insertions(+), 6 deletions(-) diff --git a/server/NetdNativeService.cpp b/server/NetdNativeService.cpp index 97b41b2..388b6b5 100644 --- a/server/NetdNativeService.cpp +++ b/server/NetdNativeService.cpp @@ -30,6 +30,8 @@ #include "DumpWriter.h" #include "NetdConstants.h" #include "NetdNativeService.h" +#include "RouteController.h" +#include "UidRanges.h" using android::base::StringPrintf; @@ -124,5 +126,31 @@ binder::Status NetdNativeService::bandwidthEnableDataSaver(bool enable, bool *re return binder::Status::ok(); } +binder::Status NetdNativeService::networkRejectNonSecureVpn(bool add, + const std::vector& uidRangeArray) { + // TODO: elsewhere RouteController is only used from the tethering and network controllers, so + // it should be possible to use the same lock as NetworkController. However, every call through + // the CommandListener "network" command will need to hold this lock too, not just the ones that + // read/modify network internal state (that is sufficient for ::dump() because it doesn't + // look at routes, but it's not enough here). + NETD_BIG_LOCK_RPC(CONNECTIVITY_INTERNAL); + + UidRanges uidRanges; + uidRanges.createFrom(uidRangeArray); + + int err; + if (add) { + err = RouteController::addUsersToRejectNonSecureNetworkRule(uidRanges); + } else { + err = RouteController::removeUsersFromRejectNonSecureNetworkRule(uidRanges); + } + + if (err != 0) { + return binder::Status::fromServiceSpecificError(-err, + String8::format("RouteController error: %s", strerror(-err))); + } + return binder::Status::ok(); +} + } // namespace net } // namespace android diff --git a/server/NetdNativeService.h b/server/NetdNativeService.h index 57bb72b..96759e1 100644 --- a/server/NetdNativeService.h +++ b/server/NetdNativeService.h @@ -38,7 +38,8 @@ class NetdNativeService : public BinderService, public BnNetd const String16& chainName, bool isWhitelist, const std::vector& uids, bool *ret) override; binder::Status bandwidthEnableDataSaver(bool enable, bool *ret) override; - + binder::Status networkRejectNonSecureVpn(bool enable, const std::vector& uids) + override; }; } // namespace net diff --git a/server/RouteController.cpp b/server/RouteController.cpp index a087a12..1e17509 100644 --- a/server/RouteController.cpp +++ b/server/RouteController.cpp @@ -47,6 +47,7 @@ namespace { const uint32_t RULE_PRIORITY_VPN_OVERRIDE_SYSTEM = 10000; const uint32_t RULE_PRIORITY_VPN_OVERRIDE_OIF = 10500; const uint32_t RULE_PRIORITY_VPN_OUTPUT_TO_LOCAL = 11000; +const uint32_t RULE_PRIORITY_PROHIBIT_NON_VPN = 11500; const uint32_t RULE_PRIORITY_SECURE_VPN = 12000; const uint32_t RULE_PRIORITY_EXPLICIT_NETWORK = 13000; const uint32_t RULE_PRIORITY_OUTPUT_INTERFACE = 14000; @@ -768,6 +769,24 @@ WARN_UNUSED_RESULT int modifyPhysicalNetwork(unsigned netId, const char* interfa return modifyImplicitNetworkRule(netId, table, permission, add); } +WARN_UNUSED_RESULT int modifyRejectNonSecureNetworkRule(const UidRanges& uidRanges, bool add) { + Fwmark fwmark; + Fwmark mask; + fwmark.protectedFromVpn = false; + mask.protectedFromVpn = true; + + for (const UidRanges::Range& range : uidRanges.getRanges()) { + if (int ret = modifyIpRule(add ? RTM_NEWRULE : RTM_DELRULE, + RULE_PRIORITY_PROHIBIT_NON_VPN, FR_ACT_PROHIBIT, RT_TABLE_UNSPEC, + fwmark.intValue, mask.intValue, IIF_LOOPBACK, OIF_NONE, + range.first, range.second)) { + return ret; + } + } + + return 0; +} + WARN_UNUSED_RESULT int modifyVirtualNetwork(unsigned netId, const char* interface, const UidRanges& uidRanges, bool secure, bool add, bool modifyNonUidBasedRules) { @@ -1045,6 +1064,14 @@ int RouteController::modifyPhysicalNetworkPermission(unsigned netId, const char* return modifyPhysicalNetwork(netId, interface, oldPermission, ACTION_DEL); } +int RouteController::addUsersToRejectNonSecureNetworkRule(const UidRanges& uidRanges) { + return modifyRejectNonSecureNetworkRule(uidRanges, true); +} + +int RouteController::removeUsersFromRejectNonSecureNetworkRule(const UidRanges& uidRanges) { + return modifyRejectNonSecureNetworkRule(uidRanges, false); +} + int RouteController::addUsersToVirtualNetwork(unsigned netId, const char* interface, bool secure, const UidRanges& uidRanges) { return modifyVirtualNetwork(netId, interface, uidRanges, secure, ACTION_ADD, diff --git a/server/RouteController.h b/server/RouteController.h index 0694ea2..f1affe3 100644 --- a/server/RouteController.h +++ b/server/RouteController.h @@ -61,6 +61,11 @@ public: static int removeUsersFromVirtualNetwork(unsigned netId, const char* interface, bool secure, const UidRanges& uidRanges) WARN_UNUSED_RESULT; + static int addUsersToRejectNonSecureNetworkRule(const UidRanges& uidRanges) + WARN_UNUSED_RESULT; + static int removeUsersFromRejectNonSecureNetworkRule(const UidRanges& uidRanges) + WARN_UNUSED_RESULT; + static int addInterfaceToDefaultNetwork(const char* interface, Permission permission) WARN_UNUSED_RESULT; static int removeInterfaceFromDefaultNetwork(const char* interface, diff --git a/server/UidRanges.cpp b/server/UidRanges.cpp index 64c1b45..a2b8dde 100644 --- a/server/UidRanges.cpp +++ b/server/UidRanges.cpp @@ -75,6 +75,15 @@ bool UidRanges::parseFrom(int argc, char* argv[]) { return true; } +void UidRanges::createFrom(const std::vector& ranges) { + mRanges.resize(ranges.size()); + std::transform(ranges.begin(), ranges.end(), mRanges.begin(), + [](const android::net::UidRange& range) { + return Range(range.getStart(), range.getStop()); + }); + std::sort(mRanges.begin(), mRanges.end()); +} + void UidRanges::add(const UidRanges& other) { auto middle = mRanges.insert(mRanges.end(), other.mRanges.begin(), other.mRanges.end()); std::inplace_merge(mRanges.begin(), middle, mRanges.end()); diff --git a/server/UidRanges.h b/server/UidRanges.h index 1c40a5b..3cbbe80 100644 --- a/server/UidRanges.h +++ b/server/UidRanges.h @@ -25,12 +25,16 @@ class UidRanges { public: + // TODO: replace with AIDL type: android::net::UidRange + // int32_t may not be a safe replacement for uid_t. If not, UidRange will need to change to use + // a larger type first. typedef std::pair Range; bool hasUid(uid_t uid) const; const std::vector& getRanges() const; bool parseFrom(int argc, char* argv[]); + void createFrom(const std::vector& ranges); std::string toString() const; void add(const UidRanges& other); diff --git a/server/binder/android/net/INetd.aidl b/server/binder/android/net/INetd.aidl index cca5b58..63054f8 100644 --- a/server/binder/android/net/INetd.aidl +++ b/server/binder/android/net/INetd.aidl @@ -16,6 +16,8 @@ package android.net; +import android.net.UidRange; + /** {@hide} */ interface INetd { /** @@ -55,4 +57,27 @@ interface INetd { * @return true if the if the operation was successful, false otherwise. */ boolean bandwidthEnableDataSaver(boolean enable); + + /** + * Adds or removes one rule for each supplied UID range to prohibit all network activity outside + * of secure VPN. + * + * When a UID is covered by one of these rules, traffic sent through any socket that is not + * protected or explicitly overriden by the system will be rejected. The kernel will respond + * with an ICMP prohibit message. + * + * Initially, there are no such rules. Any rules that are added will only last until the next + * restart of netd or the device. + * + * @param add {@code true} if the specified UID ranges should be denied access to any network + * which is not secure VPN by adding rules, {@code false} to remove existing rules. + * @param uidRanges a set of non-overlapping, contiguous ranges of UIDs to which to apply or + * remove this restriction. + *

Added rules should not overlap with existing rules. Likewise, removed rules should + * each correspond to an existing rule. + * + * @throws ServiceSpecificException in case of failure, with an error code corresponding to the + * unix errno. + */ + void networkRejectNonSecureVpn(boolean add, in UidRange[] uidRanges); } diff --git a/tests/binder_test.cpp b/tests/binder_test.cpp index bc9cf88..94cc535 100644 --- a/tests/binder_test.cpp +++ b/tests/binder_test.cpp @@ -16,6 +16,8 @@ * binder_test.cpp - unit tests for netd binder RPCs. */ +#include +#include #include #include #include @@ -23,17 +25,23 @@ #include #include +#include #include #include #include "NetdConstants.h" #include "android/net/INetd.h" +#include "android/net/UidRange.h" #include "binder/IServiceManager.h" using namespace android; using namespace android::base; using namespace android::binder; using android::net::INetd; +using android::net::UidRange; + +static const char* IP_RULE_V4 = "-4"; +static const char* IP_RULE_V6 = "-6"; class BinderTest : public ::testing::Test { @@ -77,19 +85,19 @@ static int randomUid() { return 100000 * arc4random_uniform(7) + 10000 + arc4random_uniform(5000); } -static std::vector listIptablesRule(const char *binary, const char *chainName) { +static std::vector runCommand(const std::string& command) { std::vector lines; FILE *f; - std::string command = StringPrintf("%s -n -L %s", binary, chainName); if ((f = popen(command.c_str(), "r")) == nullptr) { perror("popen"); return lines; } char *line = nullptr; - size_t linelen = 0; - while (getline(&line, &linelen, f) >= 0) { + size_t bufsize = 0; + ssize_t linelen = 0; + while ((linelen = getline(&line, &bufsize, f)) >= 0) { lines.push_back(std::string(line, linelen)); free(line); line = nullptr; @@ -99,11 +107,20 @@ static std::vector listIptablesRule(const char *binary, const char return lines; } +static std::vector listIpRules(const char *ipVersion) { + std::string command = StringPrintf("%s %s rule list", IP_PATH, ipVersion); + return runCommand(command); +} + +static std::vector listIptablesRule(const char *binary, const char *chainName) { + std::string command = StringPrintf("%s -n -L %s", binary, chainName); + return runCommand(command); +} + static int iptablesRuleLineLength(const char *binary, const char *chainName) { return listIptablesRule(binary, chainName).size(); } - TEST_F(BinderTest, TestFirewallReplaceUidChain) { std::string chainName = StringPrintf("netd_binder_test_%u", arc4random_uniform(10000)); const int kNumUids = 500; @@ -210,3 +227,67 @@ TEST_F(BinderTest, TestBandwidthEnableDataSaver) { EXPECT_EQ(0, getDataSaverState()); } } + +static bool ipRuleExistsForRange(const uint32_t priority, const UidRange& range, + const std::string& action, const char* ipVersion) { + // Output looks like this: + // "11500:\tfrom all fwmark 0x0/0x20000 iif lo uidrange 1000-2000 prohibit" + std::vector rules = listIpRules(ipVersion); + + std::string prefix = StringPrintf("%" PRIu32 ":", priority); + std::string suffix = StringPrintf(" iif lo uidrange %d-%d %s\n", + range.getStart(), range.getStop(), action.c_str()); + for (std::string line : rules) { + if (android::base::StartsWith(line, prefix.c_str()) + && android::base::EndsWith(line, suffix.c_str())) { + return true; + } + } + return false; +} + +static bool ipRuleExistsForRange(const uint32_t priority, const UidRange& range, + const std::string& action) { + bool existsIp4 = ipRuleExistsForRange(priority, range, action, IP_RULE_V4); + bool existsIp6 = ipRuleExistsForRange(priority, range, action, IP_RULE_V6); + EXPECT_EQ(existsIp4, existsIp6); + return existsIp4; +} + +TEST_F(BinderTest, TestNetworkRejectNonSecureVpn) { + constexpr uint32_t RULE_PRIORITY = 11500; + + constexpr int baseUid = MULTIUSER_APP_PER_USER_RANGE * 5; + std::vector uidRanges = { + {baseUid + 150, baseUid + 224}, + {baseUid + 226, baseUid + 300} + }; + + const std::vector initialRulesV4 = listIpRules(IP_RULE_V4); + const std::vector initialRulesV6 = listIpRules(IP_RULE_V6); + + // Create two valid rules. + ASSERT_TRUE(mNetd->networkRejectNonSecureVpn(true, uidRanges).isOk()); + EXPECT_EQ(initialRulesV4.size() + 2, listIpRules(IP_RULE_V4).size()); + EXPECT_EQ(initialRulesV6.size() + 2, listIpRules(IP_RULE_V6).size()); + for (auto const& range : uidRanges) { + EXPECT_TRUE(ipRuleExistsForRange(RULE_PRIORITY, range, "prohibit")); + } + + // Remove the rules. + ASSERT_TRUE(mNetd->networkRejectNonSecureVpn(false, uidRanges).isOk()); + EXPECT_EQ(initialRulesV4.size(), listIpRules(IP_RULE_V4).size()); + EXPECT_EQ(initialRulesV6.size(), listIpRules(IP_RULE_V6).size()); + for (auto const& range : uidRanges) { + EXPECT_FALSE(ipRuleExistsForRange(RULE_PRIORITY, range, "prohibit")); + } + + // Fail to remove the rules a second time after they are already deleted. + binder::Status status = mNetd->networkRejectNonSecureVpn(false, uidRanges); + ASSERT_EQ(binder::Status::EX_SERVICE_SPECIFIC, status.exceptionCode()); + EXPECT_EQ(ENOENT, status.serviceSpecificErrorCode()); + + // All rules should be the same as before. + EXPECT_EQ(initialRulesV4, listIpRules(IP_RULE_V4)); + EXPECT_EQ(initialRulesV6, listIpRules(IP_RULE_V6)); +} -- 2.11.0