Merge lp://qastaging/~jdstrand/snap-confine/ubuntu-core-launcher.nnp-off into lp://qastaging/~snappy-dev/snap-confine/trunk

One point I want to mention is that I could have dropped privs permanently after seccomp_load() in seccomp.c but instead dropped temporarily there and permanently after seccomp_load_filters() in main.c. I did this because I felt the code was easier to follow which I thought outweighed being permanently dropped for the file close and seccomp_release().

Another point to mention is the handling of UBUNTU_CORE_LAUNCHER_NO_ROOT in two places. The following should help with the logic there and impact on racing the env with the suid launcher:

UBUNTU_CORE_LAUNCHER_NO_ROOT unset in both (root): disable nnp then raise to load_seccomp (correct)
UBUNTU_CORE_LAUNCHER_NO_ROOT set in both (non-root): nnp stays then no raise to load_seccomp (correct)
UBUNTU_CORE_LAUNCHER_NO_ROOT unset first (root) and set second (non-root): disable nnp then load_seccomp fails closed
UBUNTU_CORE_LAUNCHER_NO_ROOT set first (non-root) and unset second (root): nnp stays then load_seccomp. This is more strict than the intended behavior and will trigger and apparmor denial

Note in both cases where nnp stays is the current behavior where we aren't disabling nnp, which again, is more strict and will trigger an apparmor denial.

Yikes, the kernel is live which means snappy dimension on classic is busted. Per Tyler, uploaded this as is but I'll respond to any comments from Seth in a subsequent upload.

Most of these changes look good in isolation but the "out:" label in seccomp_load_filters() is asked to do too many things. It is currently trying to handle:

- error returns
- normal code flow
- run as a 'real root'
- run via setuid execution

seccomp_load_filters() should be adjusted to die immediately on any error and avoid use of "out" for that case.

Furthermore the extensive efforts to allow testing in non-production environments is very complicated. The calls to getenv() for UBUNTU_CORE_LAUNCHER_DEBUG and UBUNTU_CORE_LAUNCHER_NO_ROOT should instead use secure_getenv().

The other environment variables should probably also be converted to use secure_getenv() as well but they have far less effect on the execution pathways, only on the correctness of the resulting policy. (secure_getenv() may break the current system but this may simply be a reflection that environment variables are being asked to do tasks they shouldn't.)

Here's the full notes I collected; not everything is direct result of this merge proposal, but this seems as good a place as any to put them:

- run_snappy_app_dev_add() does not check fork() error return
- write_string_to_file() doesn't check fclose() error return
  - close(2) manpage describes why this is important
- snappy_udev_setup_required() seems very brittle
  - why don't all snaps require this anyway?
- main() argv[NR_ARGS] = (char*)binary -- very odd, argv[NR_ARGS]
  should not have been modified at any point during the execution, and the
  statement uses a comma rather than ; to separate it from the next
  statement, exec().
- main() extra arguments passed through argv[] are passed to the executed
  program; is this intentional?

- seccomp_load_filters() "snadbox"
- seccomp_load_filters() snprintf() does not check for >= 128 error return
- seccomp_load_filters() line-too-long error handling is awkward; rather
  than jumping to out: it should probably just die immediately.
- seccomp_load_filters() line-too-long test could be written simpler
  without line length checks and use strchr('\n') instead. (I think both
  will fail if the file doesn't end with a new line character.)
- seccomp_load_filters() out: error-handling is far too complicated.
  Errors should probably just die() and allow out: for the "normal" exit
  path.
- seccomp_load_filters() "@unrestricted" check is awkward -- strncmp()
  isn't needed and only confuses the matter -- and hiding "@unrestricted"
  several hundred lines into a file means the rules that were processed to
  that point are just dropped entirely. IF we're going to allow an
  "@unrestricted" escape hatch we should enforce that it is the entire
  contents of the file, not just one line in a file.

Thanks

I've addressed all of the coding issues in lp:~jdstrand/ubuntu-core-launcher/security-review-fixes/. Specifically I:
- cleaned up 'out' handling and die instead
- fixed the typo
- verify snprint() >= 512 (I increased this length) and added tests
- add test for filter missing trailing newline (I didn't change the line-too-long handling, but did add tests and verify we must end with trailing newline
- use strcmp() with "@unrestricted". add @unrestricted near miss tests
- use getresuid() instead of UBUNTU_CORE_LAUNCHER_NO_ROOT
- replace getenv() with secure_getenv() everywhere we can
- check return code of fork()
- verify return code of fclose()
- simplified final execv()

In addition:
- don't support obsoleted SNAP_APP_TMPDIR and SNAP_APP_USER_DATA_PATH
- use uid_t and gid_t instead of unsigned
- check return codes of other (f)close()s to help futureproof

And for these two questions:

"- snappy_udev_setup_required() seems very brittle
  - why don't all snaps require this anyway?"

this is about to go away.

"- main() extra arguments passed through argv[] are passed to the executed
  program; is this intentional?"

yes

The seccomp_load_filters() error handling is still very confusing since there are a mix of die()'s and non-zero return values which ultimately end in a die(). If seccomp_load_filters() is going to internally call die() upon error in some conditions, it should be changed to have a void return and simply call die() in all error conditions. I believe that was what Seth was suggesting, as well.

Sorry, I entered the last comment in the wrong MP. Please ignore it.