Use FIFO for su request communication

Fix #3159

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
-) {
+) : Closeable {
-    private lateinit var socket: LocalSocket
-    private lateinit var output: DataOutputStream
-    private lateinit var input: DataInputStream
 
+    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(intent))
-
-        if (!init(name))
             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)
+                val name = intent.getStringExtra("fifo") ?: throw SuRequestError()
-                // Do NOT use Closable?.use(block) here as LocalServerSocket does
+                uid = intent.getIntExtra("uid", -1).also { if (it < 0) throw SuRequestError() }
-                // not implement Closable on older Android platforms
+                output = DataOutputStream(FileOutputStream(name).buffered())
-                try {
-                    socket = async { server.accept() }.timedAwait() ?: throw SocketError()
-                } finally {
-                    server.close()
-                }
             } 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)
+                    runCatching { close() }
-                        socket.close()
                     false
                 }
                 else -> throw e  // Unexpected error
@@ -116,11 +113,7 @@ abstract class SuRequestHandler(
             } catch (e: IOException) {
                 Timber.e(e)
             } finally {
-                runCatching {
+                runCatching { close() }
-                    input.close()
-                    output.close()
-                    socket.close()
-                }
                 if (until >= 0)
                     policyDB.update(policy)
             }
@@ -128,11 +121,11 @@ abstract class SuRequestHandler(
     }
 
     @Throws(IOException::class)
-    private fun readRequest(): Map<String, String> {
+    private fun DataInputStream.readRequest(): Map<String, String> {
         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<String, String>()

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);
 }

native/jni/include/socket.hpp

@@ -4,7 +4,6 @@
 #include <sys/socket.h>
 
 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);

native/jni/su/connect.cpp

@@ -3,6 +3,7 @@
 
 #include <daemon.hpp>
 #include <utils.hpp>
+#include <selinux.hpp>
 
 #include "su.hpp"
 
@@ -179,34 +180,33 @@ void app_notify(const su_context &ctx) {
 	}
 }
 
-int app_socket(const char *name, const shared_ptr<su_info> &info) {
+int app_request(const shared_ptr<su_info> &info) {
-	vector<Extra> extras;
+	// Create FIFO
-	extras.reserve(1);
+	char fifo[64];
-	extras.emplace_back("socket", name);
+	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<Extra> 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;
+	// Wait for data input for at most 70 seconds
-	size_t len = setup_sockaddr(&addr, name);
+	int fd = xopen(fifo, O_RDONLY | O_CLOEXEC);
-	int fd = xsocket(AF_LOCAL, SOCK_STREAM | SOCK_CLOEXEC, 0);
+	struct pollfd pfd = {
-	bool connected = false;
+		.fd = fd,
-	// Try at most 60 seconds
+		.events = POLL_IN
-	for (int i = 0; i < 600; ++i) {
+	};
-		if (connect(fd, reinterpret_cast<sockaddr *>(&addr), len) == 0) {
+	if (xpoll(&pfd, 1, 70 * 1000) <= 0) {
-			connected = true;
-			break;
-		}
-		usleep(100000);  // 100ms
-	}
-	if (connected) {
-		return fd;
-	} else {
 		close(fd);
-		return -1;
+		fd = -1;
 	}
-}
 
-void socket_send_request(int fd, const shared_ptr<su_info> &info) {
+	unlink(fifo);
-	write_key_token(fd, "uid", info->uid);
+	return fd;
-	write_string_be(fd, "eof");
 }

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<su_info> &info);
+int app_request(const std::shared_ptr<su_info> &info);
-void socket_send_request(int fd, const std::shared_ptr<su_info> &info);

native/jni/su/su_daemon.cpp

@@ -91,7 +91,7 @@ static shared_ptr<su_info> 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<su_info> 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<su_info> get_su_info(unsigned uid) {
 	}
 
 	// If still not determined, ask manager
-	char socket_name[32];
+	int fd = app_request(info);
-	gen_rand_str(socket_name, sizeof(socket_name));
-	int fd = app_socket(socket_name, 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<policy_t>(ret);
 		close(fd);
@@ -154,11 +151,8 @@ static shared_ptr<su_info> 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