scopes runner should set various confinement variables

Bug #1327426 reported by Jamie Strandboge on 2014-06-06
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
unity-scopes-api
Fix Released
High
Michi Henning
unity-scopes-api (Ubuntu)
Undecided
Unassigned

Bug Description

Click apps started with ubuntu-app-launch (or via aa-exec-click via legacy desktop files) have a number of environment variables set. Eg, for a test app of mine with APP_ID of 'com.ubuntu.developer.jdstrand.click-env_click-env_0.1', the following are set by ubuntu-app-launch:
APP_ID=com.ubuntu.developer.jdstrand.click-env_click-env_0.1
__GL_SHADER_DISK_CACHE_PATH=/home/phablet/.cache/com.ubuntu.developer.jdstrand.click-env
LD_LIBRARY_PATH=/opt/click.ubuntu.com/.click/users/phablet/com.ubuntu.developer.jdstrand.click-env/lib/arm-linux-gnueabihf:/opt/click.ubuntu.com/.click/users/phablet/com.ubuntu.developer.jdstrand.click-env/lib
PATH=/opt/click.ubuntu.com/.click/users/phablet/com.ubuntu.developer.jdstrand.click-env/lib/arm-linux-gnueabihf/bin:/opt/click.ubuntu.com/.click/users/phablet/com.ubuntu.developer.jdstrand.click-env:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
QML2_IMPORT_PATH=/opt/click.ubuntu.com/.click/users/phablet/com.ubuntu.developer.jdstrand.click-env/lib/arm-linux-gnueabihf
TMPDIR=/run/user/32011/confined/com.ubuntu.developer.jdstrand.click-env
UBUNTU_APPLICATION_ISOLATION=1
XDG_CACHE_HOME=/home/phablet/.cache
XDG_CONFIG_HOME=/home/phablet/.config
XDG_DATA_DIRS=/opt/click.ubuntu.com/.click/users/phablet/com.ubuntu.developer.jdstrand.click-env:/usr/share
XDG_DATA_HOME=/home/phablet/.local/share
XDG_RUNTIME_DIR=/run/user/32011

For scopes, we should probably do the same thing. I suggest the following:
  APP_ID=<apparmor profile name>
  LD_LIBRARY_PATH=<click install directory>/lib/$multiarch:<click install directory>/lib
  PATH=<click install directory>/lib/$multiarch/bin:<click install directory>:$PATH
  UBUNTU_APPLICATION_ISOLATION=1
  XDG_DATA_DIRS=<click install directory>:/usr/share
  XDG_DATA_HOME=$HOME/.local/share
  XDG_RUNTIME_DIR=/run/user/$UID

APP_ID may not strictly be needed since these are scopes and not subject to application lifecycle, etc, but if it is provided developers will have a consistent environment when developing apps and scopes and it might make it easier for them to programmatically determine the location of certain directories, specifically their scope-specific writable area. Eg:

  my_pkgname = $APP_ID.split('_')[0]
  my_writable_dir = $XDG_DATA_HOME/unity-scopes/leaf-net/$my_pkgname

You could also just provide an API to determine these directories (as was discussed at the sprint).

TMPDIR is not listed above, but probably should be since apparmor policy does not allow access to /tmp. If you want to provide that, I suggest:
  TMPDIR=/run/user/$UID/confined-scopes/leaf-net/<'name' from click manifest>

Note, 'leaf-net' is used for the same reason here as it is in {HOME}/.local/share/unity-scopes/leaf-net/@{APP_PKGNAME}/ in the policy: to prevent other scope types from having overlapped writable directories which could lead to information disclosure (see bug 1326105 for a description of the attack). 'leaf-net' should be used with scopes that use the ubuntu-scope-network apparmor template and 'leafs-fs' for scopes that use the (currently unavailable) ubuntu-scope-local-content apparmor template.

Related branches

description: updated
description: updated
tags: added: application-confinement
Michal Hruby (mhr3) wrote :

As scope-registry is the one exec-ing the scopes, it should set the envvars. Plus the API should provide a method to get the writable path.

Michal Hruby (mhr3) on 2014-07-02
Changed in unity-scopes-api:
status: New → Triaged
importance: Undecided → High
assignee: nobody → Pete Woods (pete-woods)
Changed in unity-scopes-api:
assignee: Pete Woods (pete-woods) → Michi Henning (michihenning)
Michi Henning (michihenning) wrote :

Because we have aggregating scopes too, it seems that we really need three (not two) locations for the cache directory, so aggregating scopes have a place to drop their data files too.

How about using $XDG_DATA_HOME/unity-scopes/aggregator/<scope_id>? Would that be OK?

There is no way for us in the run time to find out whether a scope is a filesystem scope, network scope, or aggregator scope.
This raises the question of how to set the cache directory correctly for the scope to read.

Suggestion: the scope installation is responsible for creating the scope's cache directory. In the run time, I go and try and find the following:

$XDG_DATA_HOME/unity-scopes/aggregator/<scope_id>
$XDG_DATA_HOME/unity-scopes/leaf-net/<scope_id>
$XDG_DATA_HOME/unity-scopes/leaf-fs/<scope_id>

If I find that exactly one of these three is present AND has permissions 700, I conclude that I have found the correct directory and set the cache directory in the run time accordingly. Otherwise, the cache directory is considered invalid and, when the scope tries to retrieve it from the run time, I throw an exception.

Please let me know whether that's an acceptable approach.

Michi Henning (michihenning) wrote :

I have a branch that sets the following as shown. <scope_type> is one of "aggregator", "leaf-net", or "leaf-fs".

XDG_RUNTIME_DIR=/run/user/$UID
XDG_DATA_HOME=$HOME/.local/share/unity-scopes/<scope_type>/<scope_id>
TMPDIR=$HOME/.local/share/unity-scopes/<scope_type>/<scope_id>
XDG_CONFIG_HOME=<scope install directory>
LD_LIBRARY_PATH=<scope install directory>
UBUNTU_APPLICATION_ISOLATION=1

I don't think there is any point in adding <click-install-dir>/lib to LD_LIBRARY_PATH because a scope can't install anything there.

Similarly, I don't think there is any point in setting PATH because a scope isn't allowed to exec anything anyway?

I didn't set APP_ID because we now have a cache_directory() method available to the scope that returns the root dir of the scope's scratch area in the filesystem. There is also a scope_directory() method that returns the install directory of the scope.

Will this be OK?

Jamie Strandboge (jdstrand) wrote :

APP_ID is up to you. If it doesn't make sense, that's fine. The nice thing about the APP_ID is that it is used with apps and its value also corresponds to what is used in application lifecycle and application isolation (it is the profile name). It might be nice to continue this line of thinking into scopes, but like I said, if it isn't useful, I don't have a problem with not including it.

I'm not sure I understand this statement: "I don't think there is any point in adding <click-install-dir>/lib to LD_LIBRARY_PATH because a scope can't install anything there." Click packaging is quite simple and is simply an archive file to unpack and some metadata (the click manifest and any corresponding files to support its declared click hooks). As such, a click can contain executables, libraries, data files, etc and a click install will simply unpack whatever is in the archive file into the <click install directory>.

As such, I do think PATH and LD_LIBRARY_PATH should be set since scopes do have execute and mmap permissions on things in their install install directory per this rule:
  @{CLICK_DIR}/@{APP_PKGNAME}/@{APP_VERSION}/** mrklix,

Like scopes, apps may not be expected to execute (or even mmap) things from their install directory, but blocking that seems artificially limiting for developers, so we allow it. Adjusting the PATH and LD_LIBRARY_PATH in the way I described makes it so that application developers don't have to jump through hoops or hardcode click install paths in their code.

Jamie Strandboge (jdstrand) wrote :

"How about using $XDG_DATA_HOME/unity-scopes/aggregator/<scope_id>? Would that be OK?" Seems reasonable to me.

"Please let me know whether that's an acceptable approach." Having the click scope hook create the directory at install time and then later seeing which directory exists seems reasonable if you can't know at runtime what kind of scope it is. A confined scope isn't going to be able to fake this since it won't have write access to the other directories. An unconfined scope could, but, well, it is unconfined and therefore trusted.

Michi Henning (michihenning) wrote :

> I'm not sure I understand this statement: "I don't think there is any
> point in adding <click-install-dir>/lib to LD_LIBRARY_PATH because
> a scope can't install anything there."

I think that was just a mis-understanding. I think you suggested that
*both* these should be in LD_LIBRARY_PATH:

<click install directory>/lib/$multiarch
<click install directory>/lib

I was simply saying that the second of these is unnecessary because
nothing ever gets installed there. The scopes are always installed
into the arch-specific directory, and that one is set in LD_LIBRARY_PATH.

I've added PATH to the list of env vars that are set as well. Setting
is the same as LD_LIBRARY_PATH, that is, the scope install dir.

Jamie Strandboge (jdstrand) wrote :

Ah, right, yes I did suggest both should be in LD_LIBRARY_PATH:
<click install directory>/lib/$multiarch
<click install directory>/lib

The reason being that like with app developers, it is conceivable someone will create a scope click for a single architecture and use only <click install directory>/lib. It is included as a convenience since some people may not be using the sdk, etc.

Michi Henning (michihenning) wrote :
Download full text (3.9 KiB)

I'm beginning to think that the entire idea of using environment variables for all this is problematic.

We had long discussions about whether or not it is necessary for the environment variables to be set before a scope's create function is called. The only way the scope could access these environment variables is either from within the implementation of the create function, or from the constructor of the ScopeBase that the create function returns. Neither of which is necessary, IMO, seeing that we have a separate start() method to initialise the scope. (I added this method specifically to provide some threading guarantees and to create a well-defined initialisation point.)

Second, it is conceivable that a scope has global object instances whose constructors might access an environment variable. Personally, I have absolutely no sympathy for someone who writes code like that. People who do this deserve what they get, IMO, and I feel no need whatsoever to provide scope-specific information at the time global constructors run.

Third, there is a more serious concern. I have been very careful to ensure that the scope run time has a well-defined life cycle. We control precisely when and how a run time instance is created and when it is destroyed. It is possible to have multiple run time instances active concurrently in the same process, and we actually do this in a number of places (and not just the tests). There are no global/static objects in the library that can break this isolation: run time instances are separate and independent, each with its own configuration, threads, etc.

The environment variables shoot a big hole through all this because environment variables are shared globally for everything in a process. In particular, running a second instance of the scopes run time causes it to smash the environment variable settings for the first instance. True, in a production environment, we run each scope in its own process, but the whole thing still feels wrong to me because it ties the run times together in unexpected ways. I had a surprising failure yesterday because of exactly this. We have at least one place in our code where we temporarily create a second run time instance. That second instance promptly overwrote the environment of the first one.

So, I want to move away from environment variables, except for LD_LIBRARY_PATH (and maybe PATH), which we can set easily enough before the scope's .so is loaded.

Instead of getting the scope to read environment variables, we can provide accessors on ScopeBase.

We already have:

scope_directory() // returns the install dir, corresponds to XDG_CONFIG_HOME
cache_directory() // returns the dir that is writable to the scope, corresponds to XDG_DATA_HOME

This leaves PATH, TMPDIR, XDG_RUNTIME_DIR, and UBUNTU_APPLICATION_ISOLATION.

XDG_RUNTIME_DIR and TMPDIR need to be the same anyway, because TMPDIR has to be a sub-directory of /run/user/<uid>, and the scope can't write to locations "above" that, so it doesn't make sense to set XDG_RUNTIME_DIR to anything but TMPDIR.

We can add

tmp_directory() // returns a writable tmp directory somewhere below /run/user/<uid>

This will take care of TMPDIR and...

Read more...

Jamie Strandboge (jdstrand) wrote :

> So, I want to move away from environment variables, except for LD_LIBRARY_PATH (and maybe PATH), which we can set easily enough before the scope's .so is loaded.

This sounds fine. The point of both is to make things convenient for the developer who may want to organize the code in a particular way or exec() something that is shipped in the click.

> scope_directory() // returns the install dir, corresponds to XDG_CONFIG_HOME
> cache_directory() // returns the dir that is writable to the scope, corresponds to XDG_DATA_HOME

Sure.

> This leaves PATH, TMPDIR, XDG_RUNTIME_DIR, and UBUNTU_APPLICATION_ISOLATION.
>
> XDG_RUNTIME_DIR and TMPDIR need to be the same anyway, because TMPDIR has to be a sub-directory of /run/user/<uid>, and the scope can't write to locations "above" that, so it doesn't make sense to set XDG_RUNTIME_DIR to anything but TMPDIR.

Actually, on Ubuntu XDG_RUNTIME_DIR is set to /run/user/`id -u` as part of the user's login session. This should not be set to something else (and only explicitly set to '/run/user/`id -u`' it it isn't already.

> We can add

> tmp_directory() // returns a writable tmp directory somewhere below /run/user/<uid>

> This will take care of TMPDIR and XDG_RUNTIIME_DIR.

I think this is troublesome to only implement this method without setting TMPDIR. We set TMPDIR because well-written (ie, our system) libraries respect this. Not setting this and only offering tmp_directory() will make it so the developer has to know to set this separately if using some library or similar that is creating a temporary file.

>For PATH, I'm not sure I see the point. The scope knows where it is installed (in scope_directory()). If the scope wants to exec something in its own bin directory, it can just exec it without having to rely on PATH. We can set a defined path, no problem. But whatever we set it to will have to be a standard path that is the same for all scopes, not containing any scope-specific directories.

Well, again, this is all about consistency for developing on Ubuntu. If when writing an app you get PATH set, but don't for a scope, that seems inconsistent. I guess I just figured that all this would happen after the fork() but before the exec() of the scope-runner or the binary. It isn't clear to me why this would affect siblings or parents....

> That leaves UBUNTU_APPLICATION_ISOLATION. I did a Google search and couldn't find any doc on that specific variable. But is it really needed? It is understood that scopes are confined. If a scope really needs to find out whether it is currently under some Apparmor profile, it can just try doing something that won't work if confined. If the attempt fails, the scope knows it's currently confined.

https://wiki.ubuntu.com/SecurityTeam/Specifications/ApplicationConfinement#Launching_applications
http://developer.ubuntu.com/publish/apps/security-policy-for-click-packages/ (see 'Runtime Environment')

Also, I just want to restate that none of this is a security concern when a scope is confined since apparmor will make sure the scope is confined regardless of environment. I filed this bug in the hopes of making things consistent for the developer.

Michi Henning (michihenning) wrote :
Download full text (5.5 KiB)

> > XDG_RUNTIME_DIR and TMPDIR need to be the same anyway, because TMPDIR has to be a sub-directory of /run/user/<uid>, and the scope can't write to locations "above" that, so it doesn't make sense to set XDG_RUNTIME_DIR to anything but TMPDIR.

> Actually, on Ubuntu XDG_RUNTIME_DIR is set to /run/user/`id -u` as part of the user's login session. This should not be set to
> something else (and only explicitly set to '/run/user/`id -u`' it it isn't already.

At the moment, I'm not setting it at all. It's set whatever we get from our calling environment. But I'm a bit puzzled here: unless I misunderstand something, a confined scope can't write to this directory, only to sub-directories of XDG_RUNTIME_DIR as set by the Apparmor policy. I'm wondering whether that might not be surprising for the scope if XDG_RUNTIME_DIR is not writable? (But see below, scope-specific env vars are not an option anyway.)

>I think this is troublesome to only implement this method without setting TMPDIR. We set TMPDIR because well-written (ie, our system) libraries respect this. Not setting this and only offering tmp_directory() will make it so the developer has to know to set this separately if using some library or similar that is creating a temporary file.

We are really running into a much deeper issue here. Environment variables are an idea that is three decades old, and they do not play at all well with threads and plug-in architectures. For one, there is no way for a thread to retrieve the value of an environment variable without risking a crash because getenv() and setenv() are not thread-safe. Second, environment variables affect the entire process, but that destroys the notion of having more than one run time in the same process because the setting would have to be scope-specific. (Note that this is entirely orthogonal to confinement.)

> Well, again, this is all about consistency for developing on Ubuntu. If when writing an app you get PATH set, but don't for a scope, that seems inconsistent. I guess I just figured that all this would happen after the fork() but before the exec() of the scope-runner or the binary. It isn't clear to me why this would affect siblings or parents....

The issue is that several scopes can share a single process. This is not so much because that would be an essential feature, but a software design decision: things in libraries must have a well-defined life cycle and must not share any hidden state. Environment variables effectively are global variables that tie otherwise separate run time instances together in an invisible way.

The problem here is not my design decision, the problem is TMPDIR. In a well-defined threaded execution environment (especially when thinking about sand-boxing) *no* library should *ever* have to read an environment variable to decide anything. (Of course, I'm aware that there are historical reasons for all this. But the assumptions that were valid when TMPDIR was invented are becoming increasingly less valid, as we are changing the security models for UNIX, and threads didn't exist back then.)

>> That leaves UBUNTU_APPLICATION_ISOLATION. I did a Google search and couldn't find any doc on that specific ...

Read more...

Changed in unity-scopes-api:
status: Triaged → In Progress
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:unity-scopes-api/devel at revision None, scheduled for release in unity-scopes-api, milestone Unknown

Changed in unity-scopes-api:
status: In Progress → Fix Committed
Changed in unity-scopes-api (Ubuntu):
status: New → Fix Released
Changed in unity-scopes-api:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers