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

Proposed by James Hunt
Status: Merged
Merged at revision: 1608
Proposed branch: lp:~jamesodhunt/ubuntu/vivid/upstart/bug-1425685
Merge into: lp:ubuntu/vivid/upstart
Diff against target: 37 lines (+18/-2)
2 files modified
debian/changelog (+8/-0)
debian/upstart-bin.upstart.cron.daily (+10/-2)
To merge this branch: bzr merge lp:~jamesodhunt/ubuntu/vivid/upstart/bug-1425685
Reviewer Review Type Date Requested Status
Colin Watson Approve
Dimitri John Ledkov Pending
Review via email: mp+251050@code.launchpad.net

Description of the change

* 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).

This branch provides a basic fix for the reported issue. However, I think we could potentially do better by:

- ensuring the user (extracted from the UPSTART_SESSION value) exists.
- ensuring the pid (extracted from the UPSTART_SESSION value) is running and is an upstart process.
- running /sbin/initctl as the user in question (assuming that user is validated as above). The risk here is that doing so may cause the cron job to block indefinately.

Input from other Upstart develops and the Security Team welcome.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) wrote :

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.

review: Approve
1608. By James Hunt

* debian/upstart-bin.upstart.cron.daily: Additional checks on the input
  file based on review feedback.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

I'm so sorry.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2015-01-20 10:55:02 +0000
3+++ debian/changelog 2015-02-26 12:23:52 +0000
4@@ -1,3 +1,11 @@
5+upstart (1.13.2-0ubuntu8) UNRELEASED; urgency=medium
6+
7+ * debian/upstart-bin.upstart.cron.daily:
8+ - [SECURITY FIX]: Only consider valid session files to avoid possible
9+ privilege escalation. Thanks to halfdog for reporting (LP: #1425685).
10+
11+ -- James Hunt <james.hunt@ubuntu.com> Thu, 26 Feb 2015 09:59:50 +0000
12+
13 upstart (1.13.2-0ubuntu7) vivid; urgency=medium
14
15 * Correct upstart-udev-bridge session job start/stop on conditions.
16
17=== modified file 'debian/upstart-bin.upstart.cron.daily'
18--- debian/upstart-bin.upstart.cron.daily 2015-01-16 23:55:17 +0000
19+++ debian/upstart-bin.upstart.cron.daily 2015-02-26 12:23:52 +0000
20@@ -11,7 +11,15 @@
21
22 [ -x /sbin/initctl ] || exit 0
23
24-for session in /run/user/*/upstart/sessions/*
25+for file in /run/user/*/upstart/sessions/*.session
26 do
27- env $(cat $session) /sbin/initctl emit rotate-logs >/dev/null 2>&1 || true
28+ [ -f "$file" ] || continue
29+ [ -h "$file" ] && continue
30+
31+ session=$(grep \
32+ "^UPSTART_SESSION=unix:abstract=/com/ubuntu/upstart-session/[0-9][0-9]*/[0-9][0-9]*$" \
33+ "$file" 2>/dev/null || true)
34+ [ -z "$session" ] && continue
35+
36+ env -i "$session" /sbin/initctl emit rotate-logs >/dev/null 2>&1 || true
37 done

Subscribers

People subscribed via source and target branches

to all changes: