[Merge] lp:~jamesodhunt/ubuntu/vivid/upstart/bug-1425685 into lp:ubuntu/upstart

[Colin Watson]

Review: Approve

This seems fine as far as it goes, although I think it's still slightly incomplete and you could make it safer with a one-liner.

Diff comments:

> === modified file 'debian/changelog'
> --- debian/changelog	2015-01-20 10:55:02 +0000
> +++ debian/changelog	2015-02-26 10:06:13 +0000
> @@ -1,3 +1,11 @@
> +upstart (1.13.2-0ubuntu8) UNRELEASED; urgency=medium
> +
> +  * debian/upstart-bin.upstart.cron.daily:
> +    - [SECURITY FIX]: Only consider valid session files to avoid possible
> +      privilege escalation. Thanks to halfdog for reporting (LP: #1425685).
> +
> + -- James Hunt <james.hunt at ubuntu.com>  Thu, 26 Feb 2015 09:59:50 +0000
> +
>  upstart (1.13.2-0ubuntu7) vivid; urgency=medium
>  
>    * Correct upstart-udev-bridge session job start/stop on conditions.
> 
> === modified file 'debian/upstart-bin.upstart.cron.daily'
> --- debian/upstart-bin.upstart.cron.daily	2015-01-16 23:55:17 +0000
> +++ debian/upstart-bin.upstart.cron.daily	2015-02-26 10:06:13 +0000
> @@ -11,7 +11,12 @@
>  
>  [ -x /sbin/initctl ] || exit 0
>  
> -for session in /run/user/*/upstart/sessions/*
> +for file in /run/user/*/upstart/sessions/*.session
>  do
> -    env $(cat $session) /sbin/initctl emit rotate-logs >/dev/null 2>&1 || true
> +    session=$(grep \
> +	    "^UPSTART_SESSION=unix:abstract=/com/ubuntu/upstart-session/[0-9][0-9]*/[0-9][0-9]*$" \
> +	    "$file" 2>/dev/null || true)

The user could equally well insert non-regular-file objects into this directory, which could at the very least cause the cron job to hang indefinitely, perhaps worse.  How about also checking [ -f "$file" ] && [ ! -h "$file" ]?

> +    [ -z "$session" ] && continue
> +
> +    env -i "$session" /sbin/initctl emit rotate-logs >/dev/null 2>&1 || true
>  done
> 


-- 
https://code.launchpad.net/~jamesodhunt/ubuntu/vivid/upstart/bug-1425685/+merge/251050
Your team Ubuntu branches is subscribed to branch lp:ubuntu/upstart.