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)