Fix netd crash in checkUserNetworkAccessLocked()
SIGABRT happens when unprivileged applications call Network.bindSocket
with a local network (netId=99). Netd statically cast a LocalNetwork
object to PhysicalNetwork, call member function getPermission().
However, the function exists in PhysicalNetwork only.
This commit makes getPermission() as a pure virtual function in the
ancestor class and implements it in each subclass.
Bug: 255954808
Test: atest netdclient_root_test
Change-Id: Ibe1eabe6ff4dc4ee124b820d596bb16c3fe371b2
diff --git a/TEST_MAPPING b/TEST_MAPPING
index 5e200e9..4e5a047 100644
--- a/TEST_MAPPING
+++ b/TEST_MAPPING
@@ -3,6 +3,7 @@
{ "name": "netd_integration_test" },
{ "name": "netd_unit_test" },
{ "name": "netdclient_test" },
+ { "name": "netdclient_root_test" },
{ "name": "netdutils_test" }
],
"postsubmit": [
@@ -18,6 +19,7 @@
{ "name": "netd_integration_test" },
{ "name": "netd_unit_test" },
{ "name": "netdclient_test" },
+ { "name": "netdclient_root_test" },
{ "name": "netdutils_test" }
]
}
diff --git a/client/Android.bp b/client/Android.bp
index 22cb5c1..528b354 100644
--- a/client/Android.bp
+++ b/client/Android.bp
@@ -65,3 +65,28 @@
recover: [ "all" ],
},
}
+
+cc_test {
+ name: "netdclient_root_test",
+ require_root: true, // for ScopedUidChange
+ srcs: [
+ "NetdClientRootTest.cpp",
+ ],
+ defaults: [
+ "netd_aidl_interface_lateststable_cpp_static",
+ "netd_defaults",
+ ],
+ test_suites: ["device-tests"],
+ include_dirs: [
+ "system/netd/include",
+ ],
+ static_libs: [
+ "libbase",
+ "libnetd_client",
+ "libnetd_test_utils",
+ ],
+ sanitize: {
+ address: false,
+ recover: [ "all" ],
+ },
+}
diff --git a/client/NetdClientRootTest.cpp b/client/NetdClientRootTest.cpp
new file mode 100644
index 0000000..3ecbcef
--- /dev/null
+++ b/client/NetdClientRootTest.cpp
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <android-base/unique_fd.h>
+#include <gtest/gtest.h>
+
+#include "NetdClient.h"
+#include "android/net/INetd.h"
+#include "netid_client.h"
+#include "test_utils.h"
+
+constexpr int TEST_UID1 = 99999;
+
+TEST(NetdClientTest, setSocketToInvalidNetwork) {
+ const android::base::unique_fd s(socket(AF_INET6, SOCK_STREAM | SOCK_CLOEXEC, 0));
+ ASSERT_LE(0, s);
+
+ unsigned netId = NETID_UNSET;
+ const ScopedUidChange scopedUidChange(TEST_UID1);
+ EXPECT_EQ(-EACCES, setNetworkForSocket(android::net::INetd::LOCAL_NET_ID, s));
+ EXPECT_EQ(0, getNetworkForSocket(&netId, s));
+ EXPECT_EQ(static_cast<unsigned>(NETID_UNSET), netId);
+}
diff --git a/server/DummyNetwork.h b/server/DummyNetwork.h
index 8f9960b..e699dad 100644
--- a/server/DummyNetwork.h
+++ b/server/DummyNetwork.h
@@ -25,6 +25,7 @@
static const char* INTERFACE_NAME;
explicit DummyNetwork(unsigned netId);
virtual ~DummyNetwork();
+ Permission getPermission() const { return PERMISSION_SYSTEM; };
private:
std::string getTypeString() const override { return "DUMMY"; };
diff --git a/server/LocalNetwork.h b/server/LocalNetwork.h
index c774067..af56d24 100644
--- a/server/LocalNetwork.h
+++ b/server/LocalNetwork.h
@@ -24,6 +24,7 @@
public:
explicit LocalNetwork(unsigned netId);
virtual ~LocalNetwork();
+ Permission getPermission() const { return PERMISSION_SYSTEM; };
private:
std::string getTypeString() const override { return "LOCAL"; };
diff --git a/server/Network.h b/server/Network.h
index dfead17..6c3d01d 100644
--- a/server/Network.h
+++ b/server/Network.h
@@ -17,6 +17,7 @@
#pragma once
#include "NetdConstants.h"
+#include "Permission.h"
#include "UidRanges.h"
#include <set>
@@ -48,6 +49,7 @@
std::string toString() const;
std::string uidRangesToString() const;
bool appliesToUser(uid_t uid, int32_t* subPriority) const;
+ virtual Permission getPermission() const = 0;
[[nodiscard]] virtual int addUsers(const UidRanges&, int32_t /*subPriority*/) {
return -EINVAL;
};
diff --git a/server/NetworkController.cpp b/server/NetworkController.cpp
index f144139..ca9ec22 100644
--- a/server/NetworkController.cpp
+++ b/server/NetworkController.cpp
@@ -887,7 +887,7 @@
// Check whether the UID's permission bits are sufficient to use the network.
// Because the permission of the system default network is PERMISSION_NONE(0x0), apps can always
// pass the check here when using the system default network.
- Permission networkPermission = static_cast<PhysicalNetwork*>(network)->getPermission();
+ const Permission networkPermission = network->getPermission();
return ((userPermission & networkPermission) == networkPermission) ? 0 : -EACCES;
}
diff --git a/server/UnreachableNetwork.h b/server/UnreachableNetwork.h
index d2cefde..0ffc0ce 100644
--- a/server/UnreachableNetwork.h
+++ b/server/UnreachableNetwork.h
@@ -23,6 +23,7 @@
class UnreachableNetwork : public Network {
public:
explicit UnreachableNetwork(unsigned netId);
+ Permission getPermission() const { return PERMISSION_SYSTEM; };
[[nodiscard]] int addUsers(const UidRanges& uidRanges, int32_t subPriority) override;
[[nodiscard]] int removeUsers(const UidRanges& uidRanges, int32_t subPriority) override;
bool isUnreachable() override { return true; }
diff --git a/server/VirtualNetwork.h b/server/VirtualNetwork.h
index 63bc589..6e9bdad 100644
--- a/server/VirtualNetwork.h
+++ b/server/VirtualNetwork.h
@@ -35,6 +35,7 @@
public:
explicit VirtualNetwork(unsigned netId, bool secure, bool excludeLocalRoutes = false);
virtual ~VirtualNetwork();
+ Permission getPermission() const { return PERMISSION_SYSTEM; };
[[nodiscard]] int addUsers(const UidRanges& uidRanges, int32_t subPriority) override;
[[nodiscard]] int removeUsers(const UidRanges& uidRanges, int32_t subPriority) override;
bool isVirtual() override { return true; }
