From 434efec8607103d4b85390d72b2b56d5b523729e Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Thu, 10 Sep 2020 00:38:29 -0700 Subject: [PATCH] Use FIFO for su request communication Fix #3159 --- .../magisk/core/su/SuRequestHandler.kt | 59 ++++++++----------- native/jni/core/socket.cpp | 2 +- native/jni/include/socket.hpp | 1 - native/jni/su/connect.cpp | 48 +++++++-------- native/jni/su/su.hpp | 3 +- native/jni/su/su_daemon.cpp | 31 +++++----- 6 files changed, 66 insertions(+), 78 deletions(-) diff --git a/app/src/main/java/com/topjohnwu/magisk/core/su/SuRequestHandler.kt b/app/src/main/java/com/topjohnwu/magisk/core/su/SuRequestHandler.kt index 49193efda..aefbe2895 100644 --- a/app/src/main/java/com/topjohnwu/magisk/core/su/SuRequestHandler.kt +++ b/app/src/main/java/com/topjohnwu/magisk/core/su/SuRequestHandler.kt @@ -2,7 +2,6 @@ package com.topjohnwu.magisk.core.su import android.content.Intent import android.content.pm.PackageManager -import android.net.LocalServerSocket import android.net.LocalSocket import android.net.LocalSocketAddress import androidx.collection.ArrayMap @@ -22,20 +21,16 @@ import java.util.concurrent.TimeUnit.SECONDS abstract class SuRequestHandler( private val packageManager: PackageManager, private val policyDB: PolicyDao -) { - private lateinit var socket: LocalSocket - private lateinit var output: DataOutputStream - private lateinit var input: DataInputStream +) : Closeable { + private lateinit var output: DataOutputStream protected lateinit var policy: SuPolicy private set abstract fun onStart() suspend fun start(intent: Intent): Boolean { - val name = intent.getStringExtra("socket") ?: return false - - if (!init(name)) + if (!init(intent)) return false // Never allow com.topjohnwu.magisk (could be malware) @@ -63,35 +58,37 @@ abstract class SuRequestHandler( } } - private class SocketError : IOException() + @Throws(IOException::class) + override fun close() { + if (::output.isInitialized) + output.close() + } - private suspend fun init(name: String) = withContext(Dispatchers.IO) { + private class SuRequestError : IOException() + + private suspend fun init(intent: Intent) = withContext(Dispatchers.IO) { try { + val uid: Int if (Const.Version.atLeastCanary()) { - val server = LocalServerSocket(name) - // Do NOT use Closable?.use(block) here as LocalServerSocket does - // not implement Closable on older Android platforms - try { - socket = async { server.accept() }.timedAwait() ?: throw SocketError() - } finally { - server.close() - } + val name = intent.getStringExtra("fifo") ?: throw SuRequestError() + uid = intent.getIntExtra("uid", -1).also { if (it < 0) throw SuRequestError() } + output = DataOutputStream(FileOutputStream(name).buffered()) } else { - socket = LocalSocket() + val name = intent.getStringExtra("socket") ?: throw SuRequestError() + val socket = LocalSocket() socket.connect(LocalSocketAddress(name, LocalSocketAddress.Namespace.ABSTRACT)) + output = DataOutputStream(BufferedOutputStream(socket.outputStream)) + val input = DataInputStream(BufferedInputStream(socket.inputStream)) + val map = async { input.readRequest() }.timedAwait() ?: throw SuRequestError() + uid = map["uid"]?.toIntOrNull() ?: throw SuRequestError() } - output = DataOutputStream(BufferedOutputStream(socket.outputStream)) - input = DataInputStream(BufferedInputStream(socket.inputStream)) - val map = async { readRequest() }.timedAwait() ?: throw SocketError() - val uid = map["uid"]?.toIntOrNull() ?: throw SocketError() policy = uid.toPolicy(packageManager) true } catch (e: Exception) { when (e) { is IOException, is PackageManager.NameNotFoundException -> { Timber.e(e) - if (::socket.isInitialized) - socket.close() + runCatching { close() } false } else -> throw e // Unexpected error @@ -116,11 +113,7 @@ abstract class SuRequestHandler( } catch (e: IOException) { Timber.e(e) } finally { - runCatching { - input.close() - output.close() - socket.close() - } + runCatching { close() } if (until >= 0) policyDB.update(policy) } @@ -128,11 +121,11 @@ abstract class SuRequestHandler( } @Throws(IOException::class) - private fun readRequest(): Map { + private fun DataInputStream.readRequest(): Map { fun readString(): String { - val len = input.readInt() + val len = readInt() val buf = ByteArray(len) - input.readFully(buf) + readFully(buf) return String(buf, Charsets.UTF_8) } val ret = ArrayMap() diff --git a/native/jni/core/socket.cpp b/native/jni/core/socket.cpp index 4265a7fe7..8ed5bf562 100644 --- a/native/jni/core/socket.cpp +++ b/native/jni/core/socket.cpp @@ -15,7 +15,7 @@ static size_t socket_len(sockaddr_un *sun) { socklen_t setup_sockaddr(sockaddr_un *sun, const char *name) { memset(sun, 0, sizeof(*sun)); - sun->sun_family = AF_LOCAL; + sun->sun_family = AF_UNIX; strcpy(sun->sun_path + 1, name); return socket_len(sun); } diff --git a/native/jni/include/socket.hpp b/native/jni/include/socket.hpp index eb4d6a69d..cbecf3732 100644 --- a/native/jni/include/socket.hpp +++ b/native/jni/include/socket.hpp @@ -4,7 +4,6 @@ #include socklen_t setup_sockaddr(sockaddr_un *sun, const char *name); -int create_app_socket(sockaddr_un *sun); int socket_accept(int sockfd, int timeout); void get_client_cred(int fd, struct ucred *cred); int recv_fd(int sockfd); diff --git a/native/jni/su/connect.cpp b/native/jni/su/connect.cpp index 8e26607d2..5e5b4908c 100644 --- a/native/jni/su/connect.cpp +++ b/native/jni/su/connect.cpp @@ -3,6 +3,7 @@ #include #include +#include #include "su.hpp" @@ -179,34 +180,33 @@ void app_notify(const su_context &ctx) { } } -int app_socket(const char *name, const shared_ptr &info) { - vector extras; - extras.reserve(1); - extras.emplace_back("socket", name); +int app_request(const shared_ptr &info) { + // Create FIFO + char fifo[64]; + strcpy(fifo, "/dev/socket/"); + gen_rand_str(fifo + 12, 32, true); + mkfifo(fifo, 0600); + chown(fifo, info->mgr_st.st_uid, info->mgr_st.st_gid); + setfilecon(fifo, "u:object_r:" SEPOL_FILE_TYPE ":s0"); + // Send request + vector extras; + extras.reserve(2); + extras.emplace_back("fifo", fifo); + extras.emplace_back("uid", info->uid); exec_cmd("request", extras, info, PKG_ACTIVITY); - sockaddr_un addr; - size_t len = setup_sockaddr(&addr, name); - int fd = xsocket(AF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0); - bool connected = false; - // Try at most 60 seconds - for (int i = 0; i < 600; ++i) { - if (connect(fd, reinterpret_cast(&addr), len) == 0) { - connected = true; - break; - } - usleep(100000); // 100ms - } - if (connected) { - return fd; - } else { + // Wait for data input for at most 70 seconds + int fd = xopen(fifo, O_RDONLY | O_CLOEXEC); + struct pollfd pfd = { + .fd = fd, + .events = POLL_IN + }; + if (xpoll(&pfd, 1, 70 * 1000) <= 0) { close(fd); - return -1; + fd = -1; } -} -void socket_send_request(int fd, const shared_ptr &info) { - write_key_token(fd, "uid", info->uid); - write_string_be(fd, "eof"); + unlink(fifo); + return fd; } diff --git a/native/jni/su/su.hpp b/native/jni/su/su.hpp index 470fcd611..7220dd020 100644 --- a/native/jni/su/su.hpp +++ b/native/jni/su/su.hpp @@ -68,5 +68,4 @@ struct su_context { void app_log(const su_context &ctx); void app_notify(const su_context &ctx); -int app_socket(const char *name, const std::shared_ptr &info); -void socket_send_request(int fd, const std::shared_ptr &info); +int app_request(const std::shared_ptr &info); diff --git a/native/jni/su/su_daemon.cpp b/native/jni/su/su_daemon.cpp index aad4565c5..9a608aab1 100644 --- a/native/jni/su/su_daemon.cpp +++ b/native/jni/su/su_daemon.cpp @@ -91,7 +91,7 @@ static shared_ptr get_su_info(unsigned uid) { info = cached; } - auto g = info->lock(); + mutex_guard lock = info->lock(); if (info->access.policy == QUERY) { // Not cached, get data from database @@ -130,7 +130,7 @@ static shared_ptr get_su_info(unsigned uid) { return info; // If still not determined, check if manager exists - if (info->str[SU_MANAGER][0] == '\0') { + if (info->str[SU_MANAGER].empty()) { info->access = NO_SU_ACCESS; return info; } @@ -139,13 +139,10 @@ static shared_ptr get_su_info(unsigned uid) { } // If still not determined, ask manager - char socket_name[32]; - gen_rand_str(socket_name, sizeof(socket_name)); - int fd = app_socket(socket_name, info); + int fd = app_request(info); if (fd < 0) { info->access.policy = DENY; } else { - socket_send_request(fd, info); int ret = read_int_be(fd); info->access.policy = ret < 0 ? DENY : static_cast(ret); close(fd); @@ -154,11 +151,8 @@ static shared_ptr get_su_info(unsigned uid) { return info; } +// Set effective uid back to root, otherwise setres[ug]id will fail if uid isn't root static void set_identity(unsigned uid) { - /* - * Set effective uid back to root, otherwise setres[ug]id will fail - * if uid isn't root. - */ if (seteuid(0)) { PLOGE("seteuid (root)"); } @@ -196,7 +190,16 @@ void su_daemon_handler(int client, struct ucred *credential) { write_int(client, DENY); close(client); return; - } else if (int child = xfork(); child) { + } + + // Fork a child root process + // + // The child process will need to setsid, open a pseudo-terminal + // if needed, and eventually exec shell. + // The parent process will wait for the result and + // send the return code back to our client. + + if (int child = xfork(); child) { ctx.info.reset(); // Wait result @@ -214,12 +217,6 @@ void su_daemon_handler(int client, struct ucred *credential) { return; } - /* The child process will need to setsid, open a pseudo-terminal - * if needed, and will eventually run exec. - * The parent process will wait for the result and - * send the return code back to our client - */ - LOGD("su: fork handler\n"); // Abort upon any error occurred