Unsafe file and directory permissions

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.

/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

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

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

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.

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