Hi,
batctl has a "root" user check since the beginning of this tool. It even got a TODO comment in 58525b8330e9 ("batctl: adjust vis.c coding style"):
/* TODO: remove this generic check here and move it into the individual functions */
This patchset is now trying to resolve this TODO.
* batctl: Move root privileges check in separate function * batctl: Use geteuid for checks of root privileges * batctl: Return type of error on netlink_get_info error * batctl: Make root privileges check function specific * batctl: Allow to retrieve interface stats as non-root * batctl: Allow to read loglevel as normal user * batctl: Allow to read gw_mode as normal user * batctl: Allow to read sysfs settings as normal user * batctl: Allow to read list of interfaces as normal user
"Return type of error on netlink_get_info error" may look a little bit odd in this list. But it was required in my initial test to avoid that "originators" and "gwl" try to start a debugfs access when the netlink function should have returned a "-EPERM" (but did return a -EOPNOTSUPP).
Kind regards, Sven
Signed-off-by: Sven Eckelmann sven@narfation.org --- functions.c | 8 ++++++++ functions.h | 1 + main.c | 6 ++---- 3 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/functions.c b/functions.c index f8bacda..c1aaf12 100644 --- a/functions.c +++ b/functions.c @@ -1138,3 +1138,11 @@ void get_random_bytes(void *buf, size_t buflen)
get_random_bytes_fallback(buf, buflen); } + +void check_root_or_die(const char *cmd) +{ + if (getuid() || getgid()) { + fprintf(stderr, "Error - you must be root to run '%s' !\n", cmd); + exit(EXIT_FAILURE); + } +} diff --git a/functions.h b/functions.h index b085f9d..eca1406 100644 --- a/functions.h +++ b/functions.h @@ -54,6 +54,7 @@ int check_mesh_iface(char *mesh_iface); int check_mesh_iface_ownership(char *mesh_iface, char *hard_iface);
void get_random_bytes(void *buf, size_t buflen); +void check_root_or_die(const char *cmd);
int print_routing_algos(void); extern char *line_ptr; diff --git a/main.c b/main.c index 3f0b008..2fc9b75 100644 --- a/main.c +++ b/main.c @@ -136,10 +136,8 @@ int main(int argc, char **argv)
/* TODO: remove this generic check here and move it into the individual functions */ /* check if user is root */ - if ((strncmp(argv[1], "bisect", strlen("bisect")) != 0) && ((getuid()) || (getgid()))) { - fprintf(stderr, "Error - you must be root to run '%s' !\n", argv[0]); - exit(EXIT_FAILURE); - } + if (strncmp(argv[1], "bisect", strlen("bisect")) != 0) + check_root_or_die(argv[0]);
if ((strcmp(argv[1], "interface") == 0) || (strcmp(argv[1], "if") == 0)) {
The getuid only shows the UID of the current user. But the EUID is relevant for accessing the debugfs and sysfs files. This check is still to strict when it comes to netlink but avoids a lot of bogus warnings like
Error received: Operation not permitted Error - mesh has not been enabled yet Activate your mesh by adding interfaces to batman-adv
when only the euid of the process was 0 but the interface was actually working.
Signed-off-by: Sven Eckelmann sven@narfation.org --- functions.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/functions.c b/functions.c index c1aaf12..2239440 100644 --- a/functions.c +++ b/functions.c @@ -1141,7 +1141,7 @@ void get_random_bytes(void *buf, size_t buflen)
void check_root_or_die(const char *cmd) { - if (getuid() || getgid()) { + if (geteuid() != 0) { fprintf(stderr, "Error - you must be root to run '%s' !\n", cmd); exit(EXIT_FAILURE); }
It can happen that netlink_get_info will not return an algo string. This usually happens because BATADV_CMD_GET_MESH_INFO is not implemented in the kernel modules. But it is also possible that this happens because the user didn't have the correct rights to retrieve this type of information.
Signed-off-by: Sven Eckelmann sven@narfation.org --- netlink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/netlink.c b/netlink.c index 9580aa3..7fb1ee1 100644 --- a/netlink.c +++ b/netlink.c @@ -1177,11 +1177,12 @@ int netlink_print_originators(char *mesh_iface, char *orig_iface, }
/* only parse routing algorithm name */ + last_err = -EINVAL; info_header = netlink_get_info(ifindex, BATADV_CMD_GET_ORIGINATORS, NULL); free(info_header);
if (strlen(algo_name_buf) == 0) - return -EOPNOTSUPP; + return last_err;
if (!strcmp("BATMAN_IV", algo_name_buf)) header = " Originator last-seen (#/255) Nexthop [outgoingIF]\n"; @@ -1244,11 +1245,12 @@ int netlink_print_gateways(char *mesh_iface, char *orig_iface, int read_opts, }
/* only parse routing algorithm name */ + last_err = -EINVAL; info_header = netlink_get_info(ifindex, BATADV_CMD_GET_ORIGINATORS, NULL); free(info_header);
if (strlen(algo_name_buf) == 0) - return -EOPNOTSUPP; + return last_err;
if (!strcmp("BATMAN_IV", algo_name_buf)) header = " Router ( TQ) Next Hop [outgoingIf] Bandwidth\n";
It is a long standing TODO to move the root check to each batctl sub-application. This will allow later to make the checks specific to the requirements for each function instead of disallowing the use of batctl for non-root users completely.
Signed-off-by: Sven Eckelmann sven@narfation.org --- debug.c | 6 ++++++ interface.c | 2 ++ ioctl.c | 3 +++ main.c | 5 ----- ping.c | 2 ++ sys.c | 8 ++++++++ tcpdump.c | 2 ++ tp_meter.c | 2 ++ traceroute.c | 2 ++ translate.c | 2 ++ 10 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/debug.c b/debug.c index ee04928..07a91c4 100644 --- a/debug.c +++ b/debug.c @@ -219,6 +219,8 @@ int handle_debug_table(char *mesh_iface, int debug_table, int argc, char **argv) } }
+ check_root_or_die("batctl"); + if (read_opt & UNICAST_ONLY && read_opt & MULTICAST_ONLY) { fprintf(stderr, "Error - '-u' and '-m' are exclusive options\n"); debug_table_usage(debug_table); @@ -270,6 +272,8 @@ int print_vis_info(char *mesh_iface) char *debugfs_mnt; FILE *fp;
+ check_root_or_die("batctl vis_data"); + debugfs_mnt = debugfs_mount(NULL); if (!debugfs_mnt) { fprintf(stderr, "Error - can't mount or find debugfs\n"); @@ -318,6 +322,8 @@ int log_print(char *mesh_iface, int argc, char **argv) } }
+ check_root_or_die("batctl log"); + debugfs_mnt = debugfs_mount(NULL); if (!debugfs_mnt) { fprintf(stderr, "Error - can't mount or find debugfs\n"); diff --git a/interface.c b/interface.c index 8cc4f92..01ee6fc 100644 --- a/interface.c +++ b/interface.c @@ -324,6 +324,8 @@ int interface(char *mesh_iface, int argc, char **argv) } }
+ check_root_or_die("batctl interface"); + rest_argc = argc - optind; rest_argv = &argv[optind];
diff --git a/ioctl.c b/ioctl.c index b1db5e4..2ef4f8b 100644 --- a/ioctl.c +++ b/ioctl.c @@ -33,6 +33,7 @@ #include <linux/ethtool.h> #include <stdint.h>
+#include "functions.h" #include "ioctl.h"
/* code borrowed from ethtool */ @@ -104,6 +105,8 @@ int ioctl_statistics_get(char *mesh_iface) struct ifreq ifr; int fd = -1, ret = EXIT_FAILURE;
+ check_root_or_die("batctl statistics"); + memset(&ifr, 0, sizeof(ifr)); strncpy(ifr.ifr_name, mesh_iface, sizeof(ifr.ifr_name)); ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0'; diff --git a/main.c b/main.c index 2fc9b75..02d89c4 100644 --- a/main.c +++ b/main.c @@ -134,11 +134,6 @@ int main(int argc, char **argv) exit(EXIT_SUCCESS); }
- /* TODO: remove this generic check here and move it into the individual functions */ - /* check if user is root */ - if (strncmp(argv[1], "bisect", strlen("bisect")) != 0) - check_root_or_die(argv[0]); - if ((strcmp(argv[1], "interface") == 0) || (strcmp(argv[1], "if") == 0)) {
ret = interface(mesh_iface, argc - 1, argv + 1); diff --git a/ping.c b/ping.c index 4fef663..4f83afe 100644 --- a/ping.c +++ b/ping.c @@ -133,6 +133,8 @@ int ping(char *mesh_iface, int argc, char **argv) return EXIT_FAILURE; }
+ check_root_or_die("batctl ping"); + dst_string = argv[found_args]; bat_hosts_init(0); bat_host = bat_hosts_find_by_name(dst_string); diff --git a/sys.c b/sys.c index 7817234..9dcb4f2 100644 --- a/sys.c +++ b/sys.c @@ -152,6 +152,8 @@ int handle_loglevel(char *mesh_iface, int argc, char **argv) } }
+ check_root_or_die("batctl loglevel"); + path_buff = malloc(PATH_BUFF_LEN); snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
@@ -251,6 +253,8 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv) } }
+ check_root_or_die("batctl"); + /* prepare the classic path */ path_buff = malloc(PATH_BUFF_LEN); snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface); @@ -324,6 +328,8 @@ int handle_gw_setting(char *mesh_iface, int argc, char **argv) } }
+ check_root_or_die("batctl gw_mode"); + path_buff = malloc(PATH_BUFF_LEN); snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
@@ -449,6 +455,8 @@ int handle_ra_setting(int argc, char **argv) } }
+ check_root_or_die("batctl routing_algo"); + if (argc == 2) { res = write_file(SYS_SELECTED_RA_PATH, "", argv[1], NULL); goto out; diff --git a/tcpdump.c b/tcpdump.c index c7e0cbc..d52a451 100644 --- a/tcpdump.c +++ b/tcpdump.c @@ -1165,6 +1165,8 @@ int tcpdump(int argc, char **argv) return EXIT_FAILURE; }
+ check_root_or_die("batctl tcpdump"); + bat_hosts_init(read_opt);
/* init interfaces list */ diff --git a/tp_meter.c b/tp_meter.c index 4f4122c..918fb79 100644 --- a/tp_meter.c +++ b/tp_meter.c @@ -432,6 +432,8 @@ int tp_meter(char *mesh_iface, int argc, char **argv) return EXIT_FAILURE; }
+ check_root_or_die("batctl throughputmeter"); + dst_string = argv[found_args]; bat_hosts_init(read_opt); bat_host = bat_hosts_find_by_name(dst_string); diff --git a/traceroute.c b/traceroute.c index e7c55ef..124ce7c 100644 --- a/traceroute.c +++ b/traceroute.c @@ -94,6 +94,8 @@ int traceroute(char *mesh_iface, int argc, char **argv) return EXIT_FAILURE; }
+ check_root_or_die("batctl traceroute"); + dst_string = argv[found_args]; bat_hosts_init(read_opt); bat_host = bat_hosts_find_by_name(dst_string); diff --git a/translate.c b/translate.c index 18bde4d..31da3a3 100644 --- a/translate.c +++ b/translate.c @@ -46,6 +46,8 @@ int translate(char *mesh_iface, int argc, char **argv) return EXIT_FAILURE; }
+ check_root_or_die("batctl translate"); + dst_string = argv[1]; bat_hosts_init(0); bat_host = bat_hosts_find_by_name(dst_string);
Standard users are usually able to retrieve interface statistics via the SIOCETHTOOL ioctl. Don't artificially prevent users to retrieve the statistics via batctl.
Signed-off-by: Sven Eckelmann sven@narfation.org --- ioctl.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/ioctl.c b/ioctl.c index 2ef4f8b..2f25dc0 100644 --- a/ioctl.c +++ b/ioctl.c @@ -105,8 +105,6 @@ int ioctl_statistics_get(char *mesh_iface) struct ifreq ifr; int fd = -1, ret = EXIT_FAILURE;
- check_root_or_die("batctl statistics"); - memset(&ifr, 0, sizeof(ifr)); strncpy(ifr.ifr_name, mesh_iface, sizeof(ifr.ifr_name)); ifr.ifr_name[sizeof(ifr.ifr_name) - 1] = '\0';
The loglevel sysfs file can be read by normal users. Only writing to this file is restricted. Don't artificially restrict access to this file by the batctl subcommand.
Signed-off-by: Sven Eckelmann sven@narfation.org --- sys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sys.c b/sys.c index 9dcb4f2..ac978bf 100644 --- a/sys.c +++ b/sys.c @@ -152,12 +152,12 @@ int handle_loglevel(char *mesh_iface, int argc, char **argv) } }
- check_root_or_die("batctl loglevel"); - path_buff = malloc(PATH_BUFF_LEN); snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
if (argc != 1) { + check_root_or_die("batctl loglevel"); + for (i = 1; i < argc; i++) { if (strcmp(argv[i], "none") == 0) { log_level = 0;
The gw_mode sysfs file can be read by normal users. Only writing to this file is restricted. Don't artificially restrict access to this file by the batctl subcommand.
Signed-off-by: Sven Eckelmann sven@narfation.org --- sys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sys.c b/sys.c index ac978bf..a695c18 100644 --- a/sys.c +++ b/sys.c @@ -328,8 +328,6 @@ int handle_gw_setting(char *mesh_iface, int argc, char **argv) } }
- check_root_or_die("batctl gw_mode"); - path_buff = malloc(PATH_BUFF_LEN); snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
@@ -386,6 +384,8 @@ int handle_gw_setting(char *mesh_iface, int argc, char **argv) goto out; }
+ check_root_or_die("batctl gw_mode"); + if (strcmp(argv[1], "client") == 0) gw_mode = GW_MODE_CLIENT; else if (strcmp(argv[1], "server") == 0)
The sysfs files can be read by normal users. Only writing to these files is restricted. Don't artificially restrict access to these files by the batctl subcommands.
Signed-off-by: Sven Eckelmann sven@narfation.org --- sys.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sys.c b/sys.c index a695c18..65b438c 100644 --- a/sys.c +++ b/sys.c @@ -253,8 +253,6 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv) } }
- check_root_or_die("batctl"); - /* prepare the classic path */ path_buff = malloc(PATH_BUFF_LEN); snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface); @@ -272,6 +270,8 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv) goto out; }
+ check_root_or_die("batctl"); + if (!batctl_settings[setting].params) goto write_file;
The list of interfaces attached to a batman-adv interface can be read by a normal user via rtnetlink or sysfs. Only modifying this list requires more acesss. Don't artificially restrict access to this information by the batctl sucommand.
Signed-off-by: Sven Eckelmann sven@narfation.org --- interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/interface.c b/interface.c index 01ee6fc..5a57be6 100644 --- a/interface.c +++ b/interface.c @@ -324,14 +324,14 @@ int interface(char *mesh_iface, int argc, char **argv) } }
- check_root_or_die("batctl interface"); - rest_argc = argc - optind; rest_argv = &argv[optind];
if (rest_argc == 0) return print_interfaces(mesh_iface);
+ check_root_or_die("batctl interface"); + if ((strcmp(rest_argv[0], "add") != 0) && (strcmp(rest_argv[0], "a") != 0) && (strcmp(rest_argv[0], "del") != 0) && (strcmp(rest_argv[0], "d") != 0) && (strcmp(rest_argv[0], "create") != 0) && (strcmp(rest_argv[0], "c") != 0) &&
On Sunday, January 22, 2017 1:20:42 PM CET Sven Eckelmann wrote:
Hi,
batctl has a "root" user check since the beginning of this tool. It even got a TODO comment in 58525b8330e9 ("batctl: adjust vis.c coding style"):
/* TODO: remove this generic check here and move it into the individual
functions */
This patchset is now trying to resolve this TODO.
- batctl: Move root privileges check in separate function
- batctl: Use geteuid for checks of root privileges
- batctl: Return type of error on netlink_get_info error
- batctl: Make root privileges check function specific
- batctl: Allow to retrieve interface stats as non-root
- batctl: Allow to read loglevel as normal user
- batctl: Allow to read gw_mode as normal user
- batctl: Allow to read sysfs settings as normal user
- batctl: Allow to read list of interfaces as normal user
"Return type of error on netlink_get_info error" may look a little bit odd in this list. But it was required in my initial test to avoid that "originators" and "gwl" try to start a debugfs access when the netlink function should have returned a "-EPERM" (but did return a -EOPNOTSUPP).
Kind regards, Sven
I've adopted this series into 91b23ca..515cf78.
Thanks!! Simon
b.a.t.m.a.n@lists.open-mesh.org