Re: sysklogd: diff for NMU version 1.5-6.1, DELAYED/15

[Guillem Jover <guillem@debian.org>]

Hi!

On Sun, 2011-04-24 at 10:08:40 +0200, Stefano Zacchiroli wrote:
> I believe it's a valuable change with potential security implications;
> it's also a well tested one in other derivatives (Ubuntu and all its
> descendants). Considering all this, I'm hereby declaring my intention to
> sponsor the NMU prepared by Matt. For your convenience and in the hope
> to help you out, I'll upload the NMU to DELAYED/15, which won't hit the
> archive before 15 days from now. Please let me know if you are fine with
> the changes, so that I can adapt the delay.

Some nitpicking comments, mostly stuff that does not seem to match the
existing coding style, and then some implementation comments.


> diff -u sysklogd-1.5/syslogd.c sysklogd-1.5/syslogd.c
> --- sysklogd-1.5/syslogd.c
> +++ sysklogd-1.5/syslogd.c
> @@ -878,6 +885,11 @@
>  	extern char *optarg;
>  	int maxfds;
>  
> +        /* user and group id to drop to */
> +        uid_t uid = 0;
> +        gid_t gid = 0;
> +        const char* username = NULL;
> +

This is being indented with spaces, when the rest is using tabs.
The spacing around the asterisk seems inverted.

> @@ -886,7 +898,7 @@
>  		funix[i]  = -1;
>  	}
>  
> -	while ((ch = getopt(argc, argv, "a:dhf:l:m:np:rs:v")) != EOF)
> +	while ((ch = getopt(argc, argv, "a:dhf:l:m:np:rs:vu:")) != EOF)	

Added trailing tab.

>  		switch((char)ch) {
>  		case 'a':
>  			if (nfunix < MAXFUNIX)
> @@ -934,6 +946,21 @@
>  		case 'v':
>  			printf("syslogd %s.%s\n", VERSION, PATCHLEVEL);
>  			exit (0);
> +		case 'u':
> +                        if (optarg) {
> +                                username = strdup (optarg);
> +                                struct passwd *pw = getpwnam (username);

Mixed code and variable declarations is C99, I don't think the code
base has that assumption.

> +                                if (!pw) {
> +                                        fprintf (stderr, "User %s does not exist, aborting.\n", username);
> +                                        exit (1);
> +                                }
> +                                uid = pw->pw_uid;
> +                                gid = pw->pw_gid;
> +                        } else {
> +                                fputs ("Internal error: -u optarg == NULL!\n", stderr);
> +                                exit (1);

Hmm not sure this is really needed? The other cases just assume getopt
works fine.

> +                        }
> +			break;

Spaces → tabs for indentation for this whole block.

>  		case '?':
>  		default:
>  			usage();
> @@ -1087,6 +1114,19 @@
>  		kill (ppid, SIGTERM);
>  #endif
>  
> +        /*
> +         * Drop privileges if -u was specified
> +         */
> +	if (username) {
> +                if (initgroups (username, gid) || 

Trailing space.

> +                    setgid (gid) || setuid (uid)) {
> +                        perror ("Could not drop to specified user privileges");
> +                        exit (1);
> +                }
> +                free (username);
> +                username = NULL;
> +        }
> +

Spaces → tabs for indentation.

> @@ -1603,10 +1643,10 @@
>  	int msglen;
>  	char *timestamp;
>  #ifdef __gnu_linux__
> -	sigset_t mask;
> +        sigset_t mask;
>  #else
>  #ifndef SYSV
> -	sigset_t omask;
> +        sigset_t omask;
>  #endif
>  #endif
>  
> @@ -1618,9 +1658,9 @@
>  	sigaddset(&mask, SIGALRM);
>  	sigprocmask(SIG_BLOCK, &mask, NULL);
>  #else
> -#ifndef SYSV
> +#  ifndef SYSV
>  	omask = sigblock(sigmask(SIGHUP)|sigmask(SIGALRM));
> -#endif
> +#  endif
>  #endif
>  
>  	/*
> @@ -1660,11 +1700,11 @@
>  			f->f_file = -1;
>  		}
>  #ifdef __gnu_linux__
> -		sigprocmask(SIG_UNBLOCK, &mask, NULL);
> +	sigprocmask(SIG_UNBLOCK, &mask, NULL);
>  #else
> -#ifndef SYSV
> +#  ifndef SYSV
>  		(void) sigsetmask(omask);
> -#endif
> +#  endif
>  #endif
>  		return;
>  	}
> @@ -1731,9 +1771,9 @@
>  #ifdef __gnu_linux__
>  	sigprocmask(SIG_UNBLOCK, &mask, NULL);
>  #else
> -#ifndef SYSV
> +#  ifndef SYSV
>  	(void) sigsetmask(omask);
> -#endif
> +#  endif
>  #endif
>  }
>  #if FALSE

All this previous hunks seem to have suffered space damage.

> diff -u sysklogd-1.5/debian/rc.klogd sysklogd-1.5/debian/rc.klogd
> --- sysklogd-1.5/debian/rc.klogd
> +++ sysklogd-1.5/debian/rc.klogd
> @@ -12,37 +12,55 @@
>  
>  PATH=/bin:/usr/bin:/sbin:/usr/sbin
>  
> -pidfile=/var/run/klogd.pid
> +pidfile=/var/run/klogd/klogd.pid
> +kmsgpipe=/var/run/klogd/kmsg
> +kmsgpidfile=/var/run/klogd/kmsgpipe.pid
>  binpath=/sbin/klogd
>  
>  test -f $binpath || exit 0
>  
> -test ! -r /etc/default/klogd || . /etc/default/klogd
> -
>  . /lib/lsb/init-functions
>  
> +#  Use KLOGD="-k /boot/System.map-$(uname -r)" to specify System.map
> +#
> +KLOGD="-P $kmsgpipe"
> +
> +test ! -r /etc/default/klogd || . /etc/default/klogd
> +
>  case "$1" in
>    start)
>      log_begin_msg "Starting kernel log daemon..."
> -    start-stop-daemon --start --quiet --pidfile $pidfile --name klogd --startas $binpath -- $KLOGD
> +    # create klog-writeable pid and fifo directory
> +    mkdir -p /var/run/klogd
> +    chown klog:klog /var/run/klogd
> +    mkfifo -m 700 $kmsgpipe
> +    chown klog:klog $kmsgpipe
> + 
> +    # shovel /proc/kmsg to pipe readable by klogd user
> +    start-stop-daemon --start --pidfile $kmsgpidfile --exec /bin/dd -b -m -- bs=1 if=/proc/kmsg of=$kmsgpipe
> + 
> +    # start klogd as non-root with reading from kmsgpipe
> +    start-stop-daemon --start --quiet --chuid klog --exec $binpath -- $KLOGD
>      log_end_msg $?
> -    test -d /lib/init/rw/sendsigs.omit.d || mkdir -p /lib/init/rw/sendsigs.omit.d
> -    test ! -f /lib/init/rw/sendsigs.omit.d/klogd || rm -f /lib/init/rw/sendsigs.omit.d/klogd
> -    ln -s $pidfile /lib/init/rw/sendsigs.omit.d/klogd
>      ;;

Hmmm, this seems pretty ugly to me, why not just change the owner of
/proc/kmsg instead, which avoids that daemonized dd?

> diff -u sysklogd-1.5/debian/rc sysklogd-1.5/debian/rc
> --- sysklogd-1.5/debian/rc
> +++ sysklogd-1.5/debian/rc
> @@ -19,8 +19,31 @@
>  
>  test -x $binpath || exit 0
>  
> +# syslogd options should be set in /etc/default/syslogd
> +SYSLOGD=""
> +
> +# user to run syslogd as - this can overriden in /etc/default/syslogd
> +USER="syslog"
> +
>  test ! -r /etc/default/syslogd || . /etc/default/syslogd
>  
> +# Figure out under which user syslogd should be running as
> +if echo ${SYSLOGD} | grep -q '^.*-u[[:space:]]*\([[:alnum:]]*\)[[:space:]]*.*$'
> +then
> +	# A specific user has been set on the command line, try to extract it.
> +	USER=$(echo ${SYSLOGD} | sed -e 's/^.*-u[[:space:]]*\([[:alnum:]]*\)[[:space:]]*.*$/\1/')
> +else
> +	# By default, run syslogd under the syslog user
> +	SYSLOGD="${SYSLOGD} -u ${USER}"
> +fi

This seems a bit ugly, why is this needed instead of just relying on
USER being properly set? The -u option is new anyway so it's not
possible it was previously set in SYSLOGD.

> +# Unable to get the user under which syslogd should be running, stop.
> +if [ -z "${USER}" ]
> +then
> +	log_failure_msg "Unable to get syslog user"
> +	exit 1
> +fi
> +
>  . /lib/lsb/init-functions
>  
>  create_xconsole()
> @@ -36,8 +59,18 @@
[...]
> +fix_log_ownership()
> +{
> +	for l in `syslogd-listfiles -a`
> +	do
> +		chown ${USER}:adm $l
> +	done
>  }

Why is this needed?

> --- sysklogd-1.5.orig/klogd.c
> +++ sysklogd-1.5/klogd.c
> @@ -313,6 +322,8 @@
>  
>  static FILE *output_file = (FILE *) 0;
>  
> +static char     *kmsg_file = NULL; /* NULL means default /proc/kmsg */
> +

I guess either trimming spaces or using tabs?

> @@ -543,6 +554,22 @@
>  		       "console output.");
>  	}
>  
> +        /* Do we read kernel messages from a pipe? */
> +        if ( kmsg_file ) {
> +                if ( !strcmp(kmsg_file, "-") )

I always find clearer to use strcmp() == 0, instead of !strcmp().

> +                        kmsg = fileno(stdin);
> +                else {
> +                        if ( (kmsg = open(kmsg_file, O_RDONLY)) < 0 )
> +                        {
> +                                fprintf(stderr, "klogd: Cannot open kmsg file, " \
> +                                        "%d - %s.\n", errno, strerror(errno));
> +                                ksyslog(7, NULL, 0);
> +                                exit(1);
> +                        }
> +                }
> +                return proc;
> +        }
> +

Spaces → tab indentation. This does not send the klogd started
message to syslog. It might make sense to refactor the open() logic
at the end of the function, which is pretty similar.

>  	/*
>  	 * First do a stat to determine whether or not the proc based
>  	 * file system is available to get kernel messages from.
> @@ -1024,6 +1051,9 @@
>  		    case 'p':
>  			SetParanoiaLevel(1);	/* Load symbols on oops. */
>  			break;	
> +                    case 'P':           /* Alternative kmsg file path */
> +                        kmsg_file = strdup(optarg);
> +                        break;

Spaces → tab indentation.

>  		    case 's':		/* Use syscall interface. */
>  			use_syscall = 1;
>  			break;
> @@ -1035,7 +1065,6 @@
>  			break;
>  		}
>  
> -
>  	/* Set console logging level. */
>  	if ( log_level != (char *) 0 )
>  	{

Unneeded line removal.

regards,
guillem