From 7ede89aeab04fe550557fda75f025df179edbbef Mon Sep 17 00:00:00 2001 From: paul Date: Tue, 3 Aug 1999 14:00:18 +0000 Subject: [PATCH] xmonisdn was meant to be installed setuid root. However, this was not secure (scripts were executed with system() so e.g. IFS and PATH could be used to work around any attempt of security...). Now check that it is not installed setuid root (unless the user insists, details in manpage), and recommend to put user in group `dialout' or similar and also make the isdn devices group `dialout'; the scripts can then use isdnctrl to stop/start stuff. --- xmonisdn/Imakefile | 2 +- xmonisdn/Makefile.in | 17 ++++++++++++++--- xmonisdn/Net.c | 39 +++++++++++++++++++++++++++----------- xmonisdn/README | 19 +++++++++++-------- xmonisdn/xmonisdn.c | 22 +++++++++++++++++++++ xmonisdn/xmonisdn.man.in | 41 +++++++++++++++++++++++++++++++++++++--- 6 files changed, 114 insertions(+), 26 deletions(-) diff --git a/xmonisdn/Imakefile b/xmonisdn/Imakefile index 3a84f9ee..227916da 100644 --- a/xmonisdn/Imakefile +++ b/xmonisdn/Imakefile @@ -7,7 +7,7 @@ LOCAL_LIBRARIES = XawClientLibs OBJS = xmonisdn.o Net.o HEADERS = NetP.h Net.h - INSTPGMFLAGS = -m 4755 -s + INSTPGMFLAGS = -m 0755 -s ComplexProgramTarget(xmonisdn) diff --git a/xmonisdn/Makefile.in b/xmonisdn/Makefile.in index dff02447..1018bd44 100644 --- a/xmonisdn/Makefile.in +++ b/xmonisdn/Makefile.in @@ -1,14 +1,25 @@ # # This Makefile is used for compatibility to the -# isdn4k-utils package. In order to superseede -# the original Makefile, it is called makefile. +# isdn4k-utils package. In order to supercede +# the original Makefile, it is called GNUmakefile. # -# # Add security check: # - executed commands must be owned by root # and only writable by owner. + +# Additional security stuff: # +# - don't be setuid root, and check for this; Bad Things can be done with +# setting IFS and/or PATH before invoking xmonisdn! +# +# - It's better to make the ISDN devices group `dialout' (or so), and put +# the user also into that group. Then no setuid/setgid bits are needed. +# +# - Cater for users who think they know what they are doing: refuse to run +# setuid root unless an option `-r' is given; hide this option in the +# docs where the problems with setuid programs are explained :-) + I4LU_DEFINES := $(CFLAGS) -DPARANOIA_CHECK -I. ifeq (../.config,$(wildcard ../.config)) include ../.config diff --git a/xmonisdn/Net.c b/xmonisdn/Net.c index b13ea317..b50cc737 100644 --- a/xmonisdn/Net.c +++ b/xmonisdn/Net.c @@ -4,6 +4,7 @@ #include /* for X_NOT_POSIX def */ #include /* for getting username */ #include /* for stat() ** needs types.h ***/ +#include #include /* for printing error messages */ #include #include @@ -48,6 +49,8 @@ typedef union wait waitType; #include #include +/* Only allow calling scripts when setuid root if the -r option is given */ +extern int allow_setuid; /* * The default user interface is to have the netstat turn itself off whenever * the user presses a button in it. Expert users might want to make this @@ -245,15 +248,23 @@ static int paranoia_check(cmd) #ifdef PARANOIA_CHECK struct stat stbuf; - if (stat(cmd, &stbuf)) + /* only check if setuid root */ + if (geteuid() != 0 || getuid() == 0) + return 1; + + /* don't allow any scripts if setuid root and not called with -r option */ + if (!allow_setuid) + return 0; + + if (stat(cmd, &stbuf)) /* stat failed, stay on the safe side */ - return 0; + return 0; if (stbuf.st_uid != 0) /* owner is not root */ - return 0; + return 0; if (stbuf.st_mode & (S_IWGRP | S_IWOTH)) /* writable by group or world */ - return 0; + return 0; #endif return 1; } @@ -271,10 +282,11 @@ static void NetUp (gw, event, params, nparams) if ((w->netstat.state <= 1) && (!updownwait)) { w->netstat.state = 5; updownwait = 150 / w->netstat.update; - if (updownwait < 2) updownwait = 2; + if (updownwait < 2) + updownwait = 2; redraw_netstat(w); if (paranoia_check(NETUP_COMMAND)) - system(NETUP_COMMAND); + system(NETUP_COMMAND); } return; } @@ -294,9 +306,10 @@ static void NetDown (gw, event, params, nparams) w->netstat.state = 6; redraw_netstat(w); updownwait = 150 / w->netstat.update; - if (updownwait < 2) updownwait = 2; + if (updownwait < 2) + updownwait = 2; if (paranoia_check(NETDOWN_COMMAND)) - system(NETDOWN_COMMAND); + system(NETDOWN_COMMAND); } return; } @@ -592,9 +605,13 @@ static int parse_info() infoptr = infoline + 7; for (i=0; i diff --git a/xmonisdn/xmonisdn.c b/xmonisdn/xmonisdn.c index 4e0b4509..30aa3dc1 100644 --- a/xmonisdn/xmonisdn.c +++ b/xmonisdn/xmonisdn.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -8,6 +9,9 @@ extern void exit(); static void quit(); +/* Only allow calling scripts when setuid root if the -r option is given */ +int allow_setuid = 0; + char *ProgramName; static XrmOptionDescRec options[] = { @@ -56,6 +60,24 @@ int main (argc, argv) ProgramName = argv[0]; + /* check for first arg `-r'. Must be first! */ + if (argc > 1 && !strcmp(argv[1], "-r")) { + int i; + allow_setuid = 1; + argc--; + for (i = 1; i < argc; i++) + argv[i] = argv[i+1]; + argv[i] = 0; + } + else { + if (geteuid() == 0 && getuid() != 0) { + fprintf(stderr, + "`%s' setuid root not allowed! Read the manpage to find out why.\n", + ProgramName); + exit(1); + } + } + toplevel = XtAppInitialize(&xtcontext, "xmonisdn", options, XtNumber (options), &argc, argv, NULL, NULL, 0); if (argc != 1) Usage (); diff --git a/xmonisdn/xmonisdn.man.in b/xmonisdn/xmonisdn.man.in index 8e952e0d..c4dc47e7 100644 --- a/xmonisdn/xmonisdn.man.in +++ b/xmonisdn/xmonisdn.man.in @@ -1,6 +1,6 @@ -.\" $Id: xmonisdn.man.in,v 1.2 1998/12/02 11:58:51 paul Exp $ +.\" $Id: xmonisdn.man.in,v 1.3 1999/08/03 14:00:22 paul Exp $ .\" -.\" CHECKIN $Date: 1998/12/02 11:58:51 $ +.\" CHECKIN $Date: 1999/08/03 14:00:22 $ .\" .TH XMONISDN 1 "@MANDATE@" isdn4k-utils-@I4LVERSION@ "X Tools" .SH NAME @@ -199,6 +199,41 @@ The default translation is Button3: netdown() .fi .sp + +.SH SECURITY ISSUES +There are a number of reasons why you should not run +.I xmonisdn +as a `setuid root' program. +Most importantly, any setuid root program is a potential security hole. +The scripts that are executed by +.I xmonisdn +may be compromised, or contain non-absolute paths to the commands called +(so that by changing the PATH variable the wrong binary is executed!); +or the script may in turn call other things that are not secure. +.br +If you are sure that your scripts are secure and you insist on making +.I xmonisdn +setuid root, you can give the +.B -r +option as +.I first +option. This will turn off xmonisdn's setuid detection. However, you are on +your own if you do this! +.br + +A better alternative is to put the isdn devices in group +.B dialout +(if not so already), and add yourself to the +.B dialout +group. This way you have permission to manipulate the ISDN devices, e.g. +to use +.I isdnctrl +to change the dialmode setting to off (to disable dialing) or auto (for +autodial). The whole ISDN system may also be turned on and off, see the +.I isdnctrl manpage for more info. +This is a more secure way of controlling the dialing than e.g. by +manipulating routes. + .SH ENVIRONMENT .PP .TP 8 @@ -211,4 +246,4 @@ stored in the RESOURCE_MANAGER property. .SH AUTHOR Bernhard Nebel .SH SEE ALSO -.BR xmonisdn(1x) +.BR isdnctrl(8)