From c6ef3cc4e6a3d8ba23d3e431d0d1eab483527dfe Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 19 Jul 2012 16:37:23 +0400 Subject: [PATCH 01/25] Add missing usistd.h for system(3) --- activity.c | 1 + 1 file changed, 1 insertion(+) diff --git a/activity.c b/activity.c index fb2c79c..d4c2027 100644 --- a/activity.c +++ b/activity.c @@ -16,6 +16,7 @@ */ #include +#include #include "su.h" From 5bbd487c2283c8c6a58f2c3654e6a5b99ce2b54a Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 19 Jul 2012 19:36:24 +0400 Subject: [PATCH 02/25] Set effective uid/gid back, unlink fails otherwise --- activity.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/activity.c b/activity.c index d4c2027..6c5b516 100644 --- a/activity.c +++ b/activity.c @@ -23,7 +23,10 @@ int send_intent(const struct su_context *ctx, const char *socket_path, int allow, const char *action) { - char command[PATH_MAX]; + char command[PATH_MAX]; + pid_t euid; + gid_t egid; + int rc; sprintf(command, "/system/bin/am broadcast -a '%s' --es socket '%s' --ei caller_uid '%d' --ei allow '%d' --ei version_code '%d' > /dev/null", action, socket_path, ctx->from.uid, allow, VERSION_CODE); @@ -71,7 +74,13 @@ int send_intent(const struct su_context *ctx, // sane value so "am" works setenv("LD_LIBRARY_PATH", "/vendor/lib:/system/lib", 1); + euid = geteuid(); + egid = getegid(); setegid(getgid()); seteuid(getuid()); - return system(command); + rc = system(command); + seteuid(0); + setegid(egid); + seteuid(euid); + return rc; } From a315e38b69ad734194ebfa54a3a325b929dd2d33 Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 19 Jul 2012 19:48:34 +0400 Subject: [PATCH 03/25] Clean up database_check o don't use malloc/free, allocate space on stack o don't use unsafe sprintf o use ARG_MAX as cmd size for clarity, cmd isn't just a filename o remove unsafe logging, ensure last points inside cmd --- db.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/db.c b/db.c index 52f9673..92752d9 100644 --- a/db.c +++ b/db.c @@ -24,29 +24,30 @@ int database_check(const struct su_context *ctx) { FILE *fp; - char allow = '-'; - char *filename = malloc(snprintf(NULL, 0, "%s/%u-%u", REQUESTOR_STORED_PATH, ctx->from.uid, ctx->to.uid) + 1); - sprintf(filename, "%s/%u-%u", REQUESTOR_STORED_PATH, ctx->from.uid, ctx->to.uid); + int allow = '-'; + char filename[PATH_MAX]; + + snprintf(filename, sizeof(filename), + REQUESTOR_STORED_PATH "/%u-%u", ctx->from.uid, ctx->to.uid); if ((fp = fopen(filename, "r"))) { - LOGD("Found file"); - char cmd[PATH_MAX]; + LOGD("Found file %s", filename); + char cmd[ARG_MAX]; fgets(cmd, sizeof(cmd), fp); + /* skip trailing '\n' */ int last = strlen(cmd) - 1; - LOGD("this is the last character %u of the string", cmd[5]); - if (cmd[last] == '\n') { - cmd[last] = '\0'; - } - LOGD("Comparing %c %s, %u to %s", cmd[last - 2], cmd, last, get_command(&ctx->to)); + if (last >= 0) + cmd[last] = 0; + + LOGD("Comparing '%s' to '%s'", cmd, get_command(&ctx->to)); if (strcmp(cmd, get_command(&ctx->to)) == 0) { allow = fgetc(fp); } fclose(fp); } else if ((fp = fopen(REQUESTOR_STORED_DEFAULT, "r"))) { - LOGD("Using default"); + LOGD("Using default file %s", REQUESTOR_STORED_DEFAULT); allow = fgetc(fp); fclose(fp); } - free(filename); if (allow == '1') { return DB_ALLOW; From ffe8b01e0aaaa2676265fb3ab09657220a6310b8 Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 19 Jul 2012 20:05:45 +0400 Subject: [PATCH 04/25] Fix compilation on JB --- db.c | 1 - su.c | 1 - su.h | 11 +++++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/db.c b/db.c index 92752d9..247194e 100644 --- a/db.c +++ b/db.c @@ -17,7 +17,6 @@ #include #include #include -#include #include "su.h" diff --git a/su.c b/su.c index d36eaed..8925536 100644 --- a/su.c +++ b/su.c @@ -35,7 +35,6 @@ #include #include -#include #include "su.h" #include "utils.h" diff --git a/su.h b/su.h index 4cc5485..6b29b07 100644 --- a/su.h +++ b/su.h @@ -88,6 +88,17 @@ static inline char *get_command(const struct su_request *to) return (to->command) ? to->command : to->shell; } +#include +#ifndef LOGE +#define LOGE(...) ALOGE(__VA_ARGS__) +#endif +#ifndef LOGD +#define LOGD(...) ALOGD(__VA_ARGS__) +#endif +#ifndef LOGW +#define LOGW(...) ALOGW(__VA_ARGS__) +#endif + #if 0 #undef LOGE #define LOGE(fmt,args...) fprintf(stderr, fmt, ##args) From 2899822c0ed2c40eb58324e3eed08f7dc44478e6 Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 19 Jul 2012 20:06:21 +0400 Subject: [PATCH 05/25] Use /dev as storage for cache again --- su.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/su.h b/su.h index 6b29b07..9ec7450 100644 --- a/su.h +++ b/su.h @@ -25,7 +25,7 @@ #define REQUESTOR "com.noshufou.android.su" #define REQUESTOR_DATA_PATH "/data/data/" REQUESTOR -#define REQUESTOR_CACHE_PATH REQUESTOR_DATA_PATH "/cache" +#define REQUESTOR_CACHE_PATH "/dev/" REQUESTOR #define REQUESTOR_STORED_PATH REQUESTOR_DATA_PATH "/files/stored" #define REQUESTOR_STORED_DEFAULT REQUESTOR_STORED_PATH "/default" From 0fa617a13ea9c01386986a24cf76936976efb50a Mon Sep 17 00:00:00 2001 From: git-core Date: Mon, 23 Jul 2012 16:04:09 +0400 Subject: [PATCH 06/25] Move headers required by PLOGE in su.h --- su.c | 2 -- su.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/su.c b/su.c index 8925536..7d8928a 100644 --- a/su.c +++ b/su.c @@ -24,11 +24,9 @@ #include #include #include -#include #include #include #include -#include #include #include #include diff --git a/su.h b/su.h index 9ec7450..e9a37a3 100644 --- a/su.h +++ b/su.h @@ -108,6 +108,8 @@ static inline char *get_command(const struct su_request *to) #define LOGW(fmt,args...) fprintf(stderr, fmt, ##args) #endif +#include +#include #define PLOGE(fmt,args...) LOGE(fmt " failed with %d: %s", ##args, errno, strerror(errno)) #define PLOGEV(fmt,err,args...) LOGE(fmt " failed with %d: %s", ##args, err, strerror(err)) From f09ab436caaddb80606e0c3c896b91748869d005 Mon Sep 17 00:00:00 2001 From: git-core Date: Mon, 23 Jul 2012 16:08:41 +0400 Subject: [PATCH 07/25] Use fork/exec instead of system(3) By doing so we may - move change of identity to the child, so no need to change it back - drop saved set-user/group-ID safely An attacker will gain no more than a process with its ids. As a side effect, the code that unsets environment variables becomes useless. - implement asynchronous wait for child later --- activity.c | 99 ++++++++++++++++++++++++------------------------------ su.c | 44 ++++++++++++++---------- su.h | 2 +- 3 files changed, 71 insertions(+), 74 deletions(-) diff --git a/activity.c b/activity.c index 6c5b516..baf6880 100644 --- a/activity.c +++ b/activity.c @@ -15,72 +15,61 @@ ** limitations under the License. */ +#include #include #include +#include +#include +#include #include "su.h" int send_intent(const struct su_context *ctx, const char *socket_path, int allow, const char *action) { - char command[PATH_MAX]; - pid_t euid; - gid_t egid; int rc; - sprintf(command, "/system/bin/am broadcast -a '%s' --es socket '%s' --ei caller_uid '%d' --ei allow '%d' --ei version_code '%d' > /dev/null", - action, socket_path, ctx->from.uid, allow, VERSION_CODE); + pid_t pid = fork(); + /* Child */ + if (!pid) { + char command[ARG_MAX]; - // before sending the intent, make sure the (uid and euid) and (gid and egid) match, - // otherwise LD_LIBRARY_PATH is wiped in Android 4.0+. - // Also, sanitize all secure environment variables (from linker_environ.c in linker). + snprintf(command, sizeof(command), + "exec /system/bin/am broadcast -a %s --es socket '%s' " + "--ei caller_uid %d --ei allow %d " + "--ei version_code %d", + action, socket_path, ctx->from.uid, allow, VERSION_CODE); + char *args[] = { "sh", "-c", command, NULL, }; - /* The same list than GLibc at this point */ - static const char* const unsec_vars[] = { - "GCONV_PATH", - "GETCONF_DIR", - "HOSTALIASES", - "LD_AUDIT", - "LD_DEBUG", - "LD_DEBUG_OUTPUT", - "LD_DYNAMIC_WEAK", - "LD_LIBRARY_PATH", - "LD_ORIGIN_PATH", - "LD_PRELOAD", - "LD_PROFILE", - "LD_SHOW_AUXV", - "LD_USE_LOAD_BIAS", - "LOCALDOMAIN", - "LOCPATH", - "MALLOC_TRACE", - "MALLOC_CHECK_", - "NIS_PATH", - "NLSPATH", - "RESOLV_HOST_CONF", - "RES_OPTIONS", - "TMPDIR", - "TZDIR", - "LD_AOUT_LIBRARY_PATH", - "LD_AOUT_PRELOAD", - // not listed in linker, used due to system() call - "IFS", - }; - const char* const* cp = unsec_vars; - const char* const* endp = cp + sizeof(unsec_vars)/sizeof(unsec_vars[0]); - while (cp < endp) { - unsetenv(*cp); - cp++; + /* + * before sending the intent, make sure the effective uid/gid match + * the real uid/gid, otherwise LD_LIBRARY_PATH is wiped + * in Android 4.0+. + */ + set_identity(ctx->from.uid); + int zero = open("/dev/zero", O_RDONLY | O_CLOEXEC); + dup2(zero, 0); + int null = open("/dev/null", O_WRONLY | O_CLOEXEC); + dup2(null, 1); + dup2(null, 2); + LOGD("Executing %s\n", command); + execv(_PATH_BSHELL, args); + PLOGE("exec am"); + _exit(EXIT_FAILURE); } - - // sane value so "am" works - setenv("LD_LIBRARY_PATH", "/vendor/lib:/system/lib", 1); - euid = geteuid(); - egid = getegid(); - setegid(getgid()); - seteuid(getuid()); - rc = system(command); - seteuid(0); - setegid(egid); - seteuid(euid); - return rc; + /* Parent */ + if (pid < 0) { + PLOGE("fork"); + return -1; + } + pid = waitpid(pid, &rc, 0); + if (pid < 0) { + PLOGE("waitpid"); + return -1; + } + if (!WIFEXITED(rc) || WEXITSTATUS(rc)) { + LOGE("am returned error %d\n", rc); + return -1; + } + return 0; } diff --git a/su.c b/su.c index 7d8928a..cbd7241 100644 --- a/su.c +++ b/su.c @@ -123,6 +123,26 @@ static void populate_environment(const struct su_context *ctx) } } +void set_identity(unsigned int uid) +{ + /* + * Set effective uid back to root, otherwise setres[ug]id will fail + * if uid isn't root. + */ + if (seteuid(0)) { + PLOGE("seteuid (root)"); + exit(EXIT_FAILURE); + } + if (setresgid(uid, uid, uid)) { + PLOGE("setresgid (%u)", uid); + exit(EXIT_FAILURE); + } + if (setresuid(uid, uid, uid)) { + PLOGE("setresuid (%u)", uid); + exit(EXIT_FAILURE); + } +} + static void socket_cleanup(void) { unlink(socket_path); @@ -308,25 +328,8 @@ static void allow(const struct su_context *ctx) arg0 = p; } - /* - * Set effective uid back to root, otherwise setres[ug]id will fail - * if ctx->to.uid isn't root. - */ - if (seteuid(0)) { - PLOGE("seteuid (root)"); - exit(EXIT_FAILURE); - } - populate_environment(ctx); - - if (setresgid(ctx->to.uid, ctx->to.uid, ctx->to.uid)) { - PLOGE("setresgid (%u)", ctx->to.uid); - exit(EXIT_FAILURE); - } - if (setresuid(ctx->to.uid, ctx->to.uid, ctx->to.uid)) { - PLOGE("setresuid (%u)", ctx->to.uid); - exit(EXIT_FAILURE); - } + set_identity(ctx->to.uid); #define PARG(arg) \ (ctx->to.optind + (arg) < ctx->to.argc) ? " " : "", \ @@ -498,6 +501,11 @@ int main(int argc, char *argv[]) } } + /* + * set LD_LIBRARY_PATH if the linker has wiped out it due to we're suid. + * This occurs on Android 4.0+ + */ + setenv("LD_LIBRARY_PATH", "/vendor/lib:/system/lib", 0); if (ctx.from.uid == AID_ROOT || ctx.from.uid == AID_SHELL) allow(&ctx); diff --git a/su.h b/su.h index e9a37a3..b9d6602 100644 --- a/su.h +++ b/su.h @@ -79,7 +79,7 @@ enum { }; extern int database_check(const struct su_context *ctx); - +extern void set_identity(unsigned int uid); extern int send_intent(const struct su_context *ctx, const char *socket_path, int allow, const char *action); From 53f224132351f0dface13c563d7a9c87cf571bb2 Mon Sep 17 00:00:00 2001 From: git-core Date: Mon, 23 Jul 2012 17:19:51 +0400 Subject: [PATCH 08/25] Set close-on-exec for the socket An attacker could access the communication channel between su and Superuser otherwise. --- su.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/su.c b/su.c index cbd7241..b9e8911 100644 --- a/su.c +++ b/su.c @@ -169,6 +169,10 @@ static int socket_create_temp(char *path, size_t len) PLOGE("socket"); return -1; } + if (fcntl(fd, F_SETFD, FD_CLOEXEC)) { + PLOGE("fcntl FD_CLOEXEC"); + goto err; + } memset(&sun, 0, sizeof(sun)); sun.sun_family = AF_LOCAL; From 4692fd8aa93ce35f8ea292c28359259e105925a8 Mon Sep 17 00:00:00 2001 From: git-core Date: Tue, 24 Jul 2012 19:26:35 +0400 Subject: [PATCH 09/25] Move CM-specific code to access_disabled() Restructure the code a bit decreasing the cost of checks for non-CM ROMs --- su.c | 111 +++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 62 insertions(+), 49 deletions(-) diff --git a/su.c b/su.c index b9e8911..85f1aa3 100644 --- a/su.c +++ b/su.c @@ -358,6 +358,64 @@ static void allow(const struct su_context *ctx) exit(EXIT_FAILURE); } +/* + * CyanogenMod-specific behavior + * + * we can't simply use the property service, since we aren't launched from init + * and can't trust the location of the property workspace. + * Find the properties ourselves. + */ +int access_disabled(const struct su_initiator *from) +{ + char *data; + char cm_version[PROPERTY_VALUE_MAX], build_type[PROPERTY_VALUE_MAX]; + char debuggable[PROPERTY_VALUE_MAX], enabled[PROPERTY_VALUE_MAX]; + size_t len; + unsigned int sz; + + data = read_file("/system/build.prop", &sz); + get_property(data, cm_version, "ro.cm.version", ""); + if (strlen(cm_version) > 0) { + get_property(data, build_type, "ro.build.type", ""); + free(data); + + data = read_file("/default.prop", &sz); + get_property(data, debuggable, "ro.debuggable", "0"); + free(data); + /* only allow su on debuggable builds */ + if (strcmp("1", debuggable) != 0) { + LOGE("Root access is disabled on non-debug builds"); + return 1; + } + + data = read_file("/data/property/persist.sys.root_access", &sz); + if (data != NULL) { + len = strlen(data); + if (len >= PROPERTY_VALUE_MAX) + memcpy(enabled, "1", 2); + else + memcpy(enabled, data, len + 1); + free(data); + } else + memcpy(enabled, "1", 2); + + /* enforce persist.sys.root_access on non-eng builds */ + if (strcmp("eng", build_type) != 0 && (atoi(enabled) & 1) != 1 ) { + LOGE("Root access is disabled by system setting - " + "enable it under settings -> developer options"); + return 1; + } + + /* disallow su in a shell if appropriate */ + if (from->uid == AID_SHELL && (atoi(enabled) == 1)) { + LOGE("Root access is disabled by a system setting - " + "enable it under settings -> developer options"); + return 1; + } + } + return 0; +} + int main(int argc, char *argv[]) { struct su_context ctx = { @@ -380,10 +438,8 @@ int main(int argc, char *argv[]) }; struct stat st; int socket_serv_fd, fd; - char buf[64], *result, debuggable[PROPERTY_VALUE_MAX]; - char enabled[PROPERTY_VALUE_MAX], build_type[PROPERTY_VALUE_MAX]; - char cm_version[PROPERTY_VALUE_MAX];; - int c, dballow, len; + char buf[64], *result; + int c, dballow; struct option long_opts[] = { { "command", required_argument, NULL, 'c' }, { "help", no_argument, NULL, 'h' }, @@ -393,8 +449,6 @@ int main(int argc, char *argv[]) { "version", no_argument, NULL, 'v' }, { NULL, 0, NULL, 0 }, }; - char *data; - unsigned sz; while ((c = getopt_long(argc, argv, "+c:hlmps:Vv", long_opts, NULL)) != -1) { switch(c) { @@ -459,52 +513,11 @@ int main(int argc, char *argv[]) deny(&ctx); } - // we can't simply use the property service, since we aren't launched from init and - // can't trust the location of the property workspace. find the properties ourselves. - data = read_file("/default.prop", &sz); - get_property(data, debuggable, "ro.debuggable", "0"); - free(data); - - data = read_file("/system/build.prop", &sz); - get_property(data, cm_version, "ro.cm.version", ""); - get_property(data, build_type, "ro.build.type", ""); - free(data); - - data = read_file("/data/property/persist.sys.root_access", &sz); - if (data != NULL) { - len = strlen(data); - if (len >= PROPERTY_VALUE_MAX) - memcpy(enabled, "1", 2); - else - memcpy(enabled, data, len + 1); - free(data); - } else - memcpy(enabled, "1", 2); + if (access_disabled(&ctx.from)) + deny(&ctx); ctx.umask = umask(027); - // CyanogenMod-specific behavior - if (strlen(cm_version) > 0) { - // only allow su on debuggable builds - if (strcmp("1", debuggable) != 0) { - LOGE("Root access is disabled on non-debug builds"); - deny(&ctx); - } - - // enforce persist.sys.root_access on non-eng builds - if (strcmp("eng", build_type) != 0 && - (atoi(enabled) & 1) != 1 ) { - LOGE("Root access is disabled by system setting - enable it under settings -> developer options"); - deny(&ctx); - } - - // disallow su in a shell if appropriate - if (ctx.from.uid == AID_SHELL && (atoi(enabled) == 1)) { - LOGE("Root access is disabled by a system setting - enable it under settings -> developer options"); - deny(&ctx); - } - } - /* * set LD_LIBRARY_PATH if the linker has wiped out it due to we're suid. * This occurs on Android 4.0+ From 2ceab21e6c1b593d7fee6b88209a15f878601958 Mon Sep 17 00:00:00 2001 From: git-core Date: Wed, 25 Jul 2012 14:55:12 +0400 Subject: [PATCH 10/25] Speed up the CM-specific code a bit for non-CM ROMs Just check whether ro.cm.version exists without getting its value nobody cares of anyway. The check_property function used here returns true if a property with given prefix exists. More than enough for CM and others. Certainly, the best way is to fix the nighmare called get_property. At least, those stupid strdup() shall be removed. If somebody is willing to fix this "code", he/she is welcomed. --- su.c | 12 +++++------- utils.c | 50 ++++++++++++++++++++++++++++++-------------------- utils.h | 8 +++++--- 3 files changed, 40 insertions(+), 30 deletions(-) diff --git a/su.c b/su.c index 85f1aa3..1a55150 100644 --- a/su.c +++ b/su.c @@ -368,18 +368,16 @@ static void allow(const struct su_context *ctx) int access_disabled(const struct su_initiator *from) { char *data; - char cm_version[PROPERTY_VALUE_MAX], build_type[PROPERTY_VALUE_MAX]; + char build_type[PROPERTY_VALUE_MAX]; char debuggable[PROPERTY_VALUE_MAX], enabled[PROPERTY_VALUE_MAX]; size_t len; - unsigned int sz; - data = read_file("/system/build.prop", &sz); - get_property(data, cm_version, "ro.cm.version", ""); - if (strlen(cm_version) > 0) { + data = read_file("/system/build.prop"); + if (check_property(data, "ro.cm.version")) { get_property(data, build_type, "ro.build.type", ""); free(data); - data = read_file("/default.prop", &sz); + data = read_file("/default.prop"); get_property(data, debuggable, "ro.debuggable", "0"); free(data); /* only allow su on debuggable builds */ @@ -388,7 +386,7 @@ int access_disabled(const struct su_initiator *from) return 1; } - data = read_file("/data/property/persist.sys.root_access", &sz); + data = read_file("/data/property/persist.sys.root_access"); if (data != NULL) { len = strlen(data); if (len >= PROPERTY_VALUE_MAX) diff --git a/utils.c b/utils.c index ec6444b..7d9c44c 100644 --- a/utils.c +++ b/utils.c @@ -14,6 +14,8 @@ ** limitations under the License. */ +#include +#include #include #include #include @@ -27,35 +29,29 @@ #include /* reads a file, making sure it is terminated with \n \0 */ -char* read_file(const char *fn, unsigned *_sz) +char* read_file(const char *fn) { - char *data; - int sz; - int fd; + struct stat st; + char *data = NULL; - data = 0; - fd = open(fn, O_RDONLY); - if(fd < 0) return 0; + int fd = open(fn, O_RDONLY); + if (fd < 0) return data; - sz = lseek(fd, 0, SEEK_END); - if(sz < 0) goto oops; + if (fstat(fd, &st)) goto oops; - if(lseek(fd, 0, SEEK_SET) != 0) goto oops; + data = malloc(st.st_size + 2); + if (!data) goto oops; - data = (char*) malloc(sz + 2); - if(data == 0) goto oops; - - if(read(fd, data, sz) != sz) goto oops; + if (read(fd, data, st.st_size) != st.st_size) goto oops; close(fd); - data[sz] = '\n'; - data[sz+1] = 0; - if(_sz) *_sz = sz; + data[st.st_size] = '\n'; + data[st.st_size + 1] = 0; return data; oops: close(fd); - if(data != 0) free(data); - return 0; + if (data) free(data); + return NULL; } int get_property(const char *data, char *found, const char *searchkey, const char *not_found) @@ -100,4 +96,18 @@ int get_property(const char *data, char *found, const char *searchkey, const cha len = strlen(not_found); memcpy(found, not_found, len + 1); return len; -} \ No newline at end of file +} + +/* + * Fast version of get_property which purpose is to check + * whether the property with given prefix exists. + * + * Assume nobody is stupid enough to put a propery with prefix ro.cm.version + * in his build.prop on a non-CM ROM and comment it out. + */ +int check_property(const char *data, const char *prefix) +{ + if (!data) + return 0; + return strstr(data, prefix) != NULL; +} diff --git a/utils.h b/utils.h index 92e1ace..16ec345 100644 --- a/utils.h +++ b/utils.h @@ -18,7 +18,9 @@ #define _UTILS_H_ /* reads a file, making sure it is terminated with \n \0 */ -char* read_file(const char *fn, unsigned *_sz); +extern char* read_file(const char *fn); -int get_property(const char *data, char *found, const char *searchkey, const char *not_found); -#endif \ No newline at end of file +extern int get_property(const char *data, char *found, const char *searchkey, + const char *not_found); +extern int check_property(const char *data, const char *prefix); +#endif From fcd02a707856c5f1ee43b4ec88b810633b2eda9b Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 26 Jul 2012 14:18:04 +0400 Subject: [PATCH 11/25] Declare deny/allow as noreturn --- su.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/su.c b/su.c index 1a55150..76e38dc 100644 --- a/su.c +++ b/su.c @@ -300,7 +300,7 @@ static void usage(int status) exit(status); } -static void deny(const struct su_context *ctx) +static __attribute__ ((noreturn)) void deny(const struct su_context *ctx) { char *cmd = get_command(&ctx->to); @@ -310,7 +310,7 @@ static void deny(const struct su_context *ctx) exit(EXIT_FAILURE); } -static void allow(const struct su_context *ctx) +static __attribute__ ((noreturn)) void allow(const struct su_context *ctx) { char *arg0; int argc, err; @@ -611,7 +611,4 @@ int main(int argc, char *argv[]) LOGE("unknown response from Superuser Requestor: %s", result); deny(&ctx); } - - deny(&ctx); - return -1; } From acf38cda6979e558232f0fb3609ffc20c073bc04 Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 26 Jul 2012 14:40:00 +0400 Subject: [PATCH 12/25] Move socket_path to the su context Choose the value of the socket field in intents depending on the allow arg, because the socket_path argument is removed from send_intent. Use enum type for allow. --- activity.c | 3 ++- db.c | 6 +++--- su.c | 45 +++++++++++++++++++++++++++------------------ su.h | 15 ++++++++------- 4 files changed, 40 insertions(+), 29 deletions(-) diff --git a/activity.c b/activity.c index baf6880..fddae4f 100644 --- a/activity.c +++ b/activity.c @@ -25,13 +25,14 @@ #include "su.h" int send_intent(const struct su_context *ctx, - const char *socket_path, int allow, const char *action) + allow_t allow, const char *action) { int rc; pid_t pid = fork(); /* Child */ if (!pid) { + const char *socket_path = (allow == INTERACTIVE) ? ctx->sock_path : ""; char command[ARG_MAX]; snprintf(command, sizeof(command), diff --git a/db.c b/db.c index 247194e..09bce7c 100644 --- a/db.c +++ b/db.c @@ -49,10 +49,10 @@ int database_check(const struct su_context *ctx) } if (allow == '1') { - return DB_ALLOW; + return ALLOW; } else if (allow == '0') { - return DB_DENY; + return DENY; } else { - return DB_INTERACTIVE; + return INTERACTIVE; } } diff --git a/su.c b/su.c index 76e38dc..4580b1c 100644 --- a/su.c +++ b/su.c @@ -37,9 +37,6 @@ #include "su.h" #include "utils.h" -/* Still lazt, will fix this */ -static char socket_path[PATH_MAX]; - static int from_init(struct su_initiator *from) { char path[PATH_MAX], exe[PATH_MAX]; @@ -143,19 +140,30 @@ void set_identity(unsigned int uid) } } -static void socket_cleanup(void) +static void socket_cleanup(struct su_context *ctx) { - unlink(socket_path); + if (ctx && ctx->sock_path[0]) { + if (unlink(ctx->sock_path)) + PLOGE("unlink (%s)", ctx->sock_path); + ctx->sock_path[0] = 0; + } } +/* + * For use in signal handlers/atexit-function + * NOTE: su_ctx points to main's local variable. + * It's OK due to the program uses exit(3), not return from main() + */ +static struct su_context *su_ctx = NULL; + static void cleanup(void) { - socket_cleanup(); + socket_cleanup(su_ctx); } static void cleanup_signal(int sig) { - socket_cleanup(); + socket_cleanup(su_ctx); exit(128 + sig); } @@ -304,7 +312,7 @@ static __attribute__ ((noreturn)) void deny(const struct su_context *ctx) { char *cmd = get_command(&ctx->to); - send_intent(ctx, "", 0, ACTION_RESULT); + send_intent(ctx, DENY, ACTION_RESULT); LOGW("request rejected (%u->%u %s)", ctx->from.uid, ctx->to.uid, cmd); fprintf(stderr, "%s\n", strerror(EACCES)); exit(EXIT_FAILURE); @@ -316,7 +324,7 @@ static __attribute__ ((noreturn)) void allow(const struct su_context *ctx) int argc, err; umask(ctx->umask); - send_intent(ctx, "", 1, ACTION_RESULT); + send_intent(ctx, ALLOW, ACTION_RESULT); arg0 = strrchr (ctx->to.shell, '/'); arg0 = (arg0) ? arg0 + 1 : ctx->to.shell; @@ -435,9 +443,9 @@ int main(int argc, char *argv[]) }, }; struct stat st; - int socket_serv_fd, fd; + int c, socket_serv_fd, fd; char buf[64], *result; - int c, dballow; + allow_t dballow; struct option long_opts[] = { { "command", required_argument, NULL, 'c' }, { "help", no_argument, NULL, 'h' }, @@ -557,17 +565,18 @@ int main(int argc, char *argv[]) dballow = database_check(&ctx); switch (dballow) { - case DB_DENY: deny(&ctx); - case DB_ALLOW: allow(&ctx); - case DB_INTERACTIVE: break; - default: deny(&ctx); + case INTERACTIVE: break; + case ALLOW: allow(&ctx); /* never returns */ + case DENY: + default: deny(&ctx); /* never returns too */ } - socket_serv_fd = socket_create_temp(socket_path, sizeof(socket_path)); + socket_serv_fd = socket_create_temp(ctx.sock_path, sizeof(ctx.sock_path)); if (socket_serv_fd < 0) { deny(&ctx); } + su_ctx = &ctx; signal(SIGHUP, cleanup_signal); signal(SIGPIPE, cleanup_signal); signal(SIGTERM, cleanup_signal); @@ -576,7 +585,7 @@ int main(int argc, char *argv[]) signal(SIGABRT, cleanup_signal); atexit(cleanup); - if (send_intent(&ctx, socket_path, -1, ACTION_REQUEST) < 0) { + if (send_intent(&ctx, INTERACTIVE, ACTION_REQUEST) < 0) { deny(&ctx); } @@ -593,7 +602,7 @@ int main(int argc, char *argv[]) close(fd); close(socket_serv_fd); - socket_cleanup(); + socket_cleanup(&ctx); result = buf; diff --git a/su.h b/su.h index b9d6602..30c4a34 100644 --- a/su.h +++ b/su.h @@ -70,18 +70,19 @@ struct su_context { struct su_initiator from; struct su_request to; mode_t umask; + char sock_path[PATH_MAX]; }; -enum { - DB_INTERACTIVE, - DB_DENY, - DB_ALLOW -}; +typedef enum { + INTERACTIVE = -1, + DENY = 0, + ALLOW = 1, +} allow_t; -extern int database_check(const struct su_context *ctx); +extern allow_t database_check(const struct su_context *ctx); extern void set_identity(unsigned int uid); extern int send_intent(const struct su_context *ctx, - const char *socket_path, int allow, const char *action); + allow_t allow, const char *action); static inline char *get_command(const struct su_request *to) { From dbe124982f950db932999cac1bfa57442ab91ea7 Mon Sep 17 00:00:00 2001 From: git-core Date: Thu, 26 Jul 2012 20:17:28 +0400 Subject: [PATCH 13/25] Don't wait until am terminates, kill it, if it hangs Pick up am exit code in the SIGCHLD handler. Kill am with request (first child) after a response has been received from Requestor, if that am hangs. --- activity.c | 18 ++++++------------ su.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++--- su.h | 3 ++- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/activity.c b/activity.c index fddae4f..4588e90 100644 --- a/activity.c +++ b/activity.c @@ -24,10 +24,12 @@ #include "su.h" -int send_intent(const struct su_context *ctx, - allow_t allow, const char *action) +int send_intent(struct su_context *ctx, allow_t allow, const char *action) { - int rc; + if (ctx->child) { + LOGE("child %d already running", ctx->child); + return -1; + } pid_t pid = fork(); /* Child */ @@ -63,14 +65,6 @@ int send_intent(const struct su_context *ctx, PLOGE("fork"); return -1; } - pid = waitpid(pid, &rc, 0); - if (pid < 0) { - PLOGE("waitpid"); - return -1; - } - if (!WIFEXITED(rc) || WEXITSTATUS(rc)) { - LOGE("am returned error %d\n", rc); - return -1; - } + ctx->child = pid; return 0; } diff --git a/su.c b/su.c index 4580b1c..96add24 100644 --- a/su.c +++ b/su.c @@ -149,6 +149,38 @@ static void socket_cleanup(struct su_context *ctx) } } +static void kill_child(pid_t pid) +{ + LOGD("killing child %d", pid); + if (pid && kill(pid, SIGKILL)) + PLOGE("kill (%d)", pid); +} + +static void child_cleanup(struct su_context *ctx) +{ + pid_t pid = (ctx && ctx->child) ? ctx->child : -1; + int rc; + + if (!ctx) + LOGE("no context present"); + if (!ctx->child) + LOGE("unexpected child"); + + pid = waitpid(pid, &rc, WNOHANG); + if (pid < 0) { + PLOGE("waitpid"); + exit(EXIT_FAILURE); + } + if (WIFEXITED(rc) && WEXITSTATUS(rc)) { + LOGE("child %d terminated with error %d", pid, rc); + exit(EXIT_FAILURE); + } + LOGE("child %d terminated, status %d", pid, rc); + + if (ctx) + ctx->child = 0; +} + /* * For use in signal handlers/atexit-function * NOTE: su_ctx points to main's local variable. @@ -167,6 +199,12 @@ static void cleanup_signal(int sig) exit(128 + sig); } +static void sigchld_handler(int sig) +{ + child_cleanup(su_ctx); + (void)sig; +} + static int socket_create_temp(char *path, size_t len) { int fd; @@ -308,7 +346,7 @@ static void usage(int status) exit(status); } -static __attribute__ ((noreturn)) void deny(const struct su_context *ctx) +static __attribute__ ((noreturn)) void deny(struct su_context *ctx) { char *cmd = get_command(&ctx->to); @@ -318,7 +356,7 @@ static __attribute__ ((noreturn)) void deny(const struct su_context *ctx) exit(EXIT_FAILURE); } -static __attribute__ ((noreturn)) void allow(const struct su_context *ctx) +static __attribute__ ((noreturn)) void allow(struct su_context *ctx) { char *arg0; int argc, err; @@ -441,7 +479,9 @@ int main(int argc, char *argv[]) .argc = argc, .optind = 0, }, + .child = 0, }; + struct sigaction act; struct stat st; int c, socket_serv_fd, fd; char buf[64], *result; @@ -515,6 +555,15 @@ int main(int argc, char *argv[]) } ctx.to.optind = optind; + su_ctx = &ctx; + act.sa_handler = sigchld_handler; + sigemptyset(&act.sa_mask); + act.sa_flags = SA_NOCLDSTOP | SA_RESTART; + if (sigaction(SIGCHLD, &act, NULL)) { + PLOGE("sigaction(SIGCHLD)"); + exit(EXIT_FAILURE); + } + if (from_init(&ctx.from) < 0) { deny(&ctx); } @@ -576,7 +625,6 @@ int main(int argc, char *argv[]) deny(&ctx); } - su_ctx = &ctx; signal(SIGHUP, cleanup_signal); signal(SIGPIPE, cleanup_signal); signal(SIGTERM, cleanup_signal); @@ -604,6 +652,8 @@ int main(int argc, char *argv[]) close(socket_serv_fd); socket_cleanup(&ctx); + kill_child(ctx.child); /* we're sure am has done its job at this point */ + result = buf; #define SOCKET_RESPONSE "socket:" diff --git a/su.h b/su.h index 30c4a34..28b471f 100644 --- a/su.h +++ b/su.h @@ -70,6 +70,7 @@ struct su_context { struct su_initiator from; struct su_request to; mode_t umask; + pid_t child; char sock_path[PATH_MAX]; }; @@ -81,7 +82,7 @@ typedef enum { extern allow_t database_check(const struct su_context *ctx); extern void set_identity(unsigned int uid); -extern int send_intent(const struct su_context *ctx, +extern int send_intent(struct su_context *ctx, allow_t allow, const char *action); static inline char *get_command(const struct su_request *to) From 40c3d1756461061c6d22aad89d452daa45dddf1f Mon Sep 17 00:00:00 2001 From: git-core Date: Sat, 28 Jul 2012 15:19:16 +0400 Subject: [PATCH 14/25] Guard select from SIGCHLD --- su.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/su.c b/su.c index 96add24..bb3c7a9 100644 --- a/su.c +++ b/su.c @@ -252,14 +252,17 @@ static int socket_accept(int serv_fd) { struct timeval tv; fd_set fds; - int fd; + int fd, rc; /* Wait 20 seconds for a connection, then give up. */ tv.tv_sec = 20; tv.tv_usec = 0; FD_ZERO(&fds); FD_SET(serv_fd, &fds); - if (select(serv_fd + 1, &fds, NULL, NULL, &tv) < 1) { + do { + rc = select(serv_fd + 1, &fds, NULL, NULL, &tv); + } while (rc < 0 && errno == EINTR); + if (rc < 1) { PLOGE("select"); return -1; } From 679977961cd3b73b65f4e333d2e0ddcd4a55eb9c Mon Sep 17 00:00:00 2001 From: git-core Date: Tue, 31 Jul 2012 17:55:52 +0400 Subject: [PATCH 15/25] Wait until the child is really killed, ... ... so su can get its exit status --- su.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/su.c b/su.c index bb3c7a9..b2e68ee 100644 --- a/su.c +++ b/su.c @@ -152,8 +152,22 @@ static void socket_cleanup(struct su_context *ctx) static void kill_child(pid_t pid) { LOGD("killing child %d", pid); - if (pid && kill(pid, SIGKILL)) - PLOGE("kill (%d)", pid); + if (pid) { + sigset_t set, old; + + sigemptyset(&set); + sigaddset(&set, SIGCHLD); + if (sigprocmask(SIG_BLOCK, &set, &old)) { + PLOGE("sigprocmask(SIG_BLOCK)"); + return; + } + if (kill(pid, SIGKILL)) + PLOGE("kill (%d)", pid); + else if (sigsuspend(&old) && errno != EINTR) + PLOGE("sigsuspend"); + if (sigprocmask(SIG_SETMASK, &old, NULL)) + PLOGE("sigprocmask(SIG_BLOCK)"); + } } static void child_cleanup(struct su_context *ctx) @@ -172,10 +186,14 @@ static void child_cleanup(struct su_context *ctx) exit(EXIT_FAILURE); } if (WIFEXITED(rc) && WEXITSTATUS(rc)) { - LOGE("child %d terminated with error %d", pid, rc); + LOGE("child %d terminated with error %d", pid, WEXITSTATUS(rc)); + exit(EXIT_FAILURE); + } + if (WIFSIGNALED(rc) && WTERMSIG(rc) != SIGKILL) { + LOGE("child %d terminated with signal %d", pid, WTERMSIG(rc)); exit(EXIT_FAILURE); } - LOGE("child %d terminated, status %d", pid, rc); + LOGD("child %d terminated, status %d", pid, rc); if (ctx) ctx->child = 0; From dfb29f9598c3886e58986688d11af8714204414d Mon Sep 17 00:00:00 2001 From: git-core Date: Tue, 31 Jul 2012 20:02:50 +0400 Subject: [PATCH 16/25] Don't touch su context in children It's too high on the stack --- activity.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/activity.c b/activity.c index 4588e90..8877476 100644 --- a/activity.c +++ b/activity.c @@ -26,22 +26,29 @@ int send_intent(struct su_context *ctx, allow_t allow, const char *action) { + const char *socket_path; + unsigned int uid = ctx->from.uid; + if (ctx->child) { LOGE("child %d already running", ctx->child); return -1; } + if (allow == INTERACTIVE) { + socket_path = ctx->sock_path; + } else { + socket_path = ""; + } pid_t pid = fork(); /* Child */ if (!pid) { - const char *socket_path = (allow == INTERACTIVE) ? ctx->sock_path : ""; char command[ARG_MAX]; snprintf(command, sizeof(command), "exec /system/bin/am broadcast -a %s --es socket '%s' " "--ei caller_uid %d --ei allow %d " "--ei version_code %d", - action, socket_path, ctx->from.uid, allow, VERSION_CODE); + action, socket_path, uid, allow, VERSION_CODE); char *args[] = { "sh", "-c", command, NULL, }; /* @@ -49,7 +56,7 @@ int send_intent(struct su_context *ctx, allow_t allow, const char *action) * the real uid/gid, otherwise LD_LIBRARY_PATH is wiped * in Android 4.0+. */ - set_identity(ctx->from.uid); + set_identity(uid); int zero = open("/dev/zero", O_RDONLY | O_CLOEXEC); dup2(zero, 0); int null = open("/dev/null", O_WRONLY | O_CLOEXEC); From 70a5e416f0c83f5b134a6e61ef7e60c85ee10422 Mon Sep 17 00:00:00 2001 From: git-core Date: Wed, 1 Aug 2012 18:14:58 +0400 Subject: [PATCH 17/25] Move kill_child into send_intent --- activity.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- su.c | 34 +--------------------------------- su.h | 1 + 3 files changed, 47 insertions(+), 37 deletions(-) diff --git a/activity.c b/activity.c index 8877476..1645ced 100644 --- a/activity.c +++ b/activity.c @@ -20,26 +20,67 @@ #include #include #include -#include #include "su.h" +static void kill_child(pid_t pid) +{ + LOGD("killing child %d", pid); + if (pid) { + sigset_t set, old; + + sigemptyset(&set); + sigaddset(&set, SIGCHLD); + if (sigprocmask(SIG_BLOCK, &set, &old)) { + PLOGE("sigprocmask(SIG_BLOCK)"); + return; + } + if (kill(pid, SIGKILL)) + PLOGE("kill (%d)", pid); + else if (sigsuspend(&old) && errno != EINTR) + PLOGE("sigsuspend"); + if (sigprocmask(SIG_SETMASK, &old, NULL)) + PLOGE("sigprocmask(SIG_BLOCK)"); + } +} + +static void setup_sigchld_handler(__sighandler_t handler) +{ + struct sigaction act; + + act.sa_handler = handler; + sigemptyset(&act.sa_mask); + act.sa_flags = SA_NOCLDSTOP | SA_RESTART; + if (sigaction(SIGCHLD, &act, NULL)) { + PLOGE("sigaction(SIGCHLD)"); + exit(EXIT_FAILURE); + } +} + int send_intent(struct su_context *ctx, allow_t allow, const char *action) { const char *socket_path; unsigned int uid = ctx->from.uid; + __sighandler_t handler; + pid_t pid; if (ctx->child) { - LOGE("child %d already running", ctx->child); - return -1; + kill_child(ctx->child); + if (ctx->child) { + LOGE("child %d is still running", ctx->child); + return -1; + } } if (allow == INTERACTIVE) { socket_path = ctx->sock_path; + handler = sigchld_handler; } else { socket_path = ""; + handler = SIG_IGN; } + setup_sigchld_handler(handler); - pid_t pid = fork(); + pid = fork(); /* Child */ if (!pid) { char command[ARG_MAX]; diff --git a/su.c b/su.c index b2e68ee..63b3ed1 100644 --- a/su.c +++ b/su.c @@ -149,27 +149,6 @@ static void socket_cleanup(struct su_context *ctx) } } -static void kill_child(pid_t pid) -{ - LOGD("killing child %d", pid); - if (pid) { - sigset_t set, old; - - sigemptyset(&set); - sigaddset(&set, SIGCHLD); - if (sigprocmask(SIG_BLOCK, &set, &old)) { - PLOGE("sigprocmask(SIG_BLOCK)"); - return; - } - if (kill(pid, SIGKILL)) - PLOGE("kill (%d)", pid); - else if (sigsuspend(&old) && errno != EINTR) - PLOGE("sigsuspend"); - if (sigprocmask(SIG_SETMASK, &old, NULL)) - PLOGE("sigprocmask(SIG_BLOCK)"); - } -} - static void child_cleanup(struct su_context *ctx) { pid_t pid = (ctx && ctx->child) ? ctx->child : -1; @@ -217,7 +196,7 @@ static void cleanup_signal(int sig) exit(128 + sig); } -static void sigchld_handler(int sig) +void sigchld_handler(int sig) { child_cleanup(su_ctx); (void)sig; @@ -502,7 +481,6 @@ int main(int argc, char *argv[]) }, .child = 0, }; - struct sigaction act; struct stat st; int c, socket_serv_fd, fd; char buf[64], *result; @@ -577,14 +555,6 @@ int main(int argc, char *argv[]) ctx.to.optind = optind; su_ctx = &ctx; - act.sa_handler = sigchld_handler; - sigemptyset(&act.sa_mask); - act.sa_flags = SA_NOCLDSTOP | SA_RESTART; - if (sigaction(SIGCHLD, &act, NULL)) { - PLOGE("sigaction(SIGCHLD)"); - exit(EXIT_FAILURE); - } - if (from_init(&ctx.from) < 0) { deny(&ctx); } @@ -673,8 +643,6 @@ int main(int argc, char *argv[]) close(socket_serv_fd); socket_cleanup(&ctx); - kill_child(ctx.child); /* we're sure am has done its job at this point */ - result = buf; #define SOCKET_RESPONSE "socket:" diff --git a/su.h b/su.h index 28b471f..968a116 100644 --- a/su.h +++ b/su.h @@ -84,6 +84,7 @@ extern allow_t database_check(const struct su_context *ctx); extern void set_identity(unsigned int uid); extern int send_intent(struct su_context *ctx, allow_t allow, const char *action); +extern void sigchld_handler(int sig); static inline char *get_command(const struct su_request *to) { From fb7c8d1d24492028f1810fcfdf06f889385c0bfa Mon Sep 17 00:00:00 2001 From: git-core Date: Wed, 1 Aug 2012 18:32:23 +0400 Subject: [PATCH 18/25] Mark child as volatile It's shared between the SIGCHLD signal handler and normal flow. Its type is still pid_t, however, not sig_atomic_t. --- activity.c | 10 ++++++---- su.c | 13 +++++-------- su.h | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/activity.c b/activity.c index 1645ced..a515204 100644 --- a/activity.c +++ b/activity.c @@ -64,10 +64,12 @@ int send_intent(struct su_context *ctx, allow_t allow, const char *action) __sighandler_t handler; pid_t pid; - if (ctx->child) { - kill_child(ctx->child); - if (ctx->child) { - LOGE("child %d is still running", ctx->child); + pid = ctx->child; + if (pid) { + kill_child(pid); + pid = ctx->child; + if (pid) { + LOGE("child %d is still running", pid); return -1; } } diff --git a/su.c b/su.c index 63b3ed1..afc0b07 100644 --- a/su.c +++ b/su.c @@ -151,14 +151,13 @@ static void socket_cleanup(struct su_context *ctx) static void child_cleanup(struct su_context *ctx) { - pid_t pid = (ctx && ctx->child) ? ctx->child : -1; + pid_t pid = ctx->child; int rc; - if (!ctx) - LOGE("no context present"); - if (!ctx->child) + if (!pid) { LOGE("unexpected child"); - + pid = -1; /* pick up any child */ + } pid = waitpid(pid, &rc, WNOHANG); if (pid < 0) { PLOGE("waitpid"); @@ -173,9 +172,7 @@ static void child_cleanup(struct su_context *ctx) exit(EXIT_FAILURE); } LOGD("child %d terminated, status %d", pid, rc); - - if (ctx) - ctx->child = 0; + ctx->child = 0; } /* diff --git a/su.h b/su.h index 968a116..586b48a 100644 --- a/su.h +++ b/su.h @@ -70,7 +70,7 @@ struct su_context { struct su_initiator from; struct su_request to; mode_t umask; - pid_t child; + volatile pid_t child; char sock_path[PATH_MAX]; }; From 6a5c06e93c8c0706b19e0009b99d2275c848ba86 Mon Sep 17 00:00:00 2001 From: git-core Date: Wed, 1 Aug 2012 18:45:07 +0400 Subject: [PATCH 19/25] Remove defines htobe32 and like, not htonl we're using. Strictly speaking, we have to to include . su isn't aimed to be so portable though. --- su.c | 1 - utils.c | 2 -- 2 files changed, 3 deletions(-) diff --git a/su.c b/su.c index afc0b07..d6a5e0f 100644 --- a/su.c +++ b/su.c @@ -24,7 +24,6 @@ #include #include #include -#include #include #include #include diff --git a/utils.c b/utils.c index 7d9c44c..4f6f9e2 100644 --- a/utils.c +++ b/utils.c @@ -20,9 +20,7 @@ #include #include #include -#include #include - #include #include #include From ad1151e7dc72ca91c40dff9516b8c77c3e4b44f5 Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Sun, 19 Aug 2012 05:27:52 +0100 Subject: [PATCH 20/25] Read in the extra line of the default store file Corresponds with Superuser repo commit e16376e17a7680c50bdf626a73f46a8b60bd4793 --- db.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/db.c b/db.c index 09bce7c..b09de13 100644 --- a/db.c +++ b/db.c @@ -44,7 +44,15 @@ int database_check(const struct su_context *ctx) fclose(fp); } else if ((fp = fopen(REQUESTOR_STORED_DEFAULT, "r"))) { LOGD("Using default file %s", REQUESTOR_STORED_DEFAULT); - allow = fgetc(fp); + char cmd[ARG_MAX]; + fgets(cmd, sizeof(cmd), fp); + int last = strlen(cmd) - 1; + if (last >= 0) + cmd[last] = 0; + + if (strcmp(cmd, "default") == 0) { + allow = fgetc(fp); + } fclose(fp); } From fa0b93d42e4bdd633dc8c67961adbf2671d21076 Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Sun, 19 Aug 2012 08:09:54 +0100 Subject: [PATCH 21/25] Prevent command line pollution from allowing privilege escalation See matching commit in Superuser repo --- db.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/db.c b/db.c index b09de13..cde757d 100644 --- a/db.c +++ b/db.c @@ -23,42 +23,45 @@ int database_check(const struct su_context *ctx) { FILE *fp; - int allow = '-'; char filename[PATH_MAX]; + char allow[7]; + int last = 0; snprintf(filename, sizeof(filename), REQUESTOR_STORED_PATH "/%u-%u", ctx->from.uid, ctx->to.uid); if ((fp = fopen(filename, "r"))) { LOGD("Found file %s", filename); + + fgets(allow, sizeof(allow), fp); + last = strlen(allow) - 1; + if (last >= 0) + allow[last] = 0; + char cmd[ARG_MAX]; fgets(cmd, sizeof(cmd), fp); /* skip trailing '\n' */ - int last = strlen(cmd) - 1; + last = strlen(cmd) - 1; if (last >= 0) cmd[last] = 0; LOGD("Comparing '%s' to '%s'", cmd, get_command(&ctx->to)); - if (strcmp(cmd, get_command(&ctx->to)) == 0) { - allow = fgetc(fp); + if (strcmp(cmd, get_command(&ctx->to)) != 0) { + strcpy(allow, "prompt"); } fclose(fp); } else if ((fp = fopen(REQUESTOR_STORED_DEFAULT, "r"))) { LOGD("Using default file %s", REQUESTOR_STORED_DEFAULT); - char cmd[ARG_MAX]; - fgets(cmd, sizeof(cmd), fp); - int last = strlen(cmd) - 1; - if (last >= 0) - cmd[last] = 0; - - if (strcmp(cmd, "default") == 0) { - allow = fgetc(fp); - } + fgets(allow, sizeof(allow), fp); + last = strlen(allow) - 1; + if (last >=0) + allow[last] = 0; + fclose(fp); } - if (allow == '1') { + if (strcmp(allow, "allow") == 0) { return ALLOW; - } else if (allow == '0') { + } else if (strcmp(allow, "deny") == 0) { return DENY; } else { return INTERACTIVE; From 779707bcaacf766c198815ebe74b94c83b4117aa Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Mon, 1 Oct 2012 06:15:46 +0200 Subject: [PATCH 22/25] Fix for apps that use multiple commands not being remembered properly --- db.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/db.c b/db.c index cde757d..9ab54c8 100644 --- a/db.c +++ b/db.c @@ -32,21 +32,23 @@ int database_check(const struct su_context *ctx) if ((fp = fopen(filename, "r"))) { LOGD("Found file %s", filename); - fgets(allow, sizeof(allow), fp); - last = strlen(allow) - 1; - if (last >= 0) - allow[last] = 0; + while (fgets(allow, sizeof(allow), fp)) { + last = strlen(allow) - 1; + if (last >= 0) + allow[last] = 0; - char cmd[ARG_MAX]; - fgets(cmd, sizeof(cmd), fp); - /* skip trailing '\n' */ - last = strlen(cmd) - 1; - if (last >= 0) - cmd[last] = 0; + char cmd[ARG_MAX]; + fgets(cmd, sizeof(cmd), fp); + /* skip trailing '\n' */ + last = strlen(cmd) - 1; + if (last >= 0) + cmd[last] = 0; - LOGD("Comparing '%s' to '%s'", cmd, get_command(&ctx->to)); - if (strcmp(cmd, get_command(&ctx->to)) != 0) { - strcpy(allow, "prompt"); + LOGD("Comparing '%s' to '%s'", cmd, get_command(&ctx->to)); + if (strcmp(cmd, get_command(&ctx->to)) == 0) + break; + else + strcpy(allow, "prompt"); } fclose(fp); } else if ((fp = fopen(REQUESTOR_STORED_DEFAULT, "r"))) { From de0050de03d27112ffa476d175208c11532b4938 Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Sun, 18 Nov 2012 17:39:44 +0000 Subject: [PATCH 23/25] Fix for apps that use multiple commands --- activity.c | 9 +++++---- db.c | 6 +++++- su.h | 4 +++- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/activity.c b/activity.c index a515204..8062ebb 100644 --- a/activity.c +++ b/activity.c @@ -88,10 +88,11 @@ int send_intent(struct su_context *ctx, allow_t allow, const char *action) char command[ARG_MAX]; snprintf(command, sizeof(command), - "exec /system/bin/am broadcast -a %s --es socket '%s' " - "--ei caller_uid %d --ei allow %d " - "--ei version_code %d", - action, socket_path, uid, allow, VERSION_CODE); + "exec /system/bin/am broadcast --user %d -a %s --es socket '%s' " + "--ei caller_uid %d --ei allow %d --es desired_cmd '%s' " + "--ei all %d --ei version_code %d", + ctx->from.user, action, socket_path, uid, allow, get_command(&ctx->to), + ctx->to.all, VERSION_CODE); char *args[] = { "sh", "-c", command, NULL, }; /* diff --git a/db.c b/db.c index 9ab54c8..b42ce24 100644 --- a/db.c +++ b/db.c @@ -20,7 +20,7 @@ #include "su.h" -int database_check(const struct su_context *ctx) +int database_check(struct su_context *ctx) { FILE *fp; char filename[PATH_MAX]; @@ -47,6 +47,10 @@ int database_check(const struct su_context *ctx) LOGD("Comparing '%s' to '%s'", cmd, get_command(&ctx->to)); if (strcmp(cmd, get_command(&ctx->to)) == 0) break; + else if (strcmp(cmd, "any") == 0) { + ctx->to.all = 1; + break; + } else strcpy(allow, "prompt"); } diff --git a/su.h b/su.h index 586b48a..bdd4697 100644 --- a/su.h +++ b/su.h @@ -64,6 +64,8 @@ struct su_request { char **argv; int argc; int optind; + int appId; + int all; }; struct su_context { @@ -80,7 +82,7 @@ typedef enum { ALLOW = 1, } allow_t; -extern allow_t database_check(const struct su_context *ctx); +extern allow_t database_check(struct su_context *ctx); extern void set_identity(unsigned int uid); extern int send_intent(struct su_context *ctx, allow_t allow, const char *action); From 794fb0047ac06d4764f4cf16be0d03b406c354d3 Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Tue, 20 Nov 2012 20:43:49 +0000 Subject: [PATCH 24/25] Multi-user support with 3 modes --- activity.c | 2 +- db.c | 17 +++++++++++++---- su.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- su.h | 11 +++++++++++ 4 files changed, 69 insertions(+), 6 deletions(-) diff --git a/activity.c b/activity.c index 8062ebb..2bc9f1e 100644 --- a/activity.c +++ b/activity.c @@ -91,7 +91,7 @@ int send_intent(struct su_context *ctx, allow_t allow, const char *action) "exec /system/bin/am broadcast --user %d -a %s --es socket '%s' " "--ei caller_uid %d --ei allow %d --es desired_cmd '%s' " "--ei all %d --ei version_code %d", - ctx->from.user, action, socket_path, uid, allow, get_command(&ctx->to), + ctx->user.userid, action, socket_path, uid, allow, get_command(&ctx->to), ctx->to.all, VERSION_CODE); char *args[] = { "sh", "-c", command, NULL, }; diff --git a/db.c b/db.c index b42ce24..fad5371 100644 --- a/db.c +++ b/db.c @@ -26,9 +26,14 @@ int database_check(struct su_context *ctx) char filename[PATH_MAX]; char allow[7]; int last = 0; + int from_uid = ctx->from.uid; + + if (ctx->user.owner_mode) { + from_uid = from_uid % 100000; + } snprintf(filename, sizeof(filename), - REQUESTOR_STORED_PATH "/%u-%u", ctx->from.uid, ctx->to.uid); + "%s/%u-%u", ctx->user.store_path, from_uid, ctx->to.uid); if ((fp = fopen(filename, "r"))) { LOGD("Found file %s", filename); @@ -55,8 +60,8 @@ int database_check(struct su_context *ctx) strcpy(allow, "prompt"); } fclose(fp); - } else if ((fp = fopen(REQUESTOR_STORED_DEFAULT, "r"))) { - LOGD("Using default file %s", REQUESTOR_STORED_DEFAULT); + } else if ((fp = fopen(ctx->user.store_default, "r"))) { + LOGD("Using default file %s", ctx->user.store_default); fgets(allow, sizeof(allow), fp); last = strlen(allow) - 1; if (last >=0) @@ -70,6 +75,10 @@ int database_check(struct su_context *ctx) } else if (strcmp(allow, "deny") == 0) { return DENY; } else { - return INTERACTIVE; + if (ctx->user.userid != 0 && ctx->user.owner_mode) { + return DENY; + } else { + return INTERACTIVE; + } } } diff --git a/su.c b/su.c index d6a5e0f..06f4238 100644 --- a/su.c +++ b/su.c @@ -101,6 +101,35 @@ static int from_init(struct su_initiator *from) return 0; } +static void read_options(struct su_context *ctx) +{ + char mode[12]; + FILE *fp; + if ((fp = fopen(REQUESTOR_OPTIONS, "r"))) { + fgets(mode, sizeof(mode), fp); + if (strcmp(mode, "user\n") == 0) { + ctx->user.owner_mode = 0; + } else if (strcmp(mode, "owner\n") == 0) { + ctx->user.owner_mode = 1; + } + } +} + +static void user_init(struct su_context *ctx) +{ + if (ctx->from.uid > 99999) { + ctx->user.userid = ctx->from.uid / 100000; + if (!ctx->user.owner_mode) { + snprintf(ctx->user.data_path, PATH_MAX, "/data/user/%d/%s", + ctx->user.userid, REQUESTOR); + snprintf(ctx->user.store_path, PATH_MAX, "/data/user/%d/%s/files/stored", + ctx->user.userid, REQUESTOR); + snprintf(ctx->user.store_default, PATH_MAX, "/data/user/%d/%s/files/stored/default", + ctx->user.userid, REQUESTOR); + } + } +} + static void populate_environment(const struct su_context *ctx) { struct passwd *pw; @@ -452,6 +481,7 @@ int access_disabled(const struct su_initiator *from) "enable it under settings -> developer options"); return 1; } + } return 0; } @@ -475,6 +505,13 @@ int main(int argc, char *argv[]) .argc = argc, .optind = 0, }, + .user = { + .userid = 0, + .owner_mode = -1, + .data_path = REQUESTOR_DATA_PATH, + .store_path = REQUESTOR_STORED_PATH, + .store_default = REQUESTOR_STORED_DEFAULT, + }, .child = 0, }; struct stat st; @@ -554,6 +591,12 @@ int main(int argc, char *argv[]) if (from_init(&ctx.from) < 0) { deny(&ctx); } + + read_options(&ctx); + user_init(&ctx); + + if (ctx.user.owner_mode == -1 && ctx.user.userid != 0) + deny(&ctx); if (access_disabled(&ctx.from)) deny(&ctx); @@ -568,7 +611,7 @@ int main(int argc, char *argv[]) if (ctx.from.uid == AID_ROOT || ctx.from.uid == AID_SHELL) allow(&ctx); - if (stat(REQUESTOR_DATA_PATH, &st) < 0) { + if (stat(ctx.user.data_path, &st) < 0) { PLOGE("stat"); deny(&ctx); } diff --git a/su.h b/su.h index bdd4697..77e2233 100644 --- a/su.h +++ b/su.h @@ -29,6 +29,7 @@ #define REQUESTOR_STORED_PATH REQUESTOR_DATA_PATH "/files/stored" #define REQUESTOR_STORED_DEFAULT REQUESTOR_STORED_PATH "/default" +#define REQUESTOR_OPTIONS REQUESTOR_STORED_PATH "/options" /* intent actions */ #define ACTION_REQUEST REQUESTOR ".REQUEST" @@ -51,6 +52,7 @@ struct su_initiator { pid_t pid; unsigned uid; + unsigned user; char bin[PATH_MAX]; char args[4096]; }; @@ -68,9 +70,18 @@ struct su_request { int all; }; +struct su_user_info { + unsigned userid; + int owner_mode; + char data_path[PATH_MAX]; + char store_path[PATH_MAX]; + char store_default[PATH_MAX]; +}; + struct su_context { struct su_initiator from; struct su_request to; + struct su_user_info user; mode_t umask; volatile pid_t child; char sock_path[PATH_MAX]; From 95e81e55591417b9579e1c0373a22d4972b821f1 Mon Sep 17 00:00:00 2001 From: Adam Shanks Date: Tue, 20 Nov 2012 20:53:13 +0000 Subject: [PATCH 25/25] Update version --- su.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/su.h b/su.h index 77e2233..d0aa786 100644 --- a/su.h +++ b/su.h @@ -43,8 +43,8 @@ #define VERSION_EXTRA "" #endif -#define VERSION "3.2" VERSION_EXTRA -#define VERSION_CODE 18 +#define VERSION "3.3" VERSION_EXTRA +#define VERSION_CODE 19 #define DATABASE_VERSION 6 #define PROTO_VERSION 0