From c8176efe2e2dc3aadc69c32b6aa9180751778e8f Mon Sep 17 00:00:00 2001 From: Victor Hsieh Date: Mon, 8 Jan 2018 12:43:00 -0800 Subject: [PATCH] Reland: Move zygote's seccomp setup to post-fork Before this change, seccomp filter setup is as early as in zygote's main function. To make it possible to split app and system server's filter, this postpone the setup to after fork. It also starts to call app specific and system server specific setup function. The filter setup is done in Zygote's ForkAndSpecializeCommon. This is because adding a seccomp filter must be done when either the caller has CAP_SYS_ADMIN or after the PR_SET_NO_NEW_PRIVS bit is set. Given that setting PR_SET_NO_NEW_PRIVS breaks SELinux domain transition (b/71859146), this must be done after Zygote forks but before CAP_SYS_ADMIN is droppped. Test: (cts) -m CtsSecurityTestCases -t android.security.cts.SeccompTest Test: no selinux denial flood in dmesg with selinux enforced Test: debuggerd -b `pidof com.android.phone` # logcat shows tombstoned received crash request Bug: 63944145 Bug: 71859146 Change-Id: I8215c8530d3d0de504a270488f8e29635805e8b0 --- core/java/android/os/Seccomp.java | 24 ---------- core/java/com/android/internal/os/Zygote.java | 3 ++ .../com/android/internal/os/ZygoteInit.java | 6 +-- core/jni/Android.bp | 1 - core/jni/AndroidRuntime.cpp | 2 - core/jni/android_os_seccomp.cpp | 47 ------------------- core/jni/com_android_internal_os_Zygote.cpp | 30 ++++++++++++ 7 files changed, 35 insertions(+), 78 deletions(-) delete mode 100644 core/java/android/os/Seccomp.java delete mode 100644 core/jni/android_os_seccomp.cpp diff --git a/core/java/android/os/Seccomp.java b/core/java/android/os/Seccomp.java deleted file mode 100644 index f14e93fe9403a..0000000000000 --- a/core/java/android/os/Seccomp.java +++ /dev/null @@ -1,24 +0,0 @@ -/* - * Copyright (C) 2017 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package android.os; - -/** - * @hide - */ -public final class Seccomp { - public static final native void setPolicy(); -} diff --git a/core/java/com/android/internal/os/Zygote.java b/core/java/com/android/internal/os/Zygote.java index 3ee8b472869b6..9167076474ffa 100644 --- a/core/java/com/android/internal/os/Zygote.java +++ b/core/java/com/android/internal/os/Zygote.java @@ -69,6 +69,9 @@ public final class Zygote { private Zygote() {} + /** Called for some security initialization before any fork. */ + native static void nativeSecurityInit(); + /** * Forks a new VM instance. The current VM must have been started * with the -Xzygote flag. NOTE: new instance keeps all diff --git a/core/java/com/android/internal/os/ZygoteInit.java b/core/java/com/android/internal/os/ZygoteInit.java index 212cdcbce0b1c..21f1fb6527945 100644 --- a/core/java/com/android/internal/os/ZygoteInit.java +++ b/core/java/com/android/internal/os/ZygoteInit.java @@ -30,7 +30,6 @@ import android.os.IInstalld; import android.os.Environment; import android.os.Process; import android.os.RemoteException; -import android.os.Seccomp; import android.os.ServiceManager; import android.os.ServiceSpecificException; import android.os.SystemClock; @@ -781,12 +780,11 @@ public class ZygoteInit { // Zygote. Trace.setTracingEnabled(false, 0); + Zygote.nativeSecurityInit(); + // Zygote process unmounts root storage spaces. Zygote.nativeUnmountStorageOnInit(); - // Set seccomp policy - Seccomp.setPolicy(); - ZygoteHooks.stopZygoteNoThreadCreation(); if (startSystemServer) { diff --git a/core/jni/Android.bp b/core/jni/Android.bp index 551d54ab90533..bc98716ebc9cb 100644 --- a/core/jni/Android.bp +++ b/core/jni/Android.bp @@ -86,7 +86,6 @@ cc_library_shared { "android_os_MessageQueue.cpp", "android_os_Parcel.cpp", "android_os_SELinux.cpp", - "android_os_seccomp.cpp", "android_os_SharedMemory.cpp", "android_os_SystemClock.cpp", "android_os_SystemProperties.cpp", diff --git a/core/jni/AndroidRuntime.cpp b/core/jni/AndroidRuntime.cpp index 047fa8489453c..35ab56a1a4565 100644 --- a/core/jni/AndroidRuntime.cpp +++ b/core/jni/AndroidRuntime.cpp @@ -163,7 +163,6 @@ extern int register_android_os_Parcel(JNIEnv* env); extern int register_android_os_SELinux(JNIEnv* env); extern int register_android_os_VintfObject(JNIEnv *env); extern int register_android_os_VintfRuntimeInfo(JNIEnv *env); -extern int register_android_os_seccomp(JNIEnv* env); extern int register_android_os_SystemProperties(JNIEnv *env); extern int register_android_os_SystemClock(JNIEnv* env); extern int register_android_os_Trace(JNIEnv* env); @@ -1420,7 +1419,6 @@ static const RegJNIRec gRegJNI[] = { REG_JNI(register_android_os_GraphicsEnvironment), REG_JNI(register_android_os_MessageQueue), REG_JNI(register_android_os_SELinux), - REG_JNI(register_android_os_seccomp), REG_JNI(register_android_os_Trace), REG_JNI(register_android_os_UEventObserver), REG_JNI(register_android_net_LocalSocketImpl), diff --git a/core/jni/android_os_seccomp.cpp b/core/jni/android_os_seccomp.cpp deleted file mode 100644 index 06e2a167de0a8..0000000000000 --- a/core/jni/android_os_seccomp.cpp +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright (C) 2017 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include "core_jni_helpers.h" -#include -#include "utils/Log.h" -#include - -#include "seccomp_policy.h" - -static void Seccomp_setPolicy(JNIEnv* /*env*/) { - if (security_getenforce() == 0) { - ALOGI("seccomp disabled by setenforce 0"); - return; - } - - if (!set_seccomp_filter()) { - ALOGE("Failed to set seccomp policy - killing"); - exit(1); - } -} - -static const JNINativeMethod method_table[] = { - NATIVE_METHOD(Seccomp, setPolicy, "()V"), -}; - -namespace android { - -int register_android_os_seccomp(JNIEnv* env) { - return android::RegisterMethodsOrDie(env, "android/os/Seccomp", - method_table, NELEM(method_table)); -} - -} diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index 32ef3dc0aed4b..63dba43a5eb39 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include "core_jni_helpers.h" @@ -76,6 +77,8 @@ static const char kZygoteClassName[] = "com/android/internal/os/Zygote"; static jclass gZygoteClass; static jmethodID gCallPostForkChildHooks; +static bool g_is_security_enforced = true; + // Must match values in com.android.internal.os.Zygote. enum MountExternalKind { MOUNT_EXTERNAL_NONE = 0, @@ -229,6 +232,20 @@ static void PreApplicationInit() { mallopt(M_DECAY_TIME, 1); } +static void SetUpSeccompFilter(uid_t uid) { + if (!g_is_security_enforced) { + ALOGI("seccomp disabled by setenforce 0"); + return; + } + + // Apply system or app filter based on uid. + if (getuid() >= AID_APP_START) { + set_app_seccomp_filter(); + } else { + set_system_seccomp_filter(); + } +} + static void EnableKeepCapabilities(JNIEnv* env) { int rc = prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0); if (rc == -1) { @@ -541,6 +558,11 @@ static pid_t ForkAndSpecializeCommon(JNIEnv* env, uid_t uid, gid_t gid, jintArra RuntimeAbort(env, __LINE__, "Call to sigprocmask(SIG_UNBLOCK, { SIGCHLD }) failed."); } + // Must be called when the new process still has CAP_SYS_ADMIN. The other alternative is to + // call prctl(PR_SET_NO_NEW_PRIVS, 1) afterward, but that breaks SELinux domain transition (see + // b/71859146). + SetUpSeccompFilter(uid); + // Keep capabilities across UID change, unless we're staying root. if (uid != 0) { EnableKeepCapabilities(env); @@ -698,6 +720,12 @@ static uint64_t GetEffectiveCapabilityMask(JNIEnv* env) { namespace android { +static void com_android_internal_os_Zygote_nativeSecurityInit(JNIEnv*, jclass) { + // security_getenforce is not allowed on app process. Initialize and cache the value before + // zygote forks. + g_is_security_enforced = security_getenforce(); +} + static void com_android_internal_os_Zygote_nativePreApplicationInit(JNIEnv*, jclass) { PreApplicationInit(); } @@ -832,6 +860,8 @@ static void com_android_internal_os_Zygote_nativeUnmountStorageOnInit(JNIEnv* en } static const JNINativeMethod gMethods[] = { + { "nativeSecurityInit", "()V", + (void *) com_android_internal_os_Zygote_nativeSecurityInit }, { "nativeForkAndSpecialize", "(II[II[[IILjava/lang/String;Ljava/lang/String;[I[ILjava/lang/String;Ljava/lang/String;)I", (void *) com_android_internal_os_Zygote_nativeForkAndSpecialize },