From ec3705f2ed187863efc34af5415495e1ee7775d2 Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Wed, 3 Jun 2020 23:29:42 -0700 Subject: [PATCH] Redesign of MagiskSU's sepolicy model Introduce new domain `magisk_client` and new file type `magisk_exec`. Connection to magiskd's always-on socket is restricted to magisk_client only. Whitelisted process domains can transit to magisk_client through executing files labelled magisk_exec. The main magisk binary shall be the only file labelled as magisk_exec throughout the whole system. All processes thus are no longer allowed to connect to magiskd directly without going through the proper magisk binary. Connection failures are silenced from audit logs with dontaudit rules, so crazy processes which traverse through all unix domain sockets to try connection can no longer check logcat to know the actual reason behind EACCES, leaking the denied process policy (which is u:r:magisk:s0). This also allows us to remove many rules that open up holes in untrusted_app domains that were used to make remote shell work properly. Since all processes establishing the remote shell are now restricted to the magisk_client domain, all these rules are moved to magisk_client. This makes Magisk require fewer compromises in Android's security model. Note: as of this commit, requesting new root access via Magisk Manager will stop working as Magisk Manager can no longer communicate with magiskd directly. This will be addressed in a future commit that involves changes in both native and application side. --- native/jni/core/restorecon.cpp | 9 +- native/jni/core/scripting.cpp | 2 +- native/jni/magiskpolicy/rules.cpp | 120 ++++++++++++++++----------- native/jni/magiskpolicy/sepolicy.hpp | 1 - native/jni/su/su_daemon.cpp | 6 +- native/jni/utils/include/selinux.hpp | 8 +- 6 files changed, 88 insertions(+), 58 deletions(-) diff --git a/native/jni/core/restorecon.cpp b/native/jni/core/restorecon.cpp index 66984fe14..502e79d03 100644 --- a/native/jni/core/restorecon.cpp +++ b/native/jni/core/restorecon.cpp @@ -10,7 +10,8 @@ using namespace std; #define SYSTEM_CON "u:object_r:system_file:s0" #define ADB_CON "u:object_r:adb_data_file:s0" #define ROOT_CON "u:object_r:rootfs:s0" -#define MAGISK_CON "u:object_r:" SEPOL_FILE_DOMAIN ":s0" +#define MAGISK_CON "u:object_r:" SEPOL_FILE_TYPE ":s0" +#define EXEC_CON "u:object_r:" SEPOL_EXEC_TYPE ":s0" static void restore_syscon(int dirfd) { struct dirent *entry; @@ -86,9 +87,9 @@ void restore_tmpcon() { int dfd = dirfd(dir.get()); for (dirent *entry; (entry = xreaddir(dir.get()));) { - if (entry->d_name == "magisk"sv || entry->d_name == "magiskinit"sv) - setfilecon_at(dfd, entry->d_name, MAGISK_CON); + if (entry->d_name == "magisk"sv) + setfilecon_at(dfd, entry->d_name, EXEC_CON); else - setfilecon_at(dfd, entry->d_name, ROOT_CON); + setfilecon_at(dfd, entry->d_name, SYSTEM_CON); } } diff --git a/native/jni/core/scripting.cpp b/native/jni/core/scripting.cpp index 7afdea62c..27ef16d6c 100644 --- a/native/jni/core/scripting.cpp +++ b/native/jni/core/scripting.cpp @@ -83,7 +83,7 @@ rm -f $APK )EOF"; void install_apk(const char *apk) { - setfilecon(apk, "u:object_r:" SEPOL_FILE_DOMAIN ":s0"); + setfilecon(apk, "u:object_r:" SEPOL_FILE_TYPE ":s0"); exec_t exec { .pre_exec = set_standalone, .fork = fork_no_zombie diff --git a/native/jni/magiskpolicy/rules.cpp b/native/jni/magiskpolicy/rules.cpp index e7d6b9bf6..e31f0df47 100644 --- a/native/jni/magiskpolicy/rules.cpp +++ b/native/jni/magiskpolicy/rules.cpp @@ -1,33 +1,8 @@ -#include - #include #include #include "sepolicy.hpp" -void sepol_impl::allow_su_client(const char *type) { - if (!exists(type)) - return; - allow(type, SEPOL_PROC_DOMAIN, "unix_stream_socket", "connectto"); - allow(type, SEPOL_PROC_DOMAIN, "unix_stream_socket", "getopt"); - - // Allow binder service - allow(type, SEPOL_PROC_DOMAIN, "binder", "call"); - allow(type, SEPOL_PROC_DOMAIN, "binder", "transfer"); - - // Allow termios ioctl - allow(type, "devpts", "chr_file", "ioctl"); - allow(type, "untrusted_app_devpts", "chr_file", "ioctl"); - allow(type, "untrusted_app_25_devpts", "chr_file", "ioctl"); - allow(type, "untrusted_app_all_devpts", "chr_file", "ioctl"); - if (db->policyvers >= POLICYDB_VERSION_XPERMS_IOCTL) { - allowxperm(type, "devpts", "chr_file", "0x5400-0x54FF"); - allowxperm(type, "untrusted_app_devpts", "chr_file", "0x5400-0x54FF"); - allowxperm(type, "untrusted_app_25_devpts", "chr_file", "0x5400-0x54FF"); - allowxperm(type, "untrusted_app_all_devpts", "chr_file", "0x5400-0x54FF"); - } -} - void sepolicy::magisk_rules() { // Temp suppress warnings auto bak = log_cb.w; @@ -37,18 +12,86 @@ void sepolicy::magisk_rules() { deny(ALL, "kernel", "security", "load_policy"); type(SEPOL_PROC_DOMAIN, "domain"); - type(SEPOL_FILE_DOMAIN, "file_type"); - permissive(SEPOL_PROC_DOMAIN); + type(SEPOL_CLIENT_DOMAIN, "domain"); + type(SEPOL_FILE_TYPE, "file_type"); + type(SEPOL_EXEC_TYPE, "file_type"); + permissive(SEPOL_PROC_DOMAIN); /* Just in case something is missing */ typeattribute(SEPOL_PROC_DOMAIN, "mlstrustedsubject"); typeattribute(SEPOL_PROC_DOMAIN, "netdomain"); typeattribute(SEPOL_PROC_DOMAIN, "bluetoothdomain"); - typeattribute(SEPOL_FILE_DOMAIN, "mlstrustedobject"); + typeattribute(SEPOL_FILE_TYPE, "mlstrustedobject"); + + // Make our root domain unconstrained + allow(SEPOL_PROC_DOMAIN, ALL, ALL, ALL); + // Allow us to do any ioctl on all block devices + if (db->policyvers >= POLICYDB_VERSION_XPERMS_IOCTL) + allowxperm(SEPOL_PROC_DOMAIN, ALL, "blk_file", ALL); + + // Create unconstrained file type + allow(ALL, SEPOL_FILE_TYPE, "file", ALL); + allow(ALL, SEPOL_FILE_TYPE, "dir", ALL); + allow(ALL, SEPOL_FILE_TYPE, "fifo_file", ALL); + allow(ALL, SEPOL_FILE_TYPE, "chr_file", ALL); + + // Basic su client needs + allow(SEPOL_CLIENT_DOMAIN, ALL, "fd", "use"); + allow(SEPOL_CLIENT_DOMAIN, SEPOL_CLIENT_DOMAIN, ALL, ALL); + allow(SEPOL_CLIENT_DOMAIN, SEPOL_EXEC_TYPE, "file", ALL); + allow(SEPOL_CLIENT_DOMAIN, SEPOL_PROC_DOMAIN, "unix_stream_socket", "connectto"); + allow(SEPOL_CLIENT_DOMAIN, SEPOL_PROC_DOMAIN, "unix_stream_socket", "getopt"); + + // Allow su client to manipulate pts + const char *pts[] { + "devpts", "untrusted_app_devpts", "untrusted_app_25_devpts", "untrusted_app_all_devpts" }; + for (auto type : pts) { + allow(SEPOL_CLIENT_DOMAIN, type, "chr_file", "read"); + allow(SEPOL_CLIENT_DOMAIN, type, "chr_file", "write"); + allow(SEPOL_CLIENT_DOMAIN, type, "chr_file", "getattr"); + allow(SEPOL_CLIENT_DOMAIN, type, "chr_file", "ioctl"); + if (db->policyvers >= POLICYDB_VERSION_XPERMS_IOCTL) + allowxperm(SEPOL_CLIENT_DOMAIN, type, "chr_file", "0x5400-0x54FF"); + } + + // Allow these processes to access MagiskSU + const char *clients[] { + "init", "shell", "system_app", "priv_app", "platform_app", "untrusted_app", + "untrusted_app_25", "untrusted_app_27", "untrusted_app_29", "update_engine" }; + for (auto type : clients) { + if (!exists(type)) + continue; + // exec magisk + allow(type, SEPOL_EXEC_TYPE, "file", "read"); + allow(type, SEPOL_EXEC_TYPE, "file", "open"); + allow(type, SEPOL_EXEC_TYPE, "file", "getattr"); + allow(type, SEPOL_EXEC_TYPE, "file", "execute"); + + // Auto transit to client domain + type_transition(type, SEPOL_EXEC_TYPE, "process", SEPOL_CLIENT_DOMAIN); + allow(type, SEPOL_CLIENT_DOMAIN, "process", "transition"); + dontaudit(type, SEPOL_CLIENT_DOMAIN, "process", "siginh"); + dontaudit(type, SEPOL_CLIENT_DOMAIN, "process", "rlimitinh"); + dontaudit(type, SEPOL_CLIENT_DOMAIN, "process", "noatsecure"); + + allow(SEPOL_CLIENT_DOMAIN, type, "process", "sigchld"); + allow(SEPOL_CLIENT_DOMAIN, type, "fifo_file", "read"); + allow(SEPOL_CLIENT_DOMAIN, type, "fifo_file", "write"); + allow(SEPOL_CLIENT_DOMAIN, type, "fifo_file", "ioctl"); + } + + // Allow system_server to manage magisk_client + allow("system_server", SEPOL_CLIENT_DOMAIN, "process", "getpgid"); + allow("system_server", SEPOL_CLIENT_DOMAIN, "process", "sigkill"); + + // Don't allow pesky processes to monitor audit deny logs when poking magisk daemon sockets + dontaudit(ALL, SEPOL_PROC_DOMAIN, "unix_stream_socket", ALL); // Let everyone access tmpfs files (for SAR sbin overlay) allow(ALL, "tmpfs", "file", ALL); - // For normal rootfs file/directory operations when rw (for SAR / overlay) + // For relabelling files allow("rootfs", "labeledfs", "filesystem", "associate"); + allow(SEPOL_FILE_TYPE, "pipefs", "filesystem", "associate"); + allow(SEPOL_FILE_TYPE, "devpts", "filesystem", "associate"); // Let init transit to SEPOL_PROC_DOMAIN allow("kernel", "kernel", "process", "setcurrent"); @@ -60,25 +103,6 @@ void sepolicy::magisk_rules() { allow("init", "tmpfs", "file", "getattr"); allow("init", "tmpfs", "file", "execute"); - // Make our domain unconstrained - allow(SEPOL_PROC_DOMAIN, ALL, ALL, ALL); - // Allow us to do any ioctl on all block devices - if (db->policyvers >= POLICYDB_VERSION_XPERMS_IOCTL) - allowxperm(SEPOL_PROC_DOMAIN, ALL, "blk_file", ALL); - - // Make our file type unconstrained - allow(ALL, SEPOL_FILE_DOMAIN, "file", ALL); - allow(ALL, SEPOL_FILE_DOMAIN, "dir", ALL); - allow(ALL, SEPOL_FILE_DOMAIN, "fifo_file", ALL); - allow(ALL, SEPOL_FILE_DOMAIN, "chr_file", ALL); - - // Allow these processes to access MagiskSU - std::initializer_list clients { - "init", "shell", "system_app", "priv_app", "platform_app", "untrusted_app", - "untrusted_app_25", "untrusted_app_27", "untrusted_app_29", "update_engine" }; - for (auto type : clients) - impl->allow_su_client(type); - // suRights allow("servicemanager", SEPOL_PROC_DOMAIN, "dir", "search"); allow("servicemanager", SEPOL_PROC_DOMAIN, "dir", "read"); diff --git a/native/jni/magiskpolicy/sepolicy.hpp b/native/jni/magiskpolicy/sepolicy.hpp index e4feb4ce4..bf8b35887 100644 --- a/native/jni/magiskpolicy/sepolicy.hpp +++ b/native/jni/magiskpolicy/sepolicy.hpp @@ -20,7 +20,6 @@ struct sepol_impl : public sepolicy { void add_typeattribute(type_datum_t *type, type_datum_t *attr); bool add_typeattribute(const char *type, const char *attr); void strip_dontaudit(); - void allow_su_client(const char *type); }; #define impl static_cast(this) diff --git a/native/jni/su/su_daemon.cpp b/native/jni/su/su_daemon.cpp index 31ae9cd3b..6361e937c 100644 --- a/native/jni/su/su_daemon.cpp +++ b/native/jni/su/su_daemon.cpp @@ -275,9 +275,9 @@ void su_daemon_handler(int client, struct ucred *credential) { xdup2(errfd, STDERR_FILENO); // Unleash all streams from SELinux hell - setfilecon("/proc/self/fd/0", "u:object_r:" SEPOL_FILE_DOMAIN ":s0"); - setfilecon("/proc/self/fd/1", "u:object_r:" SEPOL_FILE_DOMAIN ":s0"); - setfilecon("/proc/self/fd/2", "u:object_r:" SEPOL_FILE_DOMAIN ":s0"); + setfilecon("/proc/self/fd/0", "u:object_r:" SEPOL_FILE_TYPE ":s0"); + setfilecon("/proc/self/fd/1", "u:object_r:" SEPOL_FILE_TYPE ":s0"); + setfilecon("/proc/self/fd/2", "u:object_r:" SEPOL_FILE_TYPE ":s0"); close(infd); close(outfd); diff --git a/native/jni/utils/include/selinux.hpp b/native/jni/utils/include/selinux.hpp index 2b660142a..1ea472538 100644 --- a/native/jni/utils/include/selinux.hpp +++ b/native/jni/utils/include/selinux.hpp @@ -16,8 +16,14 @@ #define SYSEXT_POLICY_DIR "/system_ext/etc/selinux/" #define SPLIT_PLAT_CIL PLAT_POLICY_DIR "plat_sepolicy.cil" +// Unconstrained domain the daemon and root processes run in #define SEPOL_PROC_DOMAIN "magisk" -#define SEPOL_FILE_DOMAIN "magisk_file" +// Highly constrained domain, sole purpose is to connect to daemon +#define SEPOL_CLIENT_DOMAIN "magisk_client" +// Unconstrained file type that anyone can access +#define SEPOL_FILE_TYPE "magisk_file" +// Special file type to allow clients to transit to client domain automatically +#define SEPOL_EXEC_TYPE "magisk_exec" extern void (*freecon)(char *con); extern int (*setcon)(const char *con);