Rearrange su daemon routine

native/jni/su/su.cpp

@@ -94,15 +94,6 @@ static void setup_sighandlers(void (*handler)(int)) {
 	}
 }
 
-// Default values
-su_req_base::su_req_base()
-		: uid(UID_ROOT), login(false), keepenv(false), mount_master(false) {}
-su_request::su_request()
-		: shell(DEFAULT_SHELL), command("") {}
-
-/*
- * Connect daemon, send argc, argv, cwd, pts slave
- */
 int su_client_main(int argc, char *argv[]) {
 	int c;
 	struct option long_opts[] = {
@@ -189,18 +180,18 @@ int su_client_main(int argc, char *argv[]) {
 	// Tell the daemon we are su
 	write_int(fd, SUPERUSER);
 
-	// Wait for ack from daemon
-	if (read_int(fd)) {
-		// Fast fail
-		fprintf(stderr, "%s\n", strerror(EACCES));
-		return DENY;
-	}
-
 	// Send su_request
 	xwrite(fd, &su_req, sizeof(su_req_base));
 	write_string(fd, su_req.shell);
 	write_string(fd, su_req.command);
 
+	// Wait for ack from daemon
+	if (read_int(fd)) {
+		// Fast fail
+		fprintf(stderr, "%s\n", strerror(EACCES));
+		return EACCES;
+	}
+
 	// Determine which one of our streams are attached to a TTY
 	int atty = 0;
 	if (isatty(STDIN_FILENO))  atty |= ATTY_IN;

native/jni/su/su.h

@@ -2,7 +2,6 @@
 
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <limits.h>
 #include <memory>
 
 #include <db.h>
@@ -40,18 +39,25 @@ private:
 };
 
 struct su_req_base {
-	unsigned uid;
+	unsigned uid = UID_ROOT;
-	bool login;
+	bool login = false;
-	bool keepenv;
+	bool keepenv = false;
-	bool mount_master;
+	bool mount_master = false;
-protected:
-	su_req_base();
 } __attribute__((packed));
 
 struct su_request : public su_req_base {
-	const char *shell;
+	const char *shell = DEFAULT_SHELL;
-	const char *command;
+	const char *command = "";
-	su_request();
+	su_request(bool dyn = false) : dyn(dyn) {}
+	~su_request() {
+		if (dyn) {
+			free(const_cast<char*>(shell));
+			free(const_cast<char*>(command));
+		}
+	}
+
+private:
+	bool dyn;
 } __attribute__((packed));
 
 struct su_context {
@@ -60,8 +66,6 @@ struct su_context {
 	pid_t pid;
 };
 
-// connect.c
-
 void app_log(const su_context &ctx);
 void app_notify(const su_context &ctx);
 void app_connect(const char *socket, const std::shared_ptr<su_info> &info);

native/jni/su/su_daemon.cpp

@@ -101,27 +101,38 @@ static shared_ptr<su_info> get_su_info(unsigned uid) {
 
 	// Lock before the policy is determined
 	info->lock();
+	RunFinally unlock([&] {
+		info->unlock();
+	});
 
 	if (info->access.policy == QUERY) {
 		// Not cached, get data from database
 		database_check(info);
 
+		// If it's root or the manager, allow it silently
+		if (info->uid == UID_ROOT || (info->uid % 100000) == (info->mgr_st.st_uid % 100000)) {
+			info->access = SILENT_SU_ACCESS;
+			return info;
+		}
+
 		// Check su access settings
 		switch (info->cfg[ROOT_ACCESS]) {
 			case ROOT_ACCESS_DISABLED:
 				LOGW("Root access is disabled!\n");
 				info->access = NO_SU_ACCESS;
-				break;
+				return info;
 			case ROOT_ACCESS_ADB_ONLY:
 				if (info->uid != UID_SHELL) {
 					LOGW("Root access limited to ADB only!\n");
 					info->access = NO_SU_ACCESS;
+					return info;
 				}
 				break;
 			case ROOT_ACCESS_APPS_ONLY:
 				if (info->uid == UID_SHELL) {
 					LOGW("Root access is disabled for ADB!\n");
 					info->access = NO_SU_ACCESS;
+					return info;
 				}
 				break;
 			case ROOT_ACCESS_APPS_AND_ADB:
@@ -129,22 +140,17 @@ static shared_ptr<su_info> get_su_info(unsigned uid) {
 				break;
 		}
 
-		// If it's the manager, allow it silently
+		if (info->access.policy != QUERY)
-		if ((info->uid % 100000) == (info->mgr_st.st_uid % 100000))
+			return info;
-			info->access = SILENT_SU_ACCESS;
-
-		// Allow if it's root
-		if (info->uid == UID_ROOT)
-			info->access = SILENT_SU_ACCESS;
 
 		// If still not determined, check if manager exists
-		if (info->access.policy == QUERY && info->str[SU_MANAGER][0] == '\0')
+		if (info->str[SU_MANAGER][0] == '\0') {
 			info->access = NO_SU_ACCESS;
+			return info;
+		}
 	}
 
 	// If still not determined, ask manager
-	if (info->access.policy == QUERY) {
-		// Create random socket
 	struct sockaddr_un addr;
 	int sockfd = create_rand_socket(&addr);
 
@@ -160,10 +166,6 @@ static shared_ptr<su_info> get_su_info(unsigned uid) {
 		close(fd);
 	}
 	close(sockfd);
-	}
-
-	// Unlock
-	info->unlock();
 
 	return info;
 }
@@ -187,24 +189,31 @@ static void set_identity(unsigned uid) {
 void su_daemon_handler(int client, struct ucred *credential) {
 	LOGD("su: request from pid=[%d], client=[%d]\n", credential->pid, client);
 
-	auto info = get_su_info(credential->uid);
+	su_context ctx = {
+		.info = get_su_info(credential->uid),
+		.req = su_request(true),
+		.pid = credential->pid
+	};
+
+	// Read su_request
+	xxread(client, &ctx.req, sizeof(su_req_base));
+	ctx.req.shell = read_string(client);
+	ctx.req.command = read_string(client);
+
+	if (ctx.info->access.log)
+		app_log(ctx);
+	else if (ctx.info->access.notify)
+		app_notify(ctx);
 
 	// Fail fast
-	if (info->access.policy == DENY && info->str[SU_MANAGER][0] == '\0') {
+	if (ctx.info->access.policy == DENY) {
-		LOGD("su: fast deny\n");
+		LOGW("su: request rejected (%u)", ctx.info->uid);
+		ctx.info.reset();
 		write_int(client, DENY);
 		close(client);
 		return;
-	}
+	} else if (int child = xfork(); child) {
-
+		ctx.info.reset();
-	/* Fork a new process, 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
-	 */
-	int child = xfork();
-	if (child) {
-		info.reset();
 
 		// Wait result
 		LOGD("su: waiting child pid=[%d]\n", child);
@@ -221,27 +230,23 @@ 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
 	log_cb.ex = exit;
 
-	su_context ctx = {
-		.info = info,
-		.pid = credential->pid
-	};
-
 	// ack
 	write_int(client, 0);
 
 	// Become session leader
 	xsetsid();
 
-	// Read su_request
-	xxread(client, &ctx.req, sizeof(su_req_base));
-	ctx.req.shell = read_string(client);
-	ctx.req.command = read_string(client);
-
 	// Get pts_slave
 	char *pts_slave = read_string(client);
 
@@ -257,7 +262,7 @@ void su_daemon_handler(int client, struct ucred *credential) {
 		xstat(pts_slave, &st);
 
 		// If caller is not root, ensure the owner of pts_slave is the caller
-		if(st.st_uid != info->uid && info->uid != 0)
+		if(st.st_uid != ctx.info->uid && ctx.info->uid != 0)
 			LOGE("su: Wrong permission of pts_slave");
 
 		// Opening the TTY has to occur after the
@@ -292,8 +297,8 @@ void su_daemon_handler(int client, struct ucred *credential) {
 
 	// Handle namespaces
 	if (ctx.req.mount_master)
-		info->cfg[SU_MNT_NS] = NAMESPACE_MODE_GLOBAL;
+		ctx.info->cfg[SU_MNT_NS] = NAMESPACE_MODE_GLOBAL;
-	switch (info->cfg[SU_MNT_NS]) {
+	switch (ctx.info->cfg[SU_MNT_NS]) {
 		case NAMESPACE_MODE_GLOBAL:
 			LOGD("su: use global namespace\n");
 			break;
@@ -309,12 +314,6 @@ void su_daemon_handler(int client, struct ucred *credential) {
 			break;
 	}
 
-	if (info->access.log)
-		app_log(ctx);
-	else if (info->access.notify)
-		app_notify(ctx);
-
-	if (info->access.policy == ALLOW) {
 	const char *argv[] = { nullptr, nullptr, nullptr, nullptr };
 
 	argv[0] = ctx.req.login ? "-" : ctx.req.shell;
@@ -358,9 +357,4 @@ void su_daemon_handler(int client, struct ucred *credential) {
 	fprintf(stderr, "Cannot execute %s: %s\n", ctx.req.shell, strerror(errno));
 	PLOGE("exec");
 	exit(EXIT_FAILURE);
-	} else {
-		LOGW("su: request rejected (%u->%u)", info->uid, ctx.req.uid);
-		fprintf(stderr, "%s\n", strerror(EACCES));
-		exit(EXIT_FAILURE);
-	}
 }