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

[Jamie Strandboge]

Don't set NO_NEW_PRIVS. This requires changing privilege dropping since CAP_SYS_ADMIN is needed with seccomp_load(). This means temporarily dropping until seccomp_load(), then raising before and permanently dropping after the filter is applied. As a result, setuid/setgid is required in all policy (but is still mediated by AppArmor).

[Jamie Strandboge (jdstrand)]

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

[Jamie Strandboge (jdstrand)]

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

[Jamie Strandboge (jdstrand)]

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.

[Jamie Strandboge (jdstrand)]

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.

review: Approve

[Seth Arnold (seth-arnold)]

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

review: Needs Fixing

[Jamie Strandboge (jdstrand)]

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

[Jamie Strandboge (jdstrand)]

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

[Tyler Hicks (tyhicks)]

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.

review: Needs Fixing

[Tyler Hicks (tyhicks)]

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

[Tyler Hicks (tyhicks)]

review: Abstain

=== modified file 'debian/changelog'
--- debian/changelog	2016-03-09 07:45:08 +0000
+++ debian/changelog	2016-03-21 17:30:32 +0000
@@ -1,3 +1,13 @@
+ubuntu-core-launcher (1.0.20) UNRELEASED; urgency=medium
+
+  * don't set NO_NEW_PRIVS. This requires changing privilege dropping since
+    CAP_SYS_ADMIN is needed with seccomp_load(). This means temporarily
+    dropping until seccomp_load(), then raising before and permanently
+    dropping after the filter is applied. As a result, setuid/setgid is
+    required in all policy (but is still mediated by AppArmor)
+
+ -- Jamie Strandboge <[e-mail]>  Mon, 21 Mar 2016 12:23:41 -0500
+
 ubuntu-core-launcher (1.0.19) xenial; urgency=medium
 
   [Michael Vogt]

=== modified file 'src/main.c'
--- src/main.c	2016-03-09 07:45:08 +0000
+++ src/main.c	2016-03-21 17:30:32 +0000
@@ -407,6 +407,8 @@
    const char *appname = argv[1];
    const char *aa_profile = argv[2];
    const char *binary = argv[3];
+   unsigned real_uid = getuid();
+   unsigned real_gid = getgid();
 
    if(!verify_appname(appname))
       die("appname %s not allowed", appname);
@@ -445,27 +447,23 @@
          setup_udev_snappy_assign(appname);
       }
 
-      // the rest does not so drop privs back to calling user
-      unsigned real_uid = getuid();
-      unsigned real_gid = getgid();
-
-      // Note that we do not call setgroups() here because its ok
-      // that the user keeps the groups he already belongs to
-      if (setgid(real_gid) != 0)
-         die("setgid failed");
-      if (setuid(real_uid) != 0)
-         die("setuid failed");
-
-      if(real_gid != 0 && (getuid() == 0 || geteuid() == 0))
+      // the rest does not so temporarily drop privs back to calling user
+      // (we'll permanently drop after loading seccomp)
+      if (setegid(real_gid) != 0)
+         die("setegid failed");
+      if (seteuid(real_uid) != 0)
+         die("seteuid failed");
+
+      if(real_gid != 0 && geteuid() == 0)
          die("dropping privs did not work");
-      if(real_uid != 0 && (getgid() == 0 || getegid() == 0))
+      if(real_uid != 0 && getegid() == 0)
          die("dropping privs did not work");
    }
 
    // Ensure that the user data path exists.
    setup_user_data();
 
-   //https://wiki.ubuntu.com/SecurityTeam/Specifications/SnappyConfinement#ubuntu-snapp-launch
+   // https://wiki.ubuntu.com/SecurityTeam/Specifications/SnappyConfinement
 
    int rc = 0;
    // set apparmor rules
@@ -480,6 +478,21 @@
    if (rc != 0)
       die("seccomp_load_filters failed with %i", rc);
 
+   // Permanently drop if not root
+   if (geteuid() == 0) {
+      // Note that we do not call setgroups() here because its ok
+      // that the user keeps the groups he already belongs to
+      if (setgid(real_gid) != 0)
+         die("setgid failed");
+      if (setuid(real_uid) != 0)
+         die("setuid failed");
+
+      if(real_gid != 0 && (getuid() == 0 || geteuid() == 0))
+         die("permanently dropping privs did not work");
+      if(real_uid != 0 && (getgid() == 0 || getegid() == 0))
+         die("permanently dropping privs did not work");
+   }
+
    // and exec the new binary
    argv[NR_ARGS] = (char*)binary,
    execv(binary, (char *const*)&argv[NR_ARGS]);

=== modified file 'src/seccomp.c'
--- src/seccomp.c	2015-04-23 10:20:17 +0000
+++ src/seccomp.c	2016-03-21 17:30:32 +0000
@@ -49,6 +49,23 @@
    if (ctx == NULL)
       return ENOMEM;
 
+   // Disable NO_NEW_PRIVS because it interferes with exec transitions in
+   // AppArmor. Unfortunately this means that security policies must be very
+   // careful to not allow the following otherwise apps can escape the snadbox:
+   //   - seccomp syscall
+   //   - prctl with PR_SET_SECCOMP
+   //   - ptrace (trace) in AppArmor
+   //   - capability sys_admin in AppArmor
+   // Note that with NO_NEW_PRIVS disabled, CAP_SYS_ADMIN is required to change
+   // the seccomp sandbox.
+   if (getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) {
+      rc = seccomp_attr_set(ctx, SCMP_FLTATR_CTL_NNP, 0);
+      if (rc != 0) {
+         fprintf(stderr, "Cannot disable nnp\n");
+         return -1;
+      }
+   }
+
    if (getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR") != NULL)
       filter_profile_dir = getenv("SNAPPY_LAUNCHER_SECCOMP_PROFILE_DIR");
 
@@ -110,15 +127,32 @@
       }
    }
 
+   // raise privileges to load seccomp policy since we don't have nnp
+   if (getenv("UBUNTU_CORE_LAUNCHER_NO_ROOT") == NULL) {
+      if (seteuid(0) != 0)
+         die("seteuid failed");
+      if (geteuid() != 0)
+         die("raising privs before seccomp_load did not work");
+   }
+
    // load it into the kernel
    rc = seccomp_load(ctx);
+
    if (rc != 0) {
       fprintf(stderr, "seccomp_load failed with %i\n", rc);
       goto out;
    }
 
-
  out:
+   // drop privileges again
+   if (geteuid() == 0) {
+      unsigned real_uid = getuid();
+      if (seteuid(real_uid) != 0)
+         die("seteuid failed");
+      if (real_uid != 0 && geteuid() == 0)
+         die("dropping privs after seccomp_load did not work");
+   }
+
    if (f != NULL) {
       fclose(f);
    }

=== modified file 'tests/test_create_user_data'
--- tests/test_create_user_data	2016-02-09 20:13:51 +0000
+++ tests/test_create_user_data	2016-03-21 17:30:32 +0000
@@ -28,6 +28,10 @@
 arch_prctl
 exit_group
 
+# required for not setting nnp
+geteuid
+getuid
+
 # unknown syscalls are ignore
 i-dont-exit
 EOF

=== modified file 'tests/test_restrictions_working'
--- tests/test_restrictions_working	2015-04-20 12:38:18 +0000
+++ tests/test_restrictions_working	2016-03-21 17:30:32 +0000
@@ -23,6 +23,9 @@
 arch_prctl
 exit_group
 
+geteuid
+getuid
+
 # unknown syscalls are ignore
 i-dont-exit
 EOF