Unsafe file and directory permissions

Bug #1235975 reported by Jamie Strandboge
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Ubuntu system image
Fix Released
Barry Warsaw
system-image (Ubuntu)
Fix Released

Bug Description

# ls -ld /var/log/system-image/
drwxrwxrwx 2 root root 4096 Sep 24 16:02 /var/log/system-image/
# ls -l /var/log/system-image/client.log
-rw-rw-rw- 1 root root 23927 Oct 6 09:11 /var/log/system-image/client.log
# ls -ld /tmp/system-image/
drwxrwxrwx 2 root root 260 Oct 6 09:11 /tmp/system-image/

Also, predictable temporary file (/tmp/system-image). This was mentioned in bug #1233521

# system-image-cli -i
current build number: 78
device name: mako
channel: stable
last update: 2013-10-03 13:05:32
version version: 78
version ubuntu: 20131003
version device: 20131002.1

Tags: client
Changed in system-image (Ubuntu):
importance: Undecided → High
Barry Warsaw (barry)
tags: added: client
Barry Warsaw (barry)
Changed in ubuntu-system-image:
importance: Undecided → Critical
status: New → Triaged
assignee: nobody → Barry Warsaw (barry)
milestone: none → 1.9
Barry Warsaw (barry)
Changed in ubuntu-system-image:
milestone: 1.9 → none
Revision history for this message
Barry Warsaw (barry) wrote :

stgraber also suggested in IRC that /tmp may not be a good idea since that's tmpfs backed and possibly limited in size. The base directory is configurable in /etc/system-image/client.ini but maybe /var/tmp/system-image would be a better default base dir. It would have to be made writable though.

I suggest using tmpfile.mkdtemp() to provide a secure unpredictable temporary directory inside that basedir for a download session. One implication of this though is that if the s-i-dbus process exits, it really should clean up this temporary, er temporary directory. Which means that once it exits, the downloaded files will be discarded. So if, as in LP: #1236818 you start the download, but leave your phone unattended for long enough, s-i-dbus will exit and you'll have to restart the whole process again.

Or, I suppose, that temporary temporary directory could be cleaned up only prior to apply-and-reboot, and if the process exits due to timing out, we'd have to persist the fact that that tempdir was created. I'll leave that to LP: #1236818.

Changed in ubuntu-system-image:
status: Triaged → In Progress
milestone: none → 1.9
Changed in system-image (Ubuntu):
status: New → In Progress
Revision history for this message
Barry Warsaw (barry) wrote :

/var/cache/system-image it is, after stgraber pushes a new lxc-android-container to make that path writable.

For now, I'll create a safe tempdir inside there for the downloads, and leave any retry/persistence issues to LP: #1236818

Revision history for this message
Barry Warsaw (barry) wrote :

I'm starting to dislike creating a safe temporary directory inside of tempdir. Doing this causes all kinds of cascading problems with the test suite, and with atexit handling (to ensure this temporary, temporary directory is properly cleaned up for both graceful and ungraceful exits).

At this late date I think it may not be worth the extra hassle, especially in light of the comments in LP: #1233521. So I think I will *not* do this but instead ensure that [system]tempdir is root-only (specifically 02700).

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

If still using /tmp/system-image, please use O_EXCL -- we don't have link restrictions (yama) on the touch images yet.

Barry Warsaw (barry)
Changed in ubuntu-system-image:
status: In Progress → Fix Committed
Changed in system-image (Ubuntu):
status: In Progress → Fix Committed
Barry Warsaw (barry)
Changed in ubuntu-system-image:
status: Fix Committed → Fix Released
Revision history for this message
Barry Warsaw (barry) wrote :

I think I was a bit too aggressive in 1.9 in fixing this, especially in light of LP: #1233521

Here's the breakdown of directories and files, and what I think we should do for each:

[system]tempdir - by default /tmp
- This directory should already exist, and we don't own it, so do not chmod if it exists. If it doesn't exist, we'll create it with 02700
- random subdir is created using Python's tempfile.mkdtemp() method, which creates it securely <http://docs.python.org/3/library/tempfile.html#tempfile.mkdtemp>. We call this function with prefix='system-image-' and dir=<[system]tempdir> so you'll end up with directories like /tmp/system-image-0ft3jq mod 700 owned by uid:gid of the process.

Thus there should be nothing we need to do with tempdir above what Python already does, unless we have to create the directory.

[system]logfile - by default /var/log/system-image/client.log
We'll create the log file with 0600 and chmod it to that if it already exists, since we own it. If the parent directory already exists, we'll chmod it to 02700 since we should assume that we own it. If it doesn't exist, we'll create it with 02700.

Note that it's possible someone would change the client.ini file to put the log file in a location we *don't* own, e.g. /var/log/client.log. In that case /var/log could get chmod'd to an unexpected mode. It's not clear to me what we can do about that other than say "Don't Do That".

[updater]cache_partition - by default /android/cache/recovery
We don't own this so we should not chmod it. If it doesn't exist, we'll create it 02700 (but it should always exist except in the test suite).

[updater]data_partition - by default /var/lib/system-image
(This will contain subdirectories, such as `keyrings`)
Create this, and subdirs, with 02700. If this directory (and subdirs) exist, chmod them to 02700. Similar to the discussion above, if someone changes it to point to an existing directory we don't own (e.g. /var/lib itself), then we could end up chmoding it unexpectedly. "Don't Do That" also applies here.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package system-image - 1.9.1-0ubuntu1

system-image (1.9.1-0ubuntu1) saucy; urgency=low

  * New upstream release:
    - LP: #1240105 - Further refinement of permission checking/fixing.
    - LP: #1240106 - Work around some failures in DEP 8 tests.
  * d/control: Point Vcs-Bzr and Vcs-Browser to the packaging branch.
  * d/system-image-common.dirs: Add /var/log/system-image.
  * d/rules, d/tests/unittests: Set $SYSTEMIMAGE_REACTOR_TIMEOUT to 1200
    seconds to avoid random timeout errors.
  * d/system-image-common.postinst, system-image-common.postrm: debhelper
    scripts for ensuring the proper permissions and for purging directories.
 -- Barry Warsaw <email address hidden> Tue, 15 Oct 2013 11:23:54 -0400

Changed in system-image (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.