[Iftop-users] Security Issue With iftop
Michael Shigorin
shigorin at gmail.com
Wed, 20 Jan 2010 17:21:24 +0200
--JwB53PgKC5A7+0Ej
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
PreScriptum: Paul, could you please accept changes done here?
http://git.altlinux.org/people/ldv/packages/?p=iftop.git
I'm attaching the full diff between 0.17 and ALT Linux package,
as well as individual patch (which was since updated, see git).
> 2 ldv: what would you say?
---
On Wed, Jan 20, 2010 at 04:07:23PM +0200, Michael Shigorin wrote:
> On Mon, Jan 18, 2010 at 07:35:52PM +0300, Ali Jawad wrote:
> > As you all know a non root user can not run iftop.
Unless iftop is installed suid-root.
> > So the most obvious workaround is to use sudo. Now if you
> > give a regular user sudo access he will execute.
> >
> > sudo iftop
> >
> > Once he is inside iftop. He can execute ! he will get the
> > following promtp
> >
> > command >
> >
> > At this point a user can execute su, and he will get a root
> > shell. He can also execute any command in privileged mode. The
> > idea of using sudo initially was giving the user iftop access.
> > However the user ends up with total root access.
> > Please comment.
>
> I'd make availability of "!" depend on explicit commandline
> switch -- IIRC comparing getuid()/geteuid() won't help much,
> and for a program intended to run with elevated privileges
> having means to start another program is something worth
> reconsidering.
>
> 2 ldv: what would you say?
I think that a privileged program shouldn't execute arbitrary user
specified programs unless explicitly configured to allow such insecure
behaviour.
A process can check at startup time whether it is executed:
- suid-root, by comparing results of getuid() and geteuid();
- by sudo, by checking SUDO_USER environment variable.
It is important to do the check at startup time, because process
privileges could be lowered later.
--
ldv
---
--
---- WBR, Michael Shigorin <mike@altlinux.ru>
------ Linux.Kiev http://www.linux.kiev.ua/
--JwB53PgKC5A7+0Ej
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="iftop-0.17-alt3.patch"
addrs_dlpi.c | 3 +-
addrs_ioctl.c | 3 +-
cfgfile.c | 12 ++------
cfgfile.h | 2 +
configure.in | 22 ++++++++++++++
get_addrs.h | 7 ++++
iftop.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++------
iftop.control | 17 +++++++++++
iftop.h | 3 --
options.c | 75 +-------------------------------------------------
options.h | 3 +-
ui.c | 17 +++++------
12 files changed, 143 insertions(+), 107 deletions(-)
diff --git a/addrs_dlpi.c b/addrs_dlpi.c
index 188fef8..baa8648 100644
--- a/addrs_dlpi.c
+++ b/addrs_dlpi.c
@@ -28,6 +28,7 @@
#include <net/if.h>
#include "dlcommon.h"
+#include "get_addrs.h"
extern char *split_dname(char *device, int *unitp);
extern char *strncpy2(char *dest, char *src, int n);
@@ -50,7 +51,7 @@ extern char *strncat2(char *dest, char *src, int n);
*/
int
-get_addrs_dlpi(char *interface, char if_hw_addr[], struct in_addr *if_ip_addr)
+get_addrs_dlpi(char *interface, unsigned char if_hw_addr[], struct in_addr *if_ip_addr)
{
int got_hw_addr = 0;
int got_ip_addr = 0;
diff --git a/addrs_ioctl.c b/addrs_ioctl.c
index 3d58d0d..c98ed6c 100644
--- a/addrs_ioctl.c
+++ b/addrs_ioctl.c
@@ -25,6 +25,7 @@
#endif
#include "iftop.h"
+#include "get_addrs.h"
/*
* This function identifies the IP address and ethernet address for the requested
@@ -40,7 +41,7 @@
*/
int
-get_addrs_ioctl(char *interface, char if_hw_addr[], struct in_addr *if_ip_addr)
+get_addrs_ioctl(char *interface, unsigned char if_hw_addr[], struct in_addr *if_ip_addr)
{
int s;
struct ifreq ifr = {};
diff --git a/cfgfile.c b/cfgfile.c
index 442316c..43f83c9 100644
--- a/cfgfile.c
+++ b/cfgfile.c
@@ -6,6 +6,7 @@
*/
#include <stdio.h>
+#include <stdlib.h>
#include <string.h>
#include <errno.h>
@@ -56,12 +57,12 @@ int config_init() {
return config != NULL;
}
-/* read_config_file:
+/* read_config:
* Read a configuration file consisting of key: value tuples, returning a
* stringmap of the results. Prints errors to stderr, rather than using
* syslog, since this file is called at program startup. Returns 1 on success
* or 0 on failure. */
-int read_config_file(const char *f, int whinge) {
+int read_config(const char *f, int whinge) {
int ret = 0;
FILE *fp;
char *line;
@@ -164,7 +165,6 @@ int config_get_int(const char *directive, int *value) {
* failure, or 0 if no value was found. */
int config_get_float(const char *directive, float *value) {
stringmap S;
- item *I;
char *s, *t;
if (!value) return -1;
@@ -233,9 +233,3 @@ void config_set_string(const char *directive, const char* s) {
if (S) stringmap_delete_free(S);
stringmap_insert(config, directive, item_ptr(xstrdup(s)));
}
-
-int read_config(char *file, int whinge_on_error) {
- void* o;
-
- return read_config_file(file, whinge_on_error);
-}
diff --git a/cfgfile.h b/cfgfile.h
index 38532ad..24baba0 100644
--- a/cfgfile.h
+++ b/cfgfile.h
@@ -19,6 +19,8 @@ char *config_get_string(const char *directive);
int config_get_bool(const char *directive);
int config_get_int(const char *directive, int *value);
int config_get_float(const char *directive, float *value);
+void config_set_string(const char *directive, const char* s);
+int config_get_enum(const char *directive, config_enumeration_type *enumeration, int *value);
int config_init();
diff --git a/configure.in b/configure.in
index 12055e0..12795d9 100644
--- a/configure.in
+++ b/configure.in
@@ -34,6 +34,7 @@ AM_INIT_AUTOMAKE(iftop, "0.17")
AC_DEFINE_UNQUOTED(IFTOP_VERSION, "$VERSION", [The iftop version number])
dnl Make sure we have a C compiler....
+AC_GNU_SOURCE
AC_PROG_CC
AC_HEADER_STDC
@@ -69,6 +70,26 @@ AC_ARG_WITH(libpcap,
[libpcap_prefix=$withval],
[libpcap_prefix=""])
+AC_ARG_WITH(user,
+ [ --with-user=USERNAME Drop privileges by default to USERNAME])
+AC_MSG_CHECKING([whether to drop root privileges by default])
+if test -n "$withval"; then
+ AC_DEFINE_UNQUOTED(WITH_USER, "$withval", [Drop privileges by default to this username])
+ AC_MSG_RESULT(to \"$withval\")
+else
+ AC_MSG_RESULT(no)
+fi
+
+AC_ARG_WITH(chroot,
+ [ --with-chroot=DIRECTORY When dropping privileges, chroot to DIRECTORY])
+AC_MSG_CHECKING([whether to chroot by default])
+if test -n "$withval"; then
+ AC_DEFINE_UNQUOTED(WITH_CHROOT, "$withval", [Chroot to this directory when dropping privileges])
+ AC_MSG_RESULT(to \"$withval\")
+else
+ AC_MSG_RESULT(no)
+fi
+
dnl
dnl Fairly generic checks.
dnl
@@ -92,6 +113,7 @@ AC_CHECK_FUNCS(regcomp select strdup strerror strspn)
AC_SEARCH_LIBS(socket, socket)
AC_SEARCH_LIBS(log, m)
+AC_SEARCH_LIBS(dlopen, dl)
AC_CHECK_FUNC(gethostbyname, ,
[AC_CHECK_LIB(nsl, gethostbyname)] )
diff --git a/get_addrs.h b/get_addrs.h
new file mode 100644
index 0000000..21a942c
--- /dev/null
+++ b/get_addrs.h
@@ -0,0 +1,7 @@
+#ifndef GET_ADDRS_H_ /* include guard */
+#define GET_ADDRS_H_
+
+int get_addrs_dlpi(char *interface, unsigned char if_hw_addr[], struct in_addr *if_ip_addr);
+int get_addrs_ioctl(char *interface, unsigned char if_hw_addr[], struct in_addr *if_ip_addr);
+
+#endif /* GET_ADDRS_H_ */
diff --git a/iftop.c b/iftop.c
index 5a7b41e..7e126d4 100644
--- a/iftop.c
+++ b/iftop.c
@@ -25,6 +25,10 @@
#include <signal.h>
#include <string.h>
#include <unistd.h>
+#include <errno.h>
+#include <pwd.h>
+#include <grp.h>
+#include <dlfcn.h>
#include "iftop.h"
#include "addr_hash.h"
@@ -44,6 +48,7 @@
#include "ethertype.h"
#include "cfgfile.h"
#include "ppp.h"
+#include "get_addrs.h"
/* ethernet address of interface. */
@@ -477,7 +482,6 @@ char *set_filter_code(const char *filter) {
void packet_init() {
char errbuf[PCAP_ERRBUF_SIZE];
char *m;
- int s;
int i;
int dlt;
int result;
@@ -507,7 +511,6 @@ void packet_init() {
}
// exit(0);
- resolver_initialise();
pd = pcap_open_live(options.interface, CAPTURE_LENGTH, options.promiscuous, 1000, errbuf);
// DEBUG: pd = pcap_open_offline("tcpdump.out", errbuf);
@@ -557,10 +560,69 @@ void packet_loop(void* ptr) {
}
+static int droproot (void) {
+#ifdef WITH_USER
+ uid_t uid;
+ gid_t gid;
+
+ if (geteuid())
+ return 0;
+
+ if ((uid = getuid()) == 0) {
+ const char *username = WITH_USER;
+ struct passwd *pw = getpwnam(username);
+
+ if (!pw) {
+ fprintf(stderr, "lookup of user \"%s\" failed\n", username);
+ return 1;
+ }
+
+ gid = pw->pw_gid;
+ uid = pw->pw_uid;
+
+ if (uid == 0) {
+ fprintf(stderr, "user \"%s\" should not be privileged\n",
+ username);
+ return 1;
+ }
+
+ if (initgroups(pw->pw_name, gid) != 0) {
+ fprintf(stderr,
+ "Failed to initialize supplementary group list: %m\n");
+ return 1;
+ }
+
+ endpwent();
+
+#ifdef WITH_CHROOT
+ (void) dlopen("libgcc_s.so.1", RTLD_LAZY);
+
+ const char *chroot_dir = WITH_CHROOT;
+ if (chdir(chroot_dir) < 0 || chroot(".") < 0) {
+ fprintf(stderr, "chroot/chdir to \"%s\" failed: %s\n",
+ chroot_dir, strerror(errno));
+ return 1;
+ }
+#endif /* WITH_CHROOT */
+
+ } else {
+ gid = getgid();
+ }
+
+ if (setgid(gid) < 0 || setuid(uid) < 0) {
+ fprintf(stderr, "setuid/setgid to %u:%u failed: %s\n",
+ uid, gid, strerror(errno));
+ return 1;
+ }
+
+#endif /* WITH_USER */
+ return 0;
+}
+
/* main:
* Entry point. See usage(). */
int main(int argc, char **argv) {
- pthread_t thread;
+ int rc;
struct sigaction sa = {};
/* TODO: tidy this up */
@@ -575,21 +637,27 @@ int main(int argc, char **argv) {
sa.sa_handler = finish;
sigaction(SIGINT, &sa, NULL);
- pthread_mutex_init(&tick_mutex, NULL);
-
packet_init();
init_history();
ui_init();
- pthread_create(&thread, NULL, (void*)&packet_loop, NULL);
+ if ((rc = droproot()) == 0) {
+ pthread_t thread;
+
+ pthread_mutex_init(&tick_mutex, NULL);
+
+ resolver_initialise();
- ui_loop();
+ pthread_create(&thread, NULL, (void*)&packet_loop, NULL);
- pthread_cancel(thread);
+ ui_loop();
+
+ pthread_cancel(thread);
+ }
ui_finish();
- return 0;
+ return rc;
}
diff --git a/iftop.control b/iftop.control
new file mode 100644
index 0000000..9b2e0a3
--- /dev/null
+++ b/iftop.control
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+. /etc/control.d/functions
+
+BINARY=/usr/bin/iftop
+
+new_summary 'Network bandwidth monitor'
+
+new_fmode public 4711 root root
+new_fmode netadmin 4710 root netadmin
+new_fmode restricted 700 root root
+
+new_help public "Any user can execute $BINARY"
+new_help netadmin "Only \"netadmin\" group members can execute $BINARY"
+new_help restricted "Only root can execute $BINARY"
+
+control_fmode "$BINARY" "$*" || exit 1
diff --git a/iftop.h b/iftop.h
index 42947af..b3101c2 100644
--- a/iftop.h
+++ b/iftop.h
@@ -32,8 +32,5 @@ void xfree(void *v);
void analyse_data(void);
void ui_init(void);
-/* options.c */
-void options_read(int argc, char **argv);
-
#endif /* __IFTOP_H_ */
diff --git a/options.c b/options.c
index 8f385de..32d4106 100644
--- a/options.c
+++ b/options.c
@@ -158,79 +158,6 @@ void options_set_defaults() {
}
-static void die(char *msg) {
- fprintf(stderr, msg);
- exit(1);
-}
-
-static void set_max_bandwidth(char* arg) {
- char* units;
- long long mult = 1;
- long long value;
- units = arg + strspn(arg, "0123456789");
- if(strlen(units) > 1) {
- die("Invalid units\n");
- }
- if(strlen(units) == 1) {
- if(*units == 'k' || *units == 'K') {
- mult = 1024;
- }
- else if(*units == 'm' || *units == 'M') {
- mult = 1024 * 1024;
- }
- else if(*units == 'g' || *units == 'G') {
- mult = 1024 * 1024 * 1024;
- }
- }
- *units = '\0';
- if(sscanf(arg, "%lld", &value) != 1) {
- die("Error reading max bandwidth\n");
- }
- options.max_bandwidth = value * mult;
-}
-
-static void set_net_filter(char* arg) {
- char* mask;
-
- mask = strchr(arg, '/');
- if (mask == NULL) {
- die("Could not parse net/mask\n");
- }
- *mask = '\0';
- mask++;
- if (inet_aton(arg, &options.netfilternet) == 0)
- die("Invalid network address\n");
- /* Accept a netmask like /24 or /255.255.255.0. */
- if (mask[strspn(mask, "0123456789")] == '\0') {
- /* Whole string is numeric */
- int n;
- n = atoi(mask);
- if (n > 32) {
- die("Invalid netmask");
- }
- else {
- if(n == 32) {
- /* This needs to be special cased, although I don't fully
- * understand why -pdw
- */
- options.netfiltermask.s_addr = htonl(0xffffffffl);
- }
- else {
- u_int32_t mm = 0xffffffffl;
- mm >>= n;
- options.netfiltermask.s_addr = htonl(~mm);
- }
- }
- }
- else if (inet_aton(mask, &options.netfiltermask) == 0) {
- die("Invalid netmask\n");
- }
- options.netfilternet.s_addr = options.netfilternet.s_addr & options.netfiltermask.s_addr;
-
- options.netfilter = 1;
-
-}
-
/* usage:
* Print usage information. */
static void usage(FILE *fp) {
@@ -481,7 +408,7 @@ int options_config_get_net_filter() {
}
-void options_make() {
+void options_make(void) {
options_config_get_string("interface", &options.interface);
options_config_get_bool("dns-resolution", &options.dnsresolution);
options_config_get_bool("port-resolution", &options.portresolution);
diff --git a/options.h b/options.h
index d6b64ea..317d253 100644
--- a/options.h
+++ b/options.h
@@ -85,6 +85,7 @@ typedef struct {
void options_set_defaults();
-void options_read(int argc, char **argv);
+void options_read_args(int argc, char **argv);
+void options_make(void);
#endif /* __OPTIONS_H_ */
diff --git a/ui.c b/ui.c
index 72ceeb0..c624b55 100644
--- a/ui.c
+++ b/ui.c
@@ -263,7 +263,7 @@ static void draw_bar_scale(int* y) {
char s[40], *p;
int x;
/* This 1024 vs 1000 stuff is just plain evil */
- readable_size(i, s, sizeof s, options.log_scale ? 1000 : 1024, 0);
+ readable_size(i, s, sizeof s, options.log_scale ? 1000 : 1024, options.bandwidth_in_bytes);
p = s + strspn(s, " ");
x = get_bar_length(i * 8);
mvaddch(*y + 1, x, ACS_BTEE);
@@ -298,10 +298,6 @@ void draw_line_total(float sent, float recv, int y, int x, option_linedisplay_t
char buf[10];
float n;
switch(linedisplay) {
- case OPTION_LINEDISPLAY_TWO_LINE:
- draw_line_total(sent, recv, y, x, OPTION_LINEDISPLAY_ONE_LINE_SENT, bytes);
- draw_line_total(sent, recv, y+1, x, OPTION_LINEDISPLAY_ONE_LINE_RECV, bytes);
- break;
case OPTION_LINEDISPLAY_ONE_LINE_SENT:
n = sent;
break;
@@ -311,11 +307,14 @@ void draw_line_total(float sent, float recv, int y, int x, option_linedisplay_t
case OPTION_LINEDISPLAY_ONE_LINE_BOTH:
n = recv + sent;
break;
+ case OPTION_LINEDISPLAY_TWO_LINE:
+ draw_line_total(sent, recv, y, x, OPTION_LINEDISPLAY_ONE_LINE_SENT, bytes);
+ draw_line_total(sent, recv, y+1, x, OPTION_LINEDISPLAY_ONE_LINE_RECV, bytes);
+ default:
+ return;
}
- if(linedisplay != OPTION_LINEDISPLAY_TWO_LINE) {
- readable_size(n, buf, 10, 1024, bytes);
- mvaddstr(y, x, buf);
- }
+ readable_size(n, buf, 10, 1024, bytes);
+ mvaddstr(y, x, buf);
}
void draw_bar(float n, int y) {
--JwB53PgKC5A7+0Ej
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="iftop-0.16-alt-droproot.patch"
diff -uprk.orig iftop-0.16.orig/configure.in iftop-0.16/configure.in
--- iftop-0.16.orig/configure.in 2004-02-28 21:53:37 +0300
+++ iftop-0.16/configure.in 2004-03-31 14:51:09 +0400
@@ -53,6 +53,26 @@ AC_ARG_WITH(libpcap,
[libpcap_prefix=$withval],
[libpcap_prefix=""])
+AC_ARG_WITH(user,
+ [ --with-user=USERNAME Drop privileges by default to USERNAME])
+AC_MSG_CHECKING([whether to drop root privileges by default])
+if test -n "$withval"; then
+ AC_DEFINE_UNQUOTED(WITH_USER, "$withval", [Drop privileges by default to this username])
+ AC_MSG_RESULT(to \"$withval\")
+else
+ AC_MSG_RESULT(no)
+fi
+
+AC_ARG_WITH(chroot,
+ [ --with-chroot=DIRECTORY When dropping privileges, chroot to DIRECTORY])
+AC_MSG_CHECKING([whether to chroot by default])
+if test -n "$withval"; then
+ AC_DEFINE_UNQUOTED(WITH_CHROOT, "$withval", [Chroot to this directory when dropping privileges])
+ AC_MSG_RESULT(to \"$withval\")
+else
+ AC_MSG_RESULT(no)
+fi
+
dnl
dnl Fairly generic checks.
dnl
diff -uprk.orig iftop-0.16.orig/iftop.c iftop-0.16/iftop.c
--- iftop-0.16.orig/iftop.c 2004-02-28 21:47:54 +0300
+++ iftop-0.16/iftop.c 2004-03-31 17:05:33 +0400
@@ -25,6 +25,9 @@
#include <signal.h>
#include <string.h>
#include <unistd.h>
+#include <errno.h>
+#include <pwd.h>
+#include <grp.h>
#include "iftop.h"
#include "addr_hash.h"
@@ -469,7 +472,6 @@ void packet_init() {
}
// exit(0);
- resolver_initialise();
pd = pcap_open_live(options.interface, CAPTURE_LENGTH, options.promiscuous, 1000, errbuf);
// DEBUG: pd = pcap_open_offline("tcpdump.out", errbuf);
@@ -516,10 +518,70 @@ void packet_loop(void* ptr) {
}
+static int droproot (void) {
+ uid_t uid;
+ gid_t gid;
+
+ if (geteuid())
+ return 0;
+
+ if (setgroups(0, NULL) < 0) {
+ perror ("setgroups");
+ return 1;
+ }
+
+ if ((uid = getuid()) == 0) {
+#ifndef WITH_USER
+ return 0;
+#else /* WITH_USER */
+ const char *username = WITH_USER;
+ struct passwd *pw = getpwnam(username);
+
+ if (!pw) {
+ fprintf(stderr, "lookup of user \"%s\" failed\n", username);
+ return 1;
+ }
+
+ gid = pw->pw_gid;
+ uid = pw->pw_uid;
+
+ if (uid == 0) {
+ fprintf(stderr, "user \"%s\" should not be privileged\n",
+ username);
+ return 1;
+ }
+
+ endpwent();
+
+#endif /* WITH_USER */
+ } else {
+ gid = getgid();
+ }
+
+#ifdef WITH_CHROOT
+ {
+ const char *chroot_dir = WITH_CHROOT;
+ if (chroot(chroot_dir) < 0 || chdir ("/") < 0) {
+ fprintf(stderr, "chroot/chdir to \"%s\" failed: %s\n",
+ chroot_dir, strerror(errno));
+ return 1;
+ }
+ }
+#endif /* WITH_CHROOT */
+
+ if (setgid(gid) < 0 || setuid(uid) < 0) {
+ fprintf(stderr, "setuid/setgid to %u:%u failed: %s\n",
+ uid, gid, strerror(errno));
+ return 1;
+ }
+
+ return 0;
+}
+
/* main:
* Entry point. See usage(). */
int main(int argc, char **argv) {
- pthread_t thread;
+ int rc;
struct sigaction sa = {};
/* TODO: tidy this up */
@@ -534,21 +596,27 @@ int main(int argc, char **argv) {
sa.sa_handler = finish;
sigaction(SIGINT, &sa, NULL);
- pthread_mutex_init(&tick_mutex, NULL);
-
packet_init();
init_history();
ui_init();
- pthread_create(&thread, NULL, (void*)&packet_loop, NULL);
+ if ((rc = droproot()) == 0) {
+ pthread_t thread;
+
+ pthread_mutex_init(&tick_mutex, NULL);
- ui_loop();
+ resolver_initialise();
- pthread_cancel(thread);
+ pthread_create(&thread, NULL, (void*)&packet_loop, NULL);
+
+ ui_loop();
+
+ pthread_cancel(thread);
+ }
ui_finish();
- return 0;
+ return rc;
}
--JwB53PgKC5A7+0Ej--