GitHub PullRequestReviewCommentEvent at 2014-05-14 00:49:14
Repository: sandstorm
Actor: kentonv
Payload: {"comment":{"url":"https://api.github.com/repos/kentonv/sandstorm/pulls/comments/12616743","id":12616743,"diff_hunk":"@@ -577,63 +569,59 @@ private:\n     KJ_SYSCALL(setdomainname(\"sandbox\", 7));\n   }\n \n-  void setupTmpfs() {\n-    // Create a new tmpfs for this run.  We don't use a shared one or just /tmp for two reasons:\n-    // 1) tmpfs has no quota control, so a shared instance could be DoS'd by any one grain, or\n-    //    just used to effectively allocate more RAM than the grain is allowed.\n-    // 2) When we exit, the mount namespace disappears and the tmpfs is thus automatically\n-    //    unmounted.  No need for careful cleanup, and no need to implement a risky recursive\n-    //    delete.\n-    KJ_SYSCALL(mount(\"tmpfs\", \"/tmp/sandstorm-grain\", \"tmpfs\", MS_NOATIME | MS_NOSUID | MS_NOEXEC,\n-                     kj::str(\"size=16m,nr_inodes=4k,mode=770,uid=\", uid, \",gid=\", gid).cStr()));\n-\n-    // Change to that directory.\n-    KJ_SYSCALL(chdir(\"/tmp/sandstorm-grain\"));\n-\n-    // Set up the directory tree.\n-\n-    // Create a minimal dev directory.\n-    KJ_SYSCALL(mkdir(\"dev\", 0755));\n-    KJ_SYSCALL(mknod(\"dev/null\", S_IFCHR | 0666, makedev(1, 3)));\n-    KJ_SYSCALL(mknod(\"dev/zero\", S_IFCHR | 0666, makedev(1, 5)));\n-    KJ_SYSCALL(mknod(\"dev/random\", S_IFCHR | 0666, makedev(1, 8)));\n-    KJ_SYSCALL(mknod(\"dev/urandom\", S_IFCHR | 0666, makedev(1, 9)));\n-\n-    // Mount point for var directory, as seen by the supervisor.\n-    KJ_SYSCALL(mkdir(\"var\", 0777));\n-\n-    // Temp directory.\n-    KJ_SYSCALL(mkdir(\"tmp\", 0777));\n-    KJ_SYSCALL(mkdir(\"tmp/sandbox\", 0777));  // Piece of tmp visible to sandbox.\n-\n-    // The root directory of the sandbox.\n-    KJ_SYSCALL(mkdir(\"sandbox\", 0777));\n-  }\n+  void setupFilesystem() {\n+    // The root of our mount namespace will be the app package itself.  We optionally create\n+    // tmp, dev, and var.  tmp is an ordinary tmpfs.  dev is a read-only tmpfs that contains\n+    // a few safe device nodes.  var is the 'var/sandbox' directory inside the grain.\n+    //\n+    // Now for the tricky part: the supervisor needs to be able to see a little bit more.\n+    // In particular, it needs to be able to see the entire var directory inside the grain.\n+    // We do this by temporarily hijacking the package's usr directory.  That will become\n+    // the supervisor's privileged space.  We detach it before starting the app, so the\n+    // app will just see its own usr directory.\n \n-  void bindDirs() {\n     // Bind the app package to \"sandbox\", which will be the grain's root directory.\n-    bind(pkgPath, \"sandbox\", MS_NODEV | MS_RDONLY);\n+    bind(pkgPath, \"/tmp/sandstorm-grain\", MS_NODEV | MS_RDONLY);\n \n-    // We want to chroot the supervisor.  It will need access to the var directory, so we need to\n-    // bind-mount that into the local tree.  We can't just map it to sandbox/var because part of\n-    // the var directory is supposed to be visible only to the supervisor.\n-    bind(varPath, \"var\", MS_NODEV | MS_NOEXEC);\n+    // Change to that directory.\n+    KJ_SYSCALL(chdir(\"/tmp/sandstorm-grain\"));\n \n     // Optionally bind var, tmp, dev if the app requests it by having the corresponding directories\n     // in the package.\n-    if (access(\"sandbox/tmp\", F_OK) == 0) {\n-      bind(\"tmp/sandbox\", \"sandbox/tmp\", MS_NODEV | MS_NOEXEC);\n+    if (access(\"tmp\", F_OK) == 0) {\n+      // Create a new tmpfs for this run.  We don't use a shared one or just /tmp for two reasons:\n+      // 1) tmpfs has no quota control, so a shared instance could be DoS'd by any one grain, or\n+      //    just used to effectively allocate more RAM than the grain is allowed.\n+      // 2) When we exit, the mount namespace disappears and the tmpfs is thus automatically\n+      //    unmounted.  No need for careful cleanup, and no need to implement a risky recursive\n+      //    delete.\n+      KJ_SYSCALL(mount(\"sandstorm-tmp\", \"tmp\", \"tmpfs\", MS_NOATIME | MS_NOSUID | MS_NOEXEC,\n+                     kj::str(\"size=16m,nr_inodes=4k,mode=770,uid=\", uid, \",gid=\", gid).cStr()));\n     }\n-    if (access(\"sandbox/dev\", F_OK) == 0) {\n-      bind(\"dev\", \"sandbox/dev\", MS_NOEXEC | MS_RDONLY);\n+    if (access(\"dev\", F_OK) == 0) {\n+      KJ_SYSCALL(mount(\"sandstorm-dev\", \"dev\", \"tmpfs\", MS_NOATIME | MS_NOSUID | MS_NOEXEC,\n+                     kj::str(\"size=1m,nr_inodes=16,mode=755,uid=\", uid, \",gid=\", gid).cStr()));\n+      KJ_SYSCALL(mknod(\"dev/null\", S_IFCHR | 0666, makedev(1, 3)));\n+      KJ_SYSCALL(mknod(\"dev/zero\", S_IFCHR | 0666, makedev(1, 5)));\n+      KJ_SYSCALL(mknod(\"dev/random\", S_IFCHR | 0666, makedev(1, 8)));\n+      KJ_SYSCALL(mknod(\"dev/urandom\", S_IFCHR | 0666, makedev(1, 9)));\n+      KJ_SYSCALL(mount(\"dev\", \"dev\", nullptr,\n+\t\t       MS_REMOUNT | MS_NOSUID | MS_NOATIME | MS_RDONLY, nullptr));\n     }\n-    if (access(\"sandbox/var\", F_OK) == 0) {\n-      bind(kj::str(varPath, \"/sandbox\"), \"sandbox/var\", MS_NODEV | MS_NOEXEC);\n+    if (access(\"var\", F_OK) == 0) {\n+      bind(kj::str(varPath, \"/sandbox\"), \"var\", MS_NODEV | MS_NOEXEC);\n     }\n \n-    // OK, everything is bound, so we can chroot.\n-    KJ_SYSCALL(chroot(\".\"));\n-    KJ_SYSCALL(chdir(\"/\"));\n+    // Set up the supervisor's directory.  This abuses directories that we know to already exist.\n+    bind(varPath, \"usr\", MS_NODEV | MS_NOEXEC);\n+\n+    // OK, everything is bound, so we can pivot_root.\n+    KJ_SYSCALL(syscall(SYS_pivot_root, \".\", \"usr/sandbox\"));\n+    KJ_SYSCALL(chdir(\"/usr\"));\n+    KJ_SYSCALL(umount2(\"sandbox\", MNT_DETACH));\n+    KJ_SYSCALL(umount2(\"/usr\", MNT_DETACH));\n+\n+    // Now '.' is the grain's var and '/' is the sandbox directory.","path":"src/sandstorm/supervisor-main.c++","position":148,"original_position":148,"commit_id":"a03c8a9e08d26b4b7128e746d5ff314f4ba54649","original_commit_id":"a03c8a9e08d26b4b7128e746d5ff314f4ba54649","user":{"login":"kentonv","id":4001805,"avatar_url":"https://avatars.githubusercontent.com/u/4001805?","gravatar_id":"b3abf6725a00a9bdc602fa02f4a75ca2","url":"https://api.github.com/users/kentonv","html_url":"https://github.com/kentonv","followers_url":"https://api.github.com/users/kentonv/followers","following_url":"https://api.github.com/users/kentonv/following{/other_user}","gists_url":"https://api.github.com/users/kentonv/gists{/gist_id}","starred_url":"https://api.github.com/users/kentonv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kentonv/subscriptions","organizations_url":"https://api.github.com/users/kentonv/orgs","repos_url":"https://api.github.com/users/kentonv/repos","events_url":"https://api.github.com/users/kentonv/events{/privacy}","received_events_url":"https://api.github.com/users/kentonv/received_events","type":"User","site_admin":false},"body":"I'm worried that having the supervisor's root be the application package could lead to vulnerabilities. The supervisor's glibc could decide that it wants to dynamically load, say, the DNS resolver library, and it would probably take the one in the app package. This obviously leads to trivial compromise of the supervisor.\r\n\r\nWould it work to have the supervisor do `chroot(\".\")` post-fork?","created_at":"2014-05-14T00:49:14Z","updated_at":"2014-05-14T00:49:14Z","html_url":"https://github.com/kentonv/sandstorm/pull/11#discussion_r12616743","pull_request_url":"https://api.github.com/repos/kentonv/sandstorm/pulls/11","_links":{"self":{"href":"https://api.github.com/repos/kentonv/sandstorm/pulls/comments/12616743"},"html":{"href":"https://github.com/kentonv/sandstorm/pull/11#discussion_r12616743"},"pull_request":{"href":"https://api.github.com/repos/kentonv/sandstorm/pulls/11"}}}}