On 2013/03/23 19:37, patrick keshishian wrote:
> On Sat, Mar 23, 2013 at 5:15 PM, Creamy <[email protected]> wrote:
> > From the pppd man page:
> >
> > 1163:.Sh SCRIPTS
> > 1164-.Nm
> > 1165-invokes scripts at various stages in its processing which can be
> > 1166-used to perform site-specific ancillary processing.
> > 1167-These scripts are usually shell scripts, but could be executable code
> > files
> > 1168-instead.
> > 1169-.Nm
> > 1170-does not wait for the scripts to finish.
> > 1171-The scripts are executed as root (with the real and effective user ID
> > set to 0),
> > ^^^^^^^^^^^^^^^^^^^^
> >
> > No, they are not. Which is wrong, the man page, or the behavior of pppd?
>
> some history: http://marc.info/?l=openbsd-tech&m=123494734721801
>
> ... from 2009. yikes :)
>
> --patrick
>
I think this diff is in the right direction, but that we should
check permissions on the scripts to make sure they aren't group/world
writable. Of course it doesn't stop people shooting themselves in
the foot by running a writable script from another, but stops
accidental problems.
Any OKs ?
Index: main.c
===================================================================
RCS file: /cvs/src/usr.sbin/pppd/main.c,v
retrieving revision 1.49
diff -u -p -r1.49 main.c
--- main.c 30 Apr 2011 18:49:38 -0000 1.49
+++ main.c 28 Mar 2013 15:25:56 -0000
@@ -1173,9 +1173,17 @@ run_program(prog, args, must_exist)
char **args;
int must_exist;
{
+ struct stat sbuf;
pid_t pid;
uid_t uid;
- gid_t gid;
+
+ if (stat(prog, &sbuf) < 0) {
+ syslog(LOG_ERR, "cannot stat program file %s: %m", prog);
+ return -1;
+ } else if ((sbuf.st_mode & (S_IWGRP | S_IWOTH)) != 0) {
+ syslog(LOG_ERR, "%s: group/world writable", prog);
+ return -1;
+ }
pid = fork();
if (pid == -1) {
@@ -1190,10 +1198,9 @@ run_program(prog, args, must_exist)
(void) umask (S_IRWXG|S_IRWXO);
(void) chdir ("/"); /* no current directory. */
- /* revoke privs */
+ /* Execute scripts as root, to allow routing table changes etc. */
uid = getuid();
- gid = getgid();
- if (setresgid(gid, gid, gid) == -1 || setresuid(uid, uid, uid) == -1) {
+ if (setreuid(0, 0) == -1) {
syslog(LOG_ERR, "revoke privileges: %s", strerror(errno));
_exit(1);
}
