From 86f08a5190c8a36497ff3b9848ce3e6d0ba2e951 Mon Sep 17 00:00:00 2001 From: Martijn Coenen Date: Thu, 3 Jan 2019 16:23:01 +0100 Subject: [PATCH] Prepare setresuid()/setresgid() seccomp filter in AppZygote. The application zygote can run untrusted user code; since it also has the capability to change the uid/gid of the process, we need to ensure that any changes to the uid and/or gid stay within the range that we have allocated for this application zygote. For application zygotes, we install the app_zygote seccomp filter instead of the regular app filter; the only difference between this filter and the app one is that it allows setuid/setgid calls. To further limit this, pass down the allocated UID range to the Zygote itself, which in turn installs an additional seccomp filter that restricts setuid/setgid calls to this range. The actual calls into seccomp are commented out until the seccomp changes are merged; to avoid catastrophe, this will leave the regular app filter for the app_zygote, which is more restrictive and doesn't allow setuid at all. Bug: 111434506 Test: atest CtsSeccompHostTestCases passes Change-Id: I112419629f5ee4774ccbf77e2b1cfa5ddcf77e73 --- core/java/android/os/AppZygote.java | 15 +++++++- core/java/android/os/ZygoteProcess.java | 10 ++++- core/java/android/webkit/WebViewZygote.java | 4 +- .../android/internal/os/ChildZygoteInit.java | 38 +++++++++++++++++++ core/java/com/android/internal/os/Zygote.java | 21 ++++++++++ core/jni/com_android_internal_os_Zygote.cpp | 34 +++++++++++++++-- .../com/android/server/am/ProcessList.java | 10 +++-- 7 files changed, 119 insertions(+), 13 deletions(-) diff --git a/core/java/android/os/AppZygote.java b/core/java/android/os/AppZygote.java index 40cbaf75c02dc..950f381677540 100644 --- a/core/java/android/os/AppZygote.java +++ b/core/java/android/os/AppZygote.java @@ -34,8 +34,15 @@ import com.android.internal.annotations.GuardedBy; public class AppZygote { private static final String LOG_TAG = "AppZygote"; + // UID of the Zygote itself private final int mZygoteUid; + // First UID/GID of the range the AppZygote can setuid()/setgid() to + private final int mZygoteUidGidMin; + + // Last UID/GID of the range the AppZygote can setuid()/setgid() to + private final int mZygoteUidGidMax; + private final Object mLock = new Object(); /** @@ -47,9 +54,11 @@ public class AppZygote { private final ApplicationInfo mAppInfo; - public AppZygote(ApplicationInfo appInfo, int zygoteUid) { + public AppZygote(ApplicationInfo appInfo, int zygoteUid, int uidGidMin, int uidGidMax) { mAppInfo = appInfo; mZygoteUid = zygoteUid; + mZygoteUidGidMin = uidGidMin; + mZygoteUidGidMax = uidGidMax; } /** @@ -104,7 +113,9 @@ public class AppZygote { "app_zygote", // seInfo abi, // abi abi, // acceptedAbiList - null); // instructionSet + null, // instructionSet + mZygoteUidGidMin, + mZygoteUidGidMax); ZygoteProcess.waitForConnectionToZygote(mZygote.getPrimarySocketAddress()); // preload application code in the zygote diff --git a/core/java/android/os/ZygoteProcess.java b/core/java/android/os/ZygoteProcess.java index 251c5eebadc4d..99a85eaefe537 100644 --- a/core/java/android/os/ZygoteProcess.java +++ b/core/java/android/os/ZygoteProcess.java @@ -809,6 +809,8 @@ public class ZygoteProcess { * may be different from abi in case the children * spawned from this Zygote only communicate using ABI-safe methods. * @param instructionSet null-ok the instruction set to use. + * @param uidRangeStart The first UID in the range the child zygote may setuid()/setgid() to + * @param uidRangeEnd The last UID in the range the child zygote may setuid()/setgid() to */ public ChildZygoteProcess startChildZygote(final String processClass, final String niceName, @@ -817,13 +819,17 @@ public class ZygoteProcess { String seInfo, String abi, String acceptedAbiList, - String instructionSet) { + String instructionSet, + int uidRangeStart, + int uidRangeEnd) { // Create an unguessable address in the global abstract namespace. final LocalSocketAddress serverAddress = new LocalSocketAddress( processClass + "/" + UUID.randomUUID().toString()); final String[] extraArgs = {Zygote.CHILD_ZYGOTE_SOCKET_NAME_ARG + serverAddress.getName(), - Zygote.CHILD_ZYGOTE_ABI_LIST_ARG + acceptedAbiList}; + Zygote.CHILD_ZYGOTE_ABI_LIST_ARG + acceptedAbiList, + Zygote.CHILD_ZYGOTE_UID_RANGE_START + uidRangeStart, + Zygote.CHILD_ZYGOTE_UID_RANGE_END + uidRangeEnd}; Process.ProcessStartResult result; try { diff --git a/core/java/android/webkit/WebViewZygote.java b/core/java/android/webkit/WebViewZygote.java index 9f7aa6a2852af..29b3b3cff0440 100644 --- a/core/java/android/webkit/WebViewZygote.java +++ b/core/java/android/webkit/WebViewZygote.java @@ -160,7 +160,9 @@ public class WebViewZygote { "webview_zygote", // seInfo sPackage.applicationInfo.primaryCpuAbi, // abi TextUtils.join(",", Build.SUPPORTED_ABIS), - null); // instructionSet + null, // instructionSet + Process.FIRST_ISOLATED_UID, + Process.LAST_ISOLATED_UID); // All the work below is usually done by LoadedApk, but the zygote can't talk to // PackageManager or construct a LoadedApk since it's single-threaded pre-fork, so diff --git a/core/java/com/android/internal/os/ChildZygoteInit.java b/core/java/com/android/internal/os/ChildZygoteInit.java index f90cd02245964..a052a3b3ab6ac 100644 --- a/core/java/com/android/internal/os/ChildZygoteInit.java +++ b/core/java/com/android/internal/os/ChildZygoteInit.java @@ -15,6 +15,7 @@ */ package com.android.internal.os; +import android.os.Process; import android.system.ErrnoException; import android.system.Os; import android.system.OsConstants; @@ -49,6 +50,22 @@ public class ChildZygoteInit { return null; } + static int parseIntFromArg(String[] argv, String desiredArg) { + int value = -1; + for (String arg : argv) { + if (arg.startsWith(desiredArg)) { + String valueStr = arg.substring(arg.indexOf('=') + 1); + try { + value = Integer.parseInt(valueStr); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid int argument: " + + valueStr, e); + } + } + } + return value; + } + /** * Starts a ZygoteServer and listens for requests * @@ -72,6 +89,27 @@ public class ChildZygoteInit { throw new RuntimeException("Failed to set PR_SET_NO_NEW_PRIVS", ex); } + int uidGidMin = parseIntFromArg(args, Zygote.CHILD_ZYGOTE_UID_RANGE_START); + int uidGidMax = parseIntFromArg(args, Zygote.CHILD_ZYGOTE_UID_RANGE_END); + if (uidGidMin == -1 || uidGidMax == -1) { + throw new RuntimeException("Couldn't parse UID range start/end"); + } + if (uidGidMin > uidGidMax) { + throw new RuntimeException("Passed in UID range is invalid, min > max."); + } + + // Verify the UIDs are in the isolated UID range, as that's the only thing that we should + // be forking right now + if (!Process.isIsolated(uidGidMin) || !Process.isIsolated(uidGidMax)) { + throw new RuntimeException("Passed in UID range does not map to isolated processes."); + } + + /** + * Install a seccomp filter that ensure this Zygote can only setuid()/setgid() + * to the passed in range. + */ + Zygote.nativeInstallSeccompUidGidFilter(uidGidMin, uidGidMax); + final Runnable caller; try { server.registerServerSocketAtAbstractName(socketName); diff --git a/core/java/com/android/internal/os/Zygote.java b/core/java/com/android/internal/os/Zygote.java index d720c689f5de1..f5746ca65f5ee 100644 --- a/core/java/com/android/internal/os/Zygote.java +++ b/core/java/com/android/internal/os/Zygote.java @@ -104,6 +104,20 @@ public final class Zygote { */ public static final String CHILD_ZYGOTE_ABI_LIST_ARG = "--abi-list="; + /** + * An extraArg passed when a zygote process is forking a child-zygote, specifying the + * start of the UID range the children of the Zygote may setuid()/setgid() to. This + * will be enforced with a seccomp filter. + */ + public static final String CHILD_ZYGOTE_UID_RANGE_START = "--uid-range-start="; + + /** + * An extraArg passed when a zygote process is forking a child-zygote, specifying the + * end of the UID range the children of the Zygote may setuid()/setgid() to. This + * will be enforced with a seccomp filter. + */ + public static final String CHILD_ZYGOTE_UID_RANGE_END = "--uid-range-end="; + private Zygote() {} /** Called for some security initialization before any fork. */ @@ -221,6 +235,13 @@ public final class Zygote { */ native protected static void nativeAllowFileAcrossFork(String path); + /** + * Installs a seccomp filter that limits setresuid()/setresgid() to the passed-in range + * @param uidGidMin The smallest allowed uid/gid + * @param uidGidMax The largest allowed uid/gid + */ + native protected static void nativeInstallSeccompUidGidFilter(int uidGidMin, int uidGidMax); + /** * Zygote unmount storage space on initializing. * This method is called once. diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index a8e142784397b..0ccbd2f2c7642 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -303,7 +303,7 @@ static void PreApplicationInit() { mallopt(M_DECAY_TIME, 1); } -static void SetUpSeccompFilter(uid_t uid) { +static void SetUpSeccompFilter(uid_t uid, bool is_child_zygote) { if (!g_is_security_enforced) { ALOGI("seccomp disabled by setenforce 0"); return; @@ -311,7 +311,14 @@ static void SetUpSeccompFilter(uid_t uid) { // Apply system or app filter based on uid. if (uid >= AID_APP_START) { - set_app_seccomp_filter(); + if (is_child_zygote) { + // set_app_zygote_seccomp_filter(); + // TODO(b/111434506) install the filter; for now, install the app filter + // which is more restrictive. + set_app_seccomp_filter(); + } else { + set_app_seccomp_filter(); + } } else { set_system_seccomp_filter(); } @@ -996,7 +1003,7 @@ static void SpecializeCommon(JNIEnv* env, uid_t uid, gid_t gid, jintArray gids, // alternative is to call prctl(PR_SET_NO_NEW_PRIVS, 1) afterward, but that // breaks SELinux domain transition (see b/71859146). As the result, // privileged syscalls used below still need to be accessible in app process. - SetUpSeccompFilter(uid); + SetUpSeccompFilter(uid, is_child_zygote); if (setresuid(uid, uid, uid) == -1) { fail_fn(CREATE_ERROR("setresuid(%d) failed: %s", uid, strerror(errno))); @@ -1295,6 +1302,23 @@ static void com_android_internal_os_Zygote_nativeUnmountStorageOnInit(JNIEnv* en UnmountTree("/storage"); } +static void com_android_internal_os_Zygote_nativeInstallSeccompUidGidFilter( + JNIEnv* env, jclass, jint uidGidMin, jint uidGidMax) { + if (!g_is_security_enforced) { + ALOGI("seccomp disabled by setenforce 0"); + return; + } + + // TODO(b/111434506) install the filter + + /* + bool installed = install_setuidgid_seccomp_filter(uidGidMin, uidGidMax); + if (!installed) { + RuntimeAbort(env, __LINE__, "Could not install setuid/setgid seccomp filter."); + } + */ +} + static const JNINativeMethod gMethods[] = { { "nativeSecurityInit", "()V", (void *) com_android_internal_os_Zygote_nativeSecurityInit }, @@ -1308,7 +1332,9 @@ static const JNINativeMethod gMethods[] = { { "nativeUnmountStorageOnInit", "()V", (void *) com_android_internal_os_Zygote_nativeUnmountStorageOnInit }, { "nativePreApplicationInit", "()V", - (void *) com_android_internal_os_Zygote_nativePreApplicationInit } + (void *) com_android_internal_os_Zygote_nativePreApplicationInit }, + { "nativeInstallSeccompUidGidFilter", "(II)V", + (void *) com_android_internal_os_Zygote_nativeInstallSeccompUidGidFilter } }; int register_com_android_internal_os_Zygote(JNIEnv* env) { diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java index 5bc88450c3765..c1be3872e0319 100644 --- a/services/core/java/com/android/server/am/ProcessList.java +++ b/services/core/java/com/android/server/am/ProcessList.java @@ -1678,12 +1678,14 @@ public final class ProcessList { AppZygote appZygote = mAppZygotes.get(app.info.processName, app.info.uid); final ArrayList zygoteProcessList; if (appZygote == null) { - final int userId = UserHandle.getUserId(app.info.uid); final IsolatedUidRange uidRange = mAppIsolatedUidRangeAllocator.getIsolatedUidRangeLocked(app.info); - // Allocate an isolated UID out of this range for the Zygote itself - final int zygoteIsolatedUid = uidRange.allocateIsolatedUidLocked(userId); - appZygote = new AppZygote(app.info, zygoteIsolatedUid); + final int userId = UserHandle.getUserId(app.info.uid); + // Create the app-zygote and provide it with the UID-range it's allowed + // to setresuid/setresgid to. + final int firstUid = UserHandle.getUid(userId, uidRange.mFirstUid); + final int lastUid = UserHandle.getUid(userId, uidRange.mLastUid); + appZygote = new AppZygote(app.info, app.info.uid, firstUid, lastUid); mAppZygotes.put(app.info.processName, app.info.uid, appZygote); zygoteProcessList = new ArrayList(); mAppZygoteProcesses.put(appZygote, zygoteProcessList);