From a9a303f79cf7ef2027d09578bc607e7eafb4b37a Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Wed, 14 Dec 2011 20:13:39 +0000 Subject: [PATCH 1/7] bump version --- su.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/su.h b/su.h index bd5b06a..6bf7259 100644 --- a/su.h +++ b/su.h @@ -32,8 +32,8 @@ #define SOCKET_PATH_TEMPLATE REQUESTOR_CACHE_PATH "/.socketXXXXXX" -#define VERSION "3.0.1" -#define VERSION_CODE 12 +#define VERSION "3.0.3" +#define VERSION_CODE 14 #define DATABASE_VERSION 6 From 0123ee3421ee5ac95b0af1e3af4d22ef0c46914b Mon Sep 17 00:00:00 2001 From: "Gleb O. Raiko" Date: Sun, 23 Oct 2011 17:18:52 +0400 Subject: [PATCH 2/7] Report error from read rather than successful completion of close --- su.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/su.c b/su.c index bb87a19..4094af0 100644 --- a/su.c +++ b/su.c @@ -71,6 +71,7 @@ static int from_init(struct su_initiator *from) int fd; ssize_t len; int i; + int err; from->uid = getuid(); from->pid = getppid(); @@ -83,9 +84,10 @@ static int from_init(struct su_initiator *from) return -1; } len = read(fd, args, sizeof(args)); + err = errno; close(fd); if (len < 0 || len == sizeof(args)) { - PLOGE("Reading command line"); + PLOGEV("Reading command line", err); return -1; } From 2e17b69cef62fead9770e1c6ff06d5f789e3fca9 Mon Sep 17 00:00:00 2001 From: "Gleb O. Raiko" Date: Sun, 23 Oct 2011 17:31:27 +0400 Subject: [PATCH 3/7] Remove setgroups in allow() supplementary groups have been dropped already at this time (introduced by commit 00f1bb547ef219b690a440920c7bc29be2a1d348) --- su.c | 1 - 1 file changed, 1 deletion(-) diff --git a/su.c b/su.c index 4094af0..8e7f488 100644 --- a/su.c +++ b/su.c @@ -289,7 +289,6 @@ static void allow(char *shell) strcpy(shell , "/system/bin/sh"); } exe = strrchr (shell, '/') + 1; - setgroups(0, NULL); setresgid(to->uid, to->uid, to->uid); setresuid(to->uid, to->uid, to->uid); LOGD("%u %s executing %u %s using shell %s : %s", from->uid, from->bin, From d5984c9ba86ea7a09389ebe6da81dad91fb7edca Mon Sep 17 00:00:00 2001 From: "Gleb O. Raiko" Date: Sun, 30 Oct 2011 00:32:54 +0400 Subject: [PATCH 4/7] Always free result table if sqlite3_get_table succeeds --- db.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db.c b/db.c index c338ee9..ca56f92 100644 --- a/db.c +++ b/db.c @@ -48,7 +48,7 @@ int database_check(sqlite3 *db, struct su_initiator *from, struct su_request *to char *zErrmsg; char **result; int nrow,ncol; - int allow; + int allow = DB_INTERACTIVE; struct timeval tv; sqlite3_snprintf( @@ -70,7 +70,7 @@ int database_check(sqlite3 *db, struct su_initiator *from, struct su_request *to } if (nrow == 0 || ncol != 3) - return DB_INTERACTIVE; + goto out; if (strcmp(result[0], "_id") == 0 && strcmp(result[2], "allow") == 0) { if (strcmp(result[5], "1") == 0) { @@ -80,10 +80,10 @@ int database_check(sqlite3 *db, struct su_initiator *from, struct su_request *to } else { allow = DB_DENY; } - return allow; } +out: sqlite3_free_table(result); - return DB_INTERACTIVE; + return allow; } From 4c6ea930caf9ac3609f3c19f11d768fadedd3f70 Mon Sep 17 00:00:00 2001 From: "Gleb O. Raiko" Date: Sun, 30 Oct 2011 14:43:23 +0400 Subject: [PATCH 5/7] Clean up sources --- db.c | 4 +--- su.c | 1 - su.h | 4 ---- 3 files changed, 1 insertion(+), 8 deletions(-) diff --git a/db.c b/db.c index ca56f92..ce4ae72 100644 --- a/db.c +++ b/db.c @@ -28,8 +28,7 @@ sqlite3 *database_init() { sqlite3 *db; - int version, rc, databaseStatus = 0; - char *zErrMsg = 0; + int rc; rc = sqlite3_open_v2(REQUESTOR_DATABASE_PATH, &db, SQLITE_OPEN_READONLY, NULL); if ( rc ) { @@ -49,7 +48,6 @@ int database_check(sqlite3 *db, struct su_initiator *from, struct su_request *to char **result; int nrow,ncol; int allow = DB_INTERACTIVE; - struct timeval tv; sqlite3_snprintf( sizeof(sql), sql, diff --git a/su.c b/su.c index 8e7f488..5d57480 100644 --- a/su.c +++ b/su.c @@ -224,7 +224,6 @@ static int socket_accept(int serv_fd) static int socket_receive_result(int serv_fd, char *result, ssize_t result_len) { ssize_t len; - char buf[64]; for (;;) { int fd = socket_accept(serv_fd); diff --git a/su.h b/su.h index 6bf7259..b8086be 100644 --- a/su.h +++ b/su.h @@ -24,10 +24,6 @@ #define REQUESTOR_DATABASES_PATH REQUESTOR_DATA_PATH "/databases" #define REQUESTOR_DATABASE_PATH REQUESTOR_DATABASES_PATH "/permissions.sqlite" -#define APPS_TABLE_DEFINITION "CREATE TABLE IF NOT EXISTS apps (_id INTEGER, uid INTEGER, package TEXT, name TEXT, exec_uid INTEGER, exec_cmd TEXT, allow INTEGER, PRIMARY KEY (_id), UNIQUE (uid,exec_uid,exec_cmd));" -#define LOGS_TABLE_DEFINITION "CREATE TABLE IF NOT EXISTS logs (_id INTEGER, uid INTEGER, name INTEGER, app_id INTEGER, date INTEGER, type INTEGER, PRIMARY KEY (_id));" -#define PREFS_TABLE_DEFINITION "CREATE TABLE IF NOT EXISTS prefs (_id INTEGER, key TEXT, value TEXT, PRIMARY KEY(_id));" - #define DEFAULT_COMMAND "/system/bin/sh" #define SOCKET_PATH_TEMPLATE REQUESTOR_CACHE_PATH "/.socketXXXXXX" From 7d97cfea964c68c9c51a77750f6c7aa86fab66c3 Mon Sep 17 00:00:00 2001 From: "Gleb O. Raiko" Date: Sat, 3 Dec 2011 14:44:51 +0400 Subject: [PATCH 6/7] Fix potential security flaw in creation of sockets/directories Previous code created a file object with two steps: the creation itself with default umask/mode and setting necessary permissions then. This approach is known to lead to a race condition when malicious process can open an object before permissions is set. The patch sets creation mask (mask) to 027, thus denying any access from others. Also, the patch removes all dead code which is not needed after changes mentioned. --- su.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/su.c b/su.c index 5d57480..9bbd439 100644 --- a/su.c +++ b/su.c @@ -146,10 +146,10 @@ static void cleanup_signal(int sig) exit(sig); } -static int socket_create_temp(unsigned req_uid) +static int socket_create_temp(void) { static char buf[PATH_MAX]; - int fd, err; + int fd; struct sockaddr_un sun; @@ -176,18 +176,6 @@ static int socket_create_temp(unsigned req_uid) } } - if (chmod(sun.sun_path, 0600) < 0) { - PLOGE("chmod(socket)"); - unlink(sun.sun_path); - return -1; - } - - if (chown(sun.sun_path, req_uid, req_uid) < 0) { - PLOGE("chown(socket)"); - unlink(sun.sun_path); - return -1; - } - if (listen(fd, 1) < 0) { PLOGE("listen"); return -1; @@ -276,12 +264,13 @@ static void deny(void) exit(EXIT_FAILURE); } -static void allow(char *shell) +static void allow(char *shell, mode_t mask) { struct su_initiator *from = &su_from; struct su_request *to = &su_to; char *exe = NULL; + umask(mask); send_intent(&su_from, &su_to, "", 1, 1); if (!strcmp(shell, "")) { @@ -308,6 +297,7 @@ int main(int argc, char *argv[]) char buf[64], shell[PATH_MAX], *result; int i, dballow; unsigned req_uid; + mode_t orig_umask; for (i = 1; i < argc; i++) { if (!strcmp(argv[i], "-c") || !strcmp(argv[i], "--command")) { @@ -356,8 +346,10 @@ int main(int argc, char *argv[]) deny(); } + orig_umask = umask(027); + if (su_from.uid == AID_ROOT || su_from.uid == AID_SHELL) - allow(shell); + allow(shell, orig_umask); if (stat(REQUESTOR_DATA_PATH, &st) < 0) { PLOGE("stat"); @@ -373,7 +365,7 @@ int main(int argc, char *argv[]) req_uid = st.st_uid; - if (mkdir(REQUESTOR_CACHE_PATH, 0771) >= 0) { + if (mkdir(REQUESTOR_CACHE_PATH, 0770) >= 0) { chown(REQUESTOR_CACHE_PATH, req_uid, req_uid); } @@ -400,12 +392,12 @@ int main(int argc, char *argv[]) switch (dballow) { case DB_DENY: deny(); - case DB_ALLOW: allow(shell); + case DB_ALLOW: allow(shell, orig_umask); case DB_INTERACTIVE: break; default: deny(); } - socket_serv_fd = socket_create_temp(req_uid); + socket_serv_fd = socket_create_temp(); if (socket_serv_fd < 0) { deny(); } @@ -432,7 +424,7 @@ int main(int argc, char *argv[]) if (!strcmp(result, "DENY")) { deny(); } else if (!strcmp(result, "ALLOW")) { - allow(shell); + allow(shell, orig_umask); } else { LOGE("unknown response from Superuser Requestor: %s", result); deny(); From 1fe89fff7b3b4539b2437238ee3032b19cf7e16e Mon Sep 17 00:00:00 2001 From: "Gleb O. Raiko" Date: Sun, 23 Oct 2011 17:30:45 +0400 Subject: [PATCH 7/7] Remove all occurences of req_uid --- su.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/su.c b/su.c index 9bbd439..9bf46bc 100644 --- a/su.c +++ b/su.c @@ -296,7 +296,6 @@ int main(int argc, char *argv[]) static int socket_serv_fd = -1; char buf[64], shell[PATH_MAX], *result; int i, dballow; - unsigned req_uid; mode_t orig_umask; for (i = 1; i < argc; i++) { @@ -363,10 +362,8 @@ int main(int argc, char *argv[]) deny(); } - req_uid = st.st_uid; - if (mkdir(REQUESTOR_CACHE_PATH, 0770) >= 0) { - chown(REQUESTOR_CACHE_PATH, req_uid, req_uid); + chown(REQUESTOR_CACHE_PATH, st.st_uid, st.st_gid); } setgroups(0, NULL);