oxide should use an app-specific path for shared memory files

[Jamie Strandboge]

Oxide creates shared memory files as /run/shm/.org.chromium.Chromium.*. This results in an AppArmor rule like the following:
  owner /run/shm/.org.chromium.Chromium.* rwk,

But this rule is too lenient because a malicious app could enumerate these files and attack shared memory of other applications. Therefore, these paths need to be made application specific.

[Chris Coulson (chrisccoulson)]

I would modify base::GetShmemTempDir() to lookup a path via base::PathService and then have Oxide override this. You'll need to add a new key in base/base_paths_posix.h as well.

[Olivier Tilloy (osomon)]

For applications packaged as snaps, snappy-debug suggests writing to "/dev/shm/snap.$SNAP_NAME.*".

[Olivier Tilloy (osomon)]

> I would modify base::GetShmemTempDir() to lookup a path via
> base::PathService and then have Oxide override this.

GetShmemTempDir() tries to create a temp file under /dev/shm/, so the problem is not the path itself, it’s the name of the temp file under that path.
IIUC what needs to be modified is the TempFileName() function, to return something like "snap.${SNAP_NAME}.XXXXXX", if SNAP_NAME is defined, and fall back to the default value, ".org.chromium.Chromium.XXXXXX".

[Olivier Tilloy (osomon)]

And here is what such a patch would look like:

diff --git a/base/files/file_util_posix.cc b/base/files/file_util_posix.cc
index 42de931..f9dec07 100644
--- a/base/files/file_util_posix.cc
+++ b/base/files/file_util_posix.cc
@@ -139,6 +139,10 @@ std::string TempFileName() {
 #if defined(GOOGLE_CHROME_BUILD)
   return std::string(".com.google.Chrome.XXXXXX");
 #else
+ const char* tmp = getenv("SNAP_NAME");
+ if (tmp) {
+ return std::string("snap.").append(tmp).append(".XXXXXX");
+ }
   return std::string(".org.chromium.Chromium.XXXXXX");
 #endif
 }

[Chris Coulson (chrisccoulson)]

That won't work for child processes, where the environment variable won't exist. Also, we should have something that works for current apps on the phone (I assume there's a different environment variable for that). If you're not using PathService, there would at least need to be a command line option to make this work.

[Olivier Tilloy (osomon)]

For current (click) apps on the phone, the APP_PKGNAME variable is used to parametrize apparmor policies. According to https://wiki.ubuntu.com/AppStore/Interfaces/ApplicationId, this is the name of the click package (e.g. "com.ubuntu.foo"). However there are currently no existing rules to allow /{dev,run}/shm/*${APP_PKGNAME}*, so apparmor-easyprof-ubuntu would need to be updated to add such a rule.

I’m fine with using PathService, but that will result in a slightly more intrusive patch to chromium. I think I’d rather rely entirely on environment variable, rather than adding a command-line option.

[Tyler Hicks (tyhicks)]

I think using APP_PKGNAME on the phone makes sense. However, I think we'd want the the APP_PKGNAME to be the leading string in the filename so that we can use "/{dev,run}/shm/${APP_PKGNAME}*" instead of "/{dev,run}/shm/*${APP_PKGNAME}*".

[Olivier Tilloy (osomon)]

Yes, that’s fine by me. I just wanted to have some sort of agreement before writing any code. Who can/will do the change to apparmor-easyprof-ubuntu?

[Tyler Hicks (tyhicks)]

Regarding the apparmor-easyprof-ubuntu changes and landing, it'll likely be Jamie next week or either of us the week after.

[Olivier Tilloy (osomon)]

Thanks Tyler, that sounds good. I’ll start the work on oxide and we can synchronize next week to adjust the paths.

[Olivier Tilloy (osomon)]

This is how the code that overrides the path for shared memory in oxide would look like. How does that look?

base::FilePath GetSharedMemoryPath() {
  // snap packages
  const char* tmp = getenv("SNAP_NAME");
  if (tmp) {
    return base::FilePath(std::string("/dev/shm/snap.") + tmp + ".oxide");
  }

  // click packages
  tmp = getenv("APP_PKGNAME");
  if (tmp) {
    return base::FilePath(std::string("/dev/shm/") + tmp + ".oxide");
  }

  // default
  return base::FilePath("/dev/shm");
}

[Olivier Tilloy (osomon)]

Oxide merge request: https://code.launchpad.net/~osomon/oxide/+git/oxide/+merge/300863

[Olivier Tilloy (osomon)]

Against oxide master: https://code.launchpad.net/~osomon/oxide/+git/oxide/+merge/300869

[Olivier Tilloy (osomon)]

This is now fixed in oxide, the apparmor-easyprof-ubuntu counterpart needs to be implemented.

[Olivier Tilloy (osomon)]

Re-opening for oxide as it turns out APP_PKGNAME is not an environment variable that is being set anywhere for click apps. According to https://developer.ubuntu.com/en/phone/platform/guides/app-confinement/, its value can be inferred like this:

  APP_PKGNAME = APP_ID.split('_')[0]

[Olivier Tilloy (osomon)]

Proposed fix for oxide: https://code.launchpad.net/~osomon/oxide/+git/oxide/+merge/303821.

[Launchpad Janitor (janitor)]

This bug was fixed in the package apparmor-easyprof-ubuntu - 16.10.3

---------------
apparmor-easyprof-ubuntu (16.10.3) yakkety; urgency=medium

  [ Michi Henning ]
  * add ClientConfig to list of allowed methods for applications using the
    thumbnailer (LP: #1528058)

 -- Jamie Strandboge <email address hidden> Fri, 26 Aug 2016 10:01:48 -0500