server: check interface names in RPC arguments for validity
This patch introduces a method isIfaceName that checks interface
names from various RPCs for validity before e.g. using them as
part of iptables arguments or in filenames.
All of these RPC calls can only be called from applications
with at least the CONNECTIVITY_INTERNAL permission in recent
Android versions, so the impact of the missing checks luckily
isn't very high.
Orig-Author: Jann Horn <jann@thejh.net>
Change-Id: I80df8d745a3de99ad02d6649f0d10562c81f6b98
Signed-off-by: JP Abgrall <jpa@google.com>
diff --git a/server/BandwidthController.cpp b/server/BandwidthController.cpp
index 4f9344c..e1db680 100644
--- a/server/BandwidthController.cpp
+++ b/server/BandwidthController.cpp
@@ -607,6 +607,8 @@
ALOGE("Invalid bytes value. 1..max_int64.");
return -1;
}
+ if (!isIfaceName(iface))
+ return -1;
if (StrncpyAndCheck(ifn, iface, sizeof(ifn))) {
ALOGE("Interface name longer than %d", MAX_IFACENAME_LEN);
return -1;
@@ -667,6 +669,8 @@
std::list<std::string>::iterator it;
const char *costName = "shared";
+ if (!isIfaceName(iface))
+ return -1;
if (StrncpyAndCheck(ifn, iface, sizeof(ifn))) {
ALOGE("Interface name longer than %d", MAX_IFACENAME_LEN);
return -1;
@@ -706,10 +710,8 @@
std::list<QuotaInfo>::iterator it;
std::string quotaCmd;
- if (!isNameLegal(iface)) {
- ALOGE("setInterfaceQuota: Invalid iface \"%s\"", iface);
+ if (!isIfaceName(iface))
return -1;
- }
if (!maxBytes) {
/* Don't talk about -1, deprecate it. */
@@ -775,36 +777,13 @@
return getInterfaceQuota("shared", bytes);
}
-bool BandwidthController::isNameLegal(const char* name) {
- size_t i;
- size_t name_len = strlen(name);
- if (name_len == 0) {
- return false;
- }
-
- /* First character must be alphanumeric */
- if (!isalnum(name[0])) {
- return false;
- }
-
- for (i = 1; i < name_len; i++) {
- if (!isalnum(name[i]) && (name[i] != '_') && (name[i] != '-') && (name[i] != ':')) {
- return false;
- }
- }
-
- return true;
-}
-
int BandwidthController::getInterfaceQuota(const char *costName, int64_t *bytes) {
FILE *fp;
char *fname;
int scanRes;
- if (!isNameLegal(costName)) {
- ALOGE("getInterfaceQuota: Invalid costName \"%s\"", costName);
+ if (!isIfaceName(costName))
return -1;
- }
asprintf(&fname, "/proc/net/xt_quota/%s", costName);
fp = fopen(fname, "r");
@@ -827,11 +806,8 @@
const char *costName;
std::list<QuotaInfo>::iterator it;
- if (!isNameLegal(iface)) {
- ALOGE("removeInterfaceQuota: Invalid iface \"%s\"", iface);
+ if (!isIfaceName(iface))
return -1;
- }
-
if (StrncpyAndCheck(ifn, iface, sizeof(ifn))) {
ALOGE("Interface name longer than %d", MAX_IFACENAME_LEN);
return -1;
@@ -861,7 +837,7 @@
FILE *fp;
char *fname;
- if (!isNameLegal(quotaName)) {
+ if (!isIfaceName(quotaName)) {
ALOGE("updateQuota: Invalid quotaName \"%s\"", quotaName);
return -1;
}
@@ -1040,7 +1016,7 @@
int BandwidthController::setInterfaceAlert(const char *iface, int64_t bytes) {
std::list<QuotaInfo>::iterator it;
- if (!isNameLegal(iface)) {
+ if (!isIfaceName(iface)) {
ALOGE("setInterfaceAlert: Invalid iface \"%s\"", iface);
return -1;
}
@@ -1065,7 +1041,7 @@
int BandwidthController::removeInterfaceAlert(const char *iface) {
std::list<QuotaInfo>::iterator it;
- if (!isNameLegal(iface)) {
+ if (!isIfaceName(iface)) {
ALOGE("removeInterfaceAlert: Invalid iface \"%s\"", iface);
return -1;
}
@@ -1089,7 +1065,7 @@
int res = 0;
char *alertName;
- if (!isNameLegal(costName)) {
+ if (!isIfaceName(costName)) {
ALOGE("setCostlyAlert: Invalid costName \"%s\"", costName);
return -1;
}
@@ -1119,7 +1095,7 @@
char *alertName;
int res = 0;
- if (!isNameLegal(costName)) {
+ if (!isIfaceName(costName)) {
ALOGE("removeCostlyAlert: Invalid costName \"%s\"", costName);
return -1;
}
diff --git a/server/BandwidthController.h b/server/BandwidthController.h
index 6b7b5d3..2aca2cd 100644
--- a/server/BandwidthController.h
+++ b/server/BandwidthController.h
@@ -202,7 +202,6 @@
std::list<int /*appUid*/> niceAppUids;
private:
- bool isNameLegal(const char* name);
static const char *IPT_FLUSH_COMMANDS[];
static const char *IPT_CLEANUP_COMMANDS[];
static const char *IPT_SETUP_COMMANDS[];
diff --git a/server/ClatdController.cpp b/server/ClatdController.cpp
index b91b69a..4114de1 100644
--- a/server/ClatdController.cpp
+++ b/server/ClatdController.cpp
@@ -21,6 +21,7 @@
#define LOG_TAG "ClatdController"
#include <cutils/log.h>
+#include "NetdConstants.h"
#include "ClatdController.h"
#include "Fwmark.h"
#include "NetdConstants.h"
@@ -42,6 +43,11 @@
return -1;
}
+ if (!isIfaceName(interface)) {
+ errno = ENOENT;
+ return -1;
+ }
+
ALOGD("starting clatd");
if ((pid = fork()) < 0) {
diff --git a/server/FirewallController.cpp b/server/FirewallController.cpp
index 0746316..17c6da4 100644
--- a/server/FirewallController.cpp
+++ b/server/FirewallController.cpp
@@ -69,6 +69,11 @@
}
int FirewallController::setInterfaceRule(const char* iface, FirewallRule rule) {
+ if (!isIfaceName(iface)) {
+ errno = ENOENT;
+ return -1;
+ }
+
const char* op;
if (rule == ALLOW) {
op = "-I";
diff --git a/server/IdletimerController.cpp b/server/IdletimerController.cpp
index 76b79a7..ae7ae04 100644
--- a/server/IdletimerController.cpp
+++ b/server/IdletimerController.cpp
@@ -192,6 +192,11 @@
int res;
char timeout_str[11]; //enough to store any 32-bit unsigned decimal
+ if (!isIfaceName(iface)) {
+ errno = ENOENT;
+ return -1;
+ }
+
snprintf(timeout_str, sizeof(timeout_str), "%u", timeout);
const char *cmd1[] = {
diff --git a/server/InterfaceController.cpp b/server/InterfaceController.cpp
index e55114f..73e96ee 100644
--- a/server/InterfaceController.cpp
+++ b/server/InterfaceController.cpp
@@ -111,6 +111,7 @@
*/
int InterfaceController::interfaceCommand(int argc, char *argv[], char **rbuf) {
int ret = -ENOSYS;
+ if (!isIfaceName(argv[2])) return -ENOENT;
if (sendCommand_)
ret = sendCommand_(argc, argv, rbuf);
@@ -119,6 +120,10 @@
int InterfaceController::writeIPv6ProcPath(const char *interface, const char *setting, const char *value) {
char *path;
+ if (!isIfaceName(interface)) {
+ errno = ENOENT;
+ return -1;
+ }
asprintf(&path, "%s/%s/%s", ipv6_proc_path, interface, setting);
int success = writeFile(path, value, strlen(value));
free(path);
@@ -187,18 +192,25 @@
char buf[16];
int size = sizeof(buf);
char *path;
+ if (!isIfaceName(interface)) {
+ errno = ENOENT;
+ return -1;
+ }
asprintf(&path, "%s/%s/mtu", sys_net_path, interface);
int success = readFile(path, buf, &size);
if (!success && mtu)
*mtu = atoi(buf);
free(path);
return success;
-
}
int InterfaceController::setMtu(const char *interface, const char *mtu)
{
char *path;
+ if (!isIfaceName(interface)) {
+ errno = ENOENT;
+ return -1;
+ }
asprintf(&path, "%s/%s/mtu", sys_net_path, interface);
int success = writeFile(path, mtu, strlen(mtu));
free(path);
diff --git a/server/NatController.cpp b/server/NatController.cpp
index 8391ad7..44b8b4a 100644
--- a/server/NatController.cpp
+++ b/server/NatController.cpp
@@ -134,11 +134,6 @@
return 0;
}
-bool NatController::checkInterface(const char *iface) {
- if (strlen(iface) > IFNAMSIZ) return false;
- return true;
-}
-
int NatController::routesOp(bool add, const char *intIface, const char *extIface, char **argv, int addrCount) {
unsigned netId = mNetCtrl->getNetworkId(extIface);
int ret = 0;
@@ -171,8 +166,7 @@
ALOGV("enableNat(intIface=<%s>, extIface=<%s>)",intIface, extIface);
- if (!checkInterface(intIface) || !checkInterface(extIface)) {
- ALOGE("Invalid interface specified");
+ if (!isIfaceName(intIface) || !isIfaceName(extIface)) {
errno = ENODEV;
return -1;
}
@@ -409,8 +403,7 @@
const char *intIface = argv[2];
const char *extIface = argv[3];
- if (!checkInterface(intIface) || !checkInterface(extIface)) {
- ALOGE("Invalid interface specified");
+ if (!isIfaceName(intIface) || !isIfaceName(extIface)) {
errno = ENODEV;
return -1;
}
diff --git a/server/NatController.h b/server/NatController.h
index 2912e4b..afddb27 100644
--- a/server/NatController.h
+++ b/server/NatController.h
@@ -50,7 +50,6 @@
int setDefaults();
int runCmd(int argc, const char **argv);
- bool checkInterface(const char *iface);
int setForwardRules(bool set, const char *intIface, const char *extIface);
int setTetherCountingRules(bool add, const char *intIface, const char *extIface);
int routesOp(bool add, const char *intIface, const char *extIface, char **argv, int addrCount);
diff --git a/server/NetdConstants.cpp b/server/NetdConstants.cpp
index c3c16eb..ea31410 100644
--- a/server/NetdConstants.cpp
+++ b/server/NetdConstants.cpp
@@ -17,6 +17,8 @@
#include <fcntl.h>
#include <string.h>
#include <sys/wait.h>
+#include <ctype.h>
+#include <net/if.h>
#define LOG_TAG "Netd"
@@ -143,3 +145,28 @@
close(fd);
return 0;
}
+
+/*
+ * Check an interface name for plausibility. This should e.g. help against
+ * directory traversal.
+ */
+bool isIfaceName(const char *name) {
+ size_t i;
+ size_t name_len = strlen(name);
+ if ((name_len == 0) || (name_len > IFNAMSIZ)) {
+ return false;
+ }
+
+ /* First character must be alphanumeric */
+ if (!isalnum(name[0])) {
+ return false;
+ }
+
+ for (i = 1; i < name_len; i++) {
+ if (!isalnum(name[i]) && (name[i] != '_') && (name[i] != '-') && (name[i] != ':')) {
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/server/NetdConstants.h b/server/NetdConstants.h
index e93347c..9b85d16 100644
--- a/server/NetdConstants.h
+++ b/server/NetdConstants.h
@@ -37,6 +37,7 @@
int execIptablesSilently(IptablesTarget target, ...);
int writeFile(const char *path, const char *value, int size);
int readFile(const char *path, char *buf, int *sizep);
+bool isIfaceName(const char *name);
#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*(a)))
diff --git a/server/SecondaryTableController.cpp b/server/SecondaryTableController.cpp
index 2ffb54d..398edd1 100644
--- a/server/SecondaryTableController.cpp
+++ b/server/SecondaryTableController.cpp
@@ -229,6 +229,11 @@
}
int SecondaryTableController::setFwmarkRule(const char *iface, bool add) {
+ if (!isIfaceName(iface)) {
+ errno = ENOENT;
+ return -1;
+ }
+
unsigned netId = mNetCtrl->getNetworkId(iface);
// Fail fast if any rules already exist for this interface
@@ -386,6 +391,11 @@
int SecondaryTableController::setFwmarkRoute(const char* iface, const char *dest, int prefix,
bool add) {
+ if (!isIfaceName(iface)) {
+ errno = ENOENT;
+ return -1;
+ }
+
unsigned netId = mNetCtrl->getNetworkId(iface);
char mark_str[11] = {0};
char dest_str[44]; // enough to store an IPv6 address + 3 character bitmask
diff --git a/server/TetherController.cpp b/server/TetherController.cpp
index 93110e0..fbee5a2 100644
--- a/server/TetherController.cpp
+++ b/server/TetherController.cpp
@@ -31,6 +31,7 @@
#include <cutils/log.h>
#include <cutils/properties.h>
+#include "NetdConstants.h"
#include "TetherController.h"
TetherController::TetherController() {
@@ -273,6 +274,10 @@
int TetherController::tetherInterface(const char *interface) {
ALOGD("tetherInterface(%s)", interface);
+ if (!isIfaceName(interface)) {
+ errno = ENOENT;
+ return -1;
+ }
mInterfaces->push_back(strdup(interface));
if (applyDnsInterfaces()) {