Fix security, tighter str comparison, fix warnings

 * When reading our and the caller's path, we were mistakingly reading only the
   first sizeof(char*) characters, so anything that was running from a path
   starting with '/sys' was being considered to be us
 * strcmp'ing strings instead of comparing only for the length of the first of
   them. Avoids possible executions from /system/xbin/suwhatever .
 * Fixed several compile-time warnings

Change-Id: I2af07f7bf5078acd0c36bd22b74fbc01276957be

diff --git a/Superuser/jni/su/daemon.c b/Superuser/jni/su/daemon.c
index c7822ab..c219018 100644 (file)
--- a/Superuser/jni/su/daemon.c
+++ b/Superuser/jni/su/daemon.c
@@ -280,7 +280,7 @@ static int pid_to_exe(int pid, char *exe) {
     int len;
 
     snprintf(path, sizeof(path), "/proc/%u/exe", pid);
-    len = readlink(path, exe, sizeof(exe));
+    len = readlink(path, exe, sizeof(path));
     if (len < 0) {
         PLOGE("Getting exe path");
         return -1;
@@ -311,10 +311,9 @@ static int daemon_accept(int fd) {
         exit(-1);
     }
 
-    if (!pid_to_exe(getpid(),&mypath) &&
-            !pid_to_exe(credentials.pid,&remotepath)) {
-        if (!strncmp(mypath,remotepath,strlen(mypath)))
-            caller_is_self = 1;
+    if (!pid_to_exe(getpid(),mypath) &&
+            !pid_to_exe(credentials.pid,remotepath)) {
+        if (!strncmp(mypath,remotepath,PATH_MAX)) caller_is_self = 1;
     }
     // if the credentials on the other side imply that
     // we're not calling ourselves, we can't trust anything being sent.
@@ -553,7 +552,7 @@ static void setup_sighandlers(void) {
 
 int connect_daemon(int argc, char *argv[], int ppid) {
     int uid = getuid();
-    int ptmx;
+    int ptmx = -1;
     char pts_slave[PATH_MAX];
 
     struct sockaddr_un sun;

diff --git a/Superuser/jni/su/pts.c b/Superuser/jni/su/pts.c
index c810a34..8e96541 100644 (file)
--- a/Superuser/jni/su/pts.c
+++ b/Superuser/jni/su/pts.c
@@ -43,7 +43,7 @@ static int write_blocking(int fd, char *buf, size_t bufsz) {
         ret = write(fd, buf + written, bufsz - written);
         if (ret == -1) return -1;
         written += ret;
-    } while (written < bufsz);
+    } while (written < (ssize_t)bufsz);
 
     return 0;
 }
@@ -106,15 +106,14 @@ static void pump_async(int input, int output) {
  */
 int pts_open(char *slave_name, size_t slave_name_size) {
     int fdm;
-    char *sn_tmp;
+    char sn_tmp[slave_name_size];
 
     // Open master ptmx device
     fdm = open("/dev/ptmx", O_RDWR);
     if (fdm == -1) return -1;
 
     // Get the slave name
-    sn_tmp = ptsname(fdm);
-    if (!sn_tmp) {
+    if (ptsname_r(fdm, sn_tmp, slave_name_size) != 0) {
         close(fdm);
         return -2;
     }
@@ -203,7 +202,7 @@ int restore_stdin(void) {
 }
 
 // Flag indicating whether the sigwinch watcher should terminate.
-volatile static int closing_time = 0;
+static volatile int closing_time = 0;
 
 /**
  * Thread process. Wait for a SIGWINCH to be received, then update 

diff --git a/Superuser/jni/su/su.c b/Superuser/jni/su/su.c
index 0a33b55..08d84b9 100644 (file)
--- a/Superuser/jni/su/su.c
+++ b/Superuser/jni/su/su.c
@@ -76,7 +76,7 @@ int fork_zero_fucks() {
         return pid;
     }
     else {
-        if (pid = fork())
+        if ((pid = fork()))
             exit(0);
         return 0;
     }