DO NOT MERGE - Merge pi-dev@5234907 into stage-aosp-master
Bug: 120848293
Change-Id: I99f9bb2968410e111b83f3f1428054d32d1914e5
diff --git a/Android.bp b/Android.bp
index 93c4543..7bce0f1 100644
--- a/Android.bp
+++ b/Android.bp
@@ -35,7 +35,6 @@
cflags: [
"-D_FILE_OFFSET_BITS=64",
"-DALLOW_DEBUG_LOGGING",
- "-DHAVE_SECUREBITS_H",
"-Wall",
"-Werror",
],
diff --git a/Makefile b/Makefile
index d58d2ad..9a13d8e 100644
--- a/Makefile
+++ b/Makefile
@@ -10,10 +10,6 @@
PRELOADPATH = \"$(LIBDIR)/$(PRELOADNAME)\"
CPPFLAGS += -DPRELOADPATH="$(PRELOADPATH)"
-ifneq ($(HAVE_SECUREBITS_H),no)
-CPPFLAGS += -DHAVE_SECUREBITS_H
-endif
-
ifeq ($(USE_seccomp),no)
CPPFLAGS += -DUSE_SECCOMP_SOFTFAIL
endif
@@ -55,7 +51,7 @@
libconstants.gen.o libsyscalls.gen.o
all: CC_BINARY(minijail0) CC_LIBRARY(libminijail.so) \
- CC_LIBRARY(libminijailpreload.so) constants.json
+ CC_LIBRARY(libminijailpreload.so)
parse_seccomp_policy: CXX_BINARY(parse_seccomp_policy)
dump_constants: CXX_BINARY(dump_constants)
diff --git a/arch.h b/arch.h
index 652f072..2e60313 100644
--- a/arch.h
+++ b/arch.h
@@ -11,10 +11,13 @@
#include <linux/audit.h>
+/* clang-format off */
#if defined(__i386__)
# define ARCH_NR AUDIT_ARCH_I386
+# define ARCH_NAME "x86"
#elif defined(__x86_64__)
# define ARCH_NR AUDIT_ARCH_X86_64
+# define ARCH_NAME "x86_64"
#elif defined(__arm__)
/*
* <linux/audit.h> includes <linux/elf-em.h>, which does not define EM_ARM.
@@ -24,42 +27,57 @@
# define EM_ARM 40
# endif
# define ARCH_NR AUDIT_ARCH_ARM
+# define ARCH_NAME "arm"
#elif defined(__aarch64__)
# define ARCH_NR AUDIT_ARCH_AARCH64
+# define ARCH_NAME "arm64"
#elif defined(__hppa__)
# define ARCH_NR AUDIT_ARCH_PARISC
+# define ARCH_NAME "parisc"
#elif defined(__ia64__)
# define ARCH_NR AUDIT_ARCH_IA64
+# define ARCH_NAME "ia64"
#elif defined(__mips__)
# if defined(__mips64)
# if defined(__MIPSEB__)
# define ARCH_NR AUDIT_ARCH_MIPS64
+# define ARCH_NAME "mips64"
# else
# define ARCH_NR AUDIT_ARCH_MIPSEL64
+# define ARCH_NAME "mipsel64"
# endif
# else
# if defined(__MIPSEB__)
# define ARCH_NR AUDIT_ARCH_MIPS
+# define ARCH_NAME "mips"
# else
# define ARCH_NR AUDIT_ARCH_MIPSEL
+# define ARCH_NAME "mipsel"
# endif
# endif
#elif defined(__powerpc64__)
# define ARCH_NR AUDIT_ARCH_PPC64
+# define ARCH_NAME "ppc64"
#elif defined(__powerpc__)
# define ARCH_NR AUDIT_ARCH_PPC
+# define ARCH_NAME "ppc"
#elif defined(__s390x__)
# define ARCH_NR AUDIT_ARCH_S390X
+# define ARCH_NAME "s390x"
#elif defined(__s390__)
# define ARCH_NR AUDIT_ARCH_S390
+# define ARCH_NAME "s390"
#elif defined(__sparc__)
# if defined(__arch64__)
# define AUDIT_ARCH_SPARC64
+# define ARCH_NAME "sparc64"
# else
# define AUDIT_ARCH_SPARC
+# define ARCH_NAME "sparc"
# endif
#else
# error "AUDIT_ARCH value unavailable"
#endif
+/* clang-format on */
#endif /* ARCH_H */
diff --git a/dump_constants.cc b/dump_constants.cc
index f33bd5b..4603809 100644
--- a/dump_constants.cc
+++ b/dump_constants.cc
@@ -17,6 +17,7 @@
int main() {
std::cout << "{\n";
std::cout << " \"arch_nr\": " << ARCH_NR << ",\n";
+ std::cout << " \"arch_name\": \"" << ARCH_NAME << "\",\n";
std::cout << " \"bits\": " << (sizeof(uintptr_t) * 8) << ",\n";
std::cout << " \"syscalls\": {\n";
bool first = true;
diff --git a/libminijail.c b/libminijail.c
index e44183f..482d6bd 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -492,6 +492,13 @@
j->flags.do_init = 1;
}
+void API minijail_namespace_pids_rw_proc(struct minijail *j)
+{
+ j->flags.vfs = 1;
+ j->flags.pids = 1;
+ j->flags.do_init = 1;
+}
+
void API minijail_namespace_ipc(struct minijail *j)
{
j->flags.ipc = 1;
@@ -771,8 +778,6 @@
flags = MS_NODEV | MS_NOEXEC | MS_NOSUID;
m->flags = flags;
- info("mount '%s' -> '%s' type '%s' flags %#lx", src, dest, type, flags);
-
/*
* Force vfs namespacing so the mounts don't leak out into the
* containing vfs namespace.
@@ -2159,23 +2164,19 @@
}
/*
- * POSIX capabilities are a bit tricky. If we drop our capability to
- * change uids, our attempt to use drop_ugid() below will fail. Hang on
- * to root caps across drop_ugid(), then lock securebits.
+ * POSIX capabilities are a bit tricky. We must set SECBIT_KEEP_CAPS
+ * before drop_ugid() below as the latter would otherwise drop all
+ * capabilities.
*/
if (j->flags.use_caps) {
/*
- * Using ambient capabilities takes care of most of the cases
- * where PR_SET_KEEPCAPS would be needed, but still try to set
- * them unless it is locked (maybe due to running minijail
- * within an already-minijailed process).
+ * When using ambient capabilities, CAP_SET{GID,UID} can be
+ * inherited across execve(2), so SECBIT_KEEP_CAPS is not
+ * strictly needed.
*/
- if (!j->flags.set_ambient_caps || !secure_keep_caps_locked()) {
- if (prctl(PR_SET_KEEPCAPS, 1))
- pdie("prctl(PR_SET_KEEPCAPS) failed");
- }
-
- if (lock_securebits(j->securebits_skip_mask) < 0) {
+ bool require_keep_caps = !j->flags.set_ambient_caps;
+ if (lock_securebits(j->securebits_skip_mask,
+ require_keep_caps) < 0) {
pdie("locking securebits failed");
}
}
@@ -2314,19 +2315,26 @@
#else
const char *preload_path = j->preload_path ?: PRELOADPATH;
char *newenv = NULL;
+ int ret = 0;
if (!oldenv)
oldenv = "";
/* Only insert a separating space if we have something to separate... */
- if (asprintf(&newenv, "%s=%s%s%s", kLdPreloadEnvVar, oldenv,
- oldenv[0] != '\0' ? " " : "", preload_path) < 0) {
- return -ENOMEM;
+ if (asprintf(&newenv, "%s%s%s", oldenv, oldenv[0] != '\0' ? " " : "",
+ preload_path) < 0) {
+ return -1;
}
- /* putenv(3) will now own the string. */
- putenv(newenv);
- return 0;
+ /*
+ * Avoid using putenv(3), since that requires us to hold onto a
+ * reference to that string until the environment is no longer used to
+ * prevent a memory leak.
+ * See https://crbug.com/930189 for more details.
+ */
+ ret = setenv(kLdPreloadEnvVar, newenv, 1);
+ free(newenv);
+ return ret;
#endif
}
@@ -2409,15 +2417,18 @@
/*
* Structure that specifies how to start a minijail.
*
- * filename - The program to exec in the child. Required if `exec_in_child` = 1.
- * argv - Arguments for the child program. Required if `exec_in_child` = 1.
+ * filename - The program to exec in the child. Required if |exec_in_child| = 1.
+ * argv - Arguments for the child program. Required if |exec_in_child| = 1.
+ * envp - Environment for the child program. Available if |exec_in_child| = 1.
+ Currently only honored if |use_preload| = 0 and non-NULL.
* use_preload - If true use LD_PRELOAD.
- * exec_in_child - If true, run `filename`. Otherwise, the child will return to
+ * exec_in_child - If true, run |filename|. Otherwise, the child will return to
* the caller.
*/
struct minijail_run_config {
const char *filename;
char *const *argv;
+ char *const *envp;
int use_preload;
int exec_in_child;
};
@@ -2448,6 +2459,7 @@
struct minijail_run_config config = {
.filename = filename,
.argv = argv,
+ .envp = NULL,
.use_preload = true,
.exec_in_child = true,
};
@@ -2461,6 +2473,7 @@
struct minijail_run_config config = {
.filename = filename,
.argv = argv,
+ .envp = NULL,
.use_preload = true,
.exec_in_child = true,
};
@@ -2476,6 +2489,7 @@
struct minijail_run_config config = {
.filename = filename,
.argv = argv,
+ .envp = NULL,
.use_preload = true,
.exec_in_child = true,
};
@@ -2492,6 +2506,7 @@
struct minijail_run_config config = {
.filename = filename,
.argv = argv,
+ .envp = NULL,
.use_preload = true,
.exec_in_child = true,
};
@@ -2510,6 +2525,7 @@
struct minijail_run_config config = {
.filename = filename,
.argv = argv,
+ .envp = NULL,
.use_preload = false,
.exec_in_child = true,
};
@@ -2528,6 +2544,30 @@
struct minijail_run_config config = {
.filename = filename,
.argv = argv,
+ .envp = NULL,
+ .use_preload = false,
+ .exec_in_child = true,
+ };
+ struct minijail_run_status status = {
+ .pstdin_fd = pstdin_fd,
+ .pstdout_fd = pstdout_fd,
+ .pstderr_fd = pstderr_fd,
+ .pchild_pid = pchild_pid,
+ };
+ return minijail_run_internal(j, &config, &status);
+}
+
+int API minijail_run_env_pid_pipes_no_preload(struct minijail *j,
+ const char *filename,
+ char *const argv[],
+ char *const envp[],
+ pid_t *pchild_pid, int *pstdin_fd,
+ int *pstdout_fd, int *pstderr_fd)
+{
+ struct minijail_run_config config = {
+ .filename = filename,
+ .argv = argv,
+ .envp = envp,
.use_preload = false,
.exec_in_child = true,
};
@@ -2574,6 +2614,8 @@
die("Minijail hooks are not supported with LD_PRELOAD");
if (!config->exec_in_child)
die("minijail_fork is not supported with LD_PRELOAD");
+ if (config->envp != NULL)
+ die("cannot pass a new environment with LD_PRELOAD");
oldenv = getenv(kLdPreloadEnvVar);
if (oldenv) {
@@ -2954,6 +2996,18 @@
return 0;
/*
+ * If not using LD_PRELOAD, support passing a new environment instead of
+ * inheriting the parent's.
+ * When not using LD_PRELOAD there is no need to modify the environment
+ * to add Minijail-related variables, so passing a new environment is
+ * fine.
+ */
+ char *const *child_env = environ;
+ if (!use_preload && config->envp != NULL) {
+ child_env = config->envp;
+ }
+
+ /*
* If we aren't pid-namespaced, or the jailed program asked to be init:
* calling process
* -> execve()-ing process
@@ -2962,7 +3016,7 @@
* -> init()-ing process
* -> execve()-ing process
*/
- ret = execve(config->filename, config->argv, environ);
+ ret = execve(config->filename, config->argv, child_env);
if (ret == -1) {
pwarn("execve(%s) failed", config->filename);
}
diff --git a/libminijail.h b/libminijail.h
index da8244d..0853c17 100644
--- a/libminijail.h
+++ b/libminijail.h
@@ -53,7 +53,7 @@
/* The hook will run just before calling execve(2). */
MINIJAIL_HOOK_EVENT_PRE_EXECVE,
- /* The hook will run just before calling chroot(2) / pivot_root(2). */
+ /* The hook will run just before calling chroot(2) / pivot_root(2). */
MINIJAIL_HOOK_EVENT_PRE_CHROOT,
/* Sentinel for error checking. Must be last. */
@@ -120,6 +120,15 @@
* WARNING: this is NOT THREAD SAFE. See the block comment in </libminijail.c>.
*/
void minijail_namespace_pids(struct minijail *j);
+/*
+ * Implies namespace_vfs.
+ * WARNING: this is NOT THREAD SAFE. See the block comment in </libminijail.c>.
+ * Minijail will by default remount /proc read-only when using a PID namespace.
+ * Certain complex applications expect to be able to do their own sandboxing
+ * which might require writing to /proc, so support a weaker version of PID
+ * namespacing with a RW /proc.
+ */
+void minijail_namespace_pids_rw_proc(struct minijail *j);
void minijail_namespace_user(struct minijail *j);
void minijail_namespace_user_disable_setgroups(struct minijail *j);
int minijail_uidmap(struct minijail *j, const char *uidmap);
@@ -295,7 +304,8 @@
/*
* Run the specified command in the given minijail, execve(2)-style.
- * Used with static binaries, or on systems without support for LD_PRELOAD.
+ * Don't use LD_PRELOAD to do privilege dropping. This is useful when sandboxing
+ * static binaries, or on systems without support for LD_PRELOAD.
*/
int minijail_run_no_preload(struct minijail *j, const char *filename,
char *const argv[]);
@@ -338,7 +348,8 @@
* standard output.
* Update |*pstderr_fd| with a fd that allows reading from the child's
* standard error.
- * Used with static binaries, or on systems without support for LD_PRELOAD.
+ * Don't use LD_PRELOAD to do privilege dropping. This is useful when sandboxing
+ * static binaries, or on systems without support for LD_PRELOAD.
*/
int minijail_run_pid_pipes_no_preload(struct minijail *j, const char *filename,
char *const argv[], pid_t *pchild_pid,
@@ -346,6 +357,26 @@
int *pstderr_fd);
/*
+ * Run the specified command in the given minijail, execve(2)-style.
+ * Pass |envp| as the full environment for the child.
+ * Update |*pchild_pid| with the pid of the child.
+ * Update |*pstdin_fd| with a fd that allows writing to the child's
+ * standard input.
+ * Update |*pstdout_fd| with a fd that allows reading from the child's
+ * standard output.
+ * Update |*pstderr_fd| with a fd that allows reading from the child's
+ * standard error.
+ * Don't use LD_PRELOAD to do privilege dropping. This is useful when sandboxing
+ * static binaries, or on systems without support for LD_PRELOAD.
+ */
+int minijail_run_env_pid_pipes_no_preload(struct minijail *j,
+ const char *filename,
+ char *const argv[],
+ char *const envp[], pid_t *pchild_pid,
+ int *pstdin_fd, int *pstdout_fd,
+ int *pstderr_fd);
+
+/*
* Fork, jail the child, and return. This behaves similar to fork(2), except it
* puts the child process in a jail before returning.
* `minijail_fork` returns in both the parent and the child. The pid of the
diff --git a/libminijail_unittest.cc b/libminijail_unittest.cc
index 4310e8b..25aa76e 100644
--- a/libminijail_unittest.cc
+++ b/libminijail_unittest.cc
@@ -320,6 +320,47 @@
minijail_destroy(j);
}
+TEST(Test, minijail_run_env_pid_pipes_no_preload) {
+ pid_t pid;
+ int child_stdin, child_stdout, child_stderr;
+ int mj_run_ret;
+ ssize_t read_ret;
+ char buf[kBufferSize];
+ int status;
+ char test_envvar[] = "TEST_VAR=test";
+ size_t testvar_len = strlen("test");
+ char *argv[4];
+ char *envp[2];
+
+ struct minijail *j = minijail_new();
+
+ argv[0] = (char*)kShellPath;
+ argv[1] = "-c";
+ argv[2] = "echo \"${TEST_PARENT+set}|${TEST_VAR}\"";
+ argv[3] = NULL;
+
+ envp[0] = test_envvar;
+ envp[1] = NULL;
+
+ // Set a canary env var in the parent that should not be present in the child.
+ ASSERT_EQ(setenv("TEST_PARENT", "test", 1 /*overwrite*/), 0);
+
+ mj_run_ret = minijail_run_env_pid_pipes_no_preload(
+ j, argv[0], argv, envp, &pid, &child_stdin, &child_stdout, &child_stderr);
+ EXPECT_EQ(mj_run_ret, 0);
+
+ read_ret = read(child_stdout, buf, sizeof(buf));
+ EXPECT_GE(read_ret, (int)testvar_len);
+
+ EXPECT_EQ("|test\n", std::string(buf));
+
+ EXPECT_EQ(waitpid(pid, &status, 0), pid);
+ ASSERT_TRUE(WIFEXITED(status));
+ EXPECT_EQ(WEXITSTATUS(status), 0);
+
+ minijail_destroy(j);
+}
+
TEST(Test, test_minijail_no_fd_leaks) {
pid_t pid;
int child_stdout;
@@ -419,13 +460,13 @@
status =
minijail_add_hook(j, &early_exit, reinterpret_cast<void *>(exit_code),
- MINIJAIL_HOOK_EVENT_PRE_DROP_CAPS);
+ MINIJAIL_HOOK_EVENT_PRE_DROP_CAPS);
EXPECT_EQ(status, 0);
argv[0] = (char*)kCatPath;
argv[1] = NULL;
mj_run_ret = minijail_run_pid_pipes_no_preload(j, argv[0], argv, &pid, NULL,
- NULL, NULL);
+ NULL, NULL);
EXPECT_EQ(mj_run_ret, 0);
status = minijail_wait(j);
diff --git a/scoped_minijail.h b/scoped_minijail.h
index 27bf487..38f1a91 100644
--- a/scoped_minijail.h
+++ b/scoped_minijail.h
@@ -10,7 +10,10 @@
#include "libminijail.h"
-namespace {
+namespace mj {
+
+namespace internal {
+
struct ScopedMinijailDeleter {
inline void operator()(minijail *j) const {
if (j) {
@@ -18,8 +21,12 @@
}
}
};
-}
-using ScopedMinijail = std::unique_ptr<minijail, ScopedMinijailDeleter>;
+} // namespace internal
+
+} // namespace mj
+
+using ScopedMinijail =
+ std::unique_ptr<minijail, mj::internal::ScopedMinijailDeleter>;
#endif /* _SCOPED_MINIJAIL_H_ */
diff --git a/system.c b/system.c
index 63f22d8..2dcf152 100644
--- a/system.c
+++ b/system.c
@@ -20,17 +20,21 @@
#include <sys/statvfs.h>
#include <unistd.h>
+#include <linux/securebits.h>
+
#include "util.h"
-#ifdef HAVE_SECUREBITS_H
-#include <linux/securebits.h>
-#else
-#define SECURE_ALL_BITS 0x55
-#define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
+/*
+ * SECBIT_NO_CAP_AMBIENT_RAISE was added in kernel 4.3, so fill in the
+ * definition if the securebits header doesn't provide it.
+ */
+#ifndef SECBIT_NO_CAP_AMBIENT_RAISE
+#define SECBIT_NO_CAP_AMBIENT_RAISE (issecure_mask(6))
#endif
-#define SECURE_BITS_NO_AMBIENT 0x15
-#define SECURE_LOCKS_NO_AMBIENT (SECURE_BITS_NO_AMBIENT << 1)
+#ifndef SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED
+#define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED (issecure_mask(7))
+#endif
/*
* Assert the value of SECURE_ALL_BITS at compile-time.
@@ -44,30 +48,49 @@
_Static_assert(SECURE_ALL_BITS == 0x55, "SECURE_ALL_BITS == 0x55.");
#endif
-int secure_keep_caps_locked(void)
-{
- int bits = prctl(PR_GET_SECUREBITS);
- if (bits < 0)
- return 0;
- return bits & SECBIT_KEEP_CAPS_LOCKED;
-}
-
int secure_noroot_set_and_locked(uint64_t mask)
{
return (mask & (SECBIT_NOROOT | SECBIT_NOROOT_LOCKED)) ==
(SECBIT_NOROOT | SECBIT_NOROOT_LOCKED);
}
-int lock_securebits(uint64_t skip_mask)
+int lock_securebits(uint64_t skip_mask, bool require_keep_caps)
{
+ /* The general idea is to set all bits, subject to exceptions below. */
+ unsigned long securebits = SECURE_ALL_BITS | SECURE_ALL_LOCKS;
+
+ /*
+ * SECBIT_KEEP_CAPS is special in that it is automatically cleared on
+ * execve(2). This implies that attempts to set SECBIT_KEEP_CAPS (as is
+ * the default) in processes that have it locked already (such as nested
+ * minijail usage) would fail. Thus, unless the caller requires it,
+ * allow it to remain off if it is already locked.
+ */
+ if (!require_keep_caps) {
+ int current_securebits = prctl(PR_GET_SECUREBITS);
+ if (current_securebits < 0) {
+ pwarn("prctl(PR_GET_SECUREBITS) failed");
+ return -1;
+ }
+
+ if ((current_securebits & SECBIT_KEEP_CAPS_LOCKED) != 0 &&
+ (current_securebits & SECBIT_KEEP_CAPS) == 0) {
+ securebits &= ~SECBIT_KEEP_CAPS;
+ }
+ }
+
/*
* Ambient capabilities can only be raised if they're already present
* in the permitted *and* inheritable set. Therefore, we don't really
* need to lock the NO_CAP_AMBIENT_RAISE securebit, since we are already
* configuring the permitted and inheritable set.
*/
- unsigned long securebits =
- (SECURE_BITS_NO_AMBIENT | SECURE_LOCKS_NO_AMBIENT) & ~skip_mask;
+ securebits &=
+ ~(SECBIT_NO_CAP_AMBIENT_RAISE | SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED);
+
+ /* Don't set any bits that the user requested not to be touched. */
+ securebits &= ~skip_mask;
+
if (!securebits) {
warn("not locking any securebits");
return 0;
diff --git a/system.h b/system.h
index 5007981..cd1a98a 100644
--- a/system.h
+++ b/system.h
@@ -38,9 +38,8 @@
#define PR_CAP_AMBIENT_CLEAR_ALL 4
#endif
-int secure_keep_caps_locked(void);
int secure_noroot_set_and_locked(uint64_t mask);
-int lock_securebits(uint64_t skip_mask);
+int lock_securebits(uint64_t skip_mask, bool require_keep_caps);
unsigned int get_last_valid_cap(void);
int cap_ambient_supported(void);
