[B.A.T.M.A.N.] [PATCH 0/9] batctl: relax root privileges check

[B.A.T.M.A.N.] [PATCH 0/9] batctl: relax root privileges check

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[B.A.T.M.A.N.] [PATCH 1/9] batctl: Move root privileges check in separate function

[Sven Eckelmann]

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)) {
 
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 2/9] batctl: Use geteuid for checks of root privileges

[Sven Eckelmann]

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);
 	}
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 3/9] batctl: Return type of error on netlink_get_info error

[Sven Eckelmann]

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";
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 4/9] batctl: Make root privileges check function specific

[Sven Eckelmann]

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);
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 5/9] batctl: Allow to retrieve interface stats as non-root

[Sven Eckelmann]

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';
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 6/9] batctl: Allow to read loglevel as normal user

[Sven Eckelmann]

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;
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 7/9] batctl: Allow to read gw_mode as normal user

[Sven Eckelmann]

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)
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 8/9] batctl: Allow to read sysfs settings as normal user

[Sven Eckelmann]

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;
 
-- 
2.11.0

[B.A.T.M.A.N.] [PATCH 9/9] batctl: Allow to read list of interfaces as normal user

[Sven Eckelmann]

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) &&
-- 
2.11.0

Re: [B.A.T.M.A.N.] [PATCH 0/9] batctl: relax root privileges check

[Simon Wunderlich]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]