Fix permissions problems of incidentd.

Test: manual
Change-Id: I4ee0d1f2349ee1a25a422cabf1b5b87c612710d2

12 files changed, 74 insertions, 57 deletions

diff --git a/cmds/incident_helper/src/TextParserBase.h b/cmds/incident_helper/src/TextParserBase.h
index c41612de4eb3..166796673e25 100644
--- a/cmds/incident_helper/src/TextParserBase.h
+++ b/cmds/incident_helper/src/TextParserBase.h
@@ -68,4 +68,4 @@ public:
     virtual status_t Parse(const int in, const int out) const;
 };
 
-#endif // TEXT_PARSER_BASE_H
\ No newline at end of file
+#endif // TEXT_PARSER_BASE_H
diff --git a/cmds/incidentd/README.md b/cmds/incidentd/README.md
index ad0fa08c7326..71c6deb18aac 100644
--- a/cmds/incidentd/README.md
+++ b/cmds/incidentd/README.md
@@ -12,8 +12,8 @@ Run the test on a device manually
 
 ```
 root$ mmm -j frameworks/base/cmds/incidentd && \
-adb push $OUT/data/nativetest/incidentd_test/* /data/nativetest/incidentd_test/ && \
-adb shell /data/nativetest/incidentd_test/incidentd_test 2>/dev/null
+adb push $OUT/data/nativetest/incidentd_test/* /data/nativetest/ && \
+adb shell /data/nativetest/incidentd_test 2>/dev/null
 ```
 
 Run the test via AndroidTest.xml
diff --git a/cmds/incidentd/incidentd.rc b/cmds/incidentd/incidentd.rc
index 66667dca2982..1bd146850ea9 100644
--- a/cmds/incidentd/incidentd.rc
+++ b/cmds/incidentd/incidentd.rc
@@ -19,4 +19,4 @@ service incidentd /system/bin/incidentd
 
 on post-fs-data
     # Create directory for incidentd
-    mkdir /data/misc/incidents 0770 root root
+    mkdir /data/misc/incidents 0770 incidentd incidentd
diff --git a/cmds/incidentd/src/FdBuffer.cpp b/cmds/incidentd/src/FdBuffer.cpp
index 30dd339a629b..0fff4e6dc4a0 100644
--- a/cmds/incidentd/src/FdBuffer.cpp
+++ b/cmds/incidentd/src/FdBuffer.cpp
@@ -63,12 +63,14 @@ FdBuffer::read(int fd, int64_t timeout)
 
         int64_t remainingTime = (mStartTime + timeout) - uptimeMillis();
         if (remainingTime <= 0) {
+            if (DEBUG) ALOGD("timed out due to long read");
             mTimedOut = true;
             break;
         }
 
         int count = poll(&pfds, 1, remainingTime);
         if (count == 0) {
+            if (DEBUG) ALOGD("timed out due to block calling poll");
             mTimedOut = true;
             break;
         } else if (count < 0) {
@@ -129,6 +131,7 @@ FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64_t timeou
 
         int64_t remainingTime = (mStartTime + timeoutMs) - uptimeMillis();
         if (remainingTime <= 0) {
+            if (DEBUG) ALOGD("timed out due to long read");
             mTimedOut = true;
             break;
         }
@@ -136,6 +139,7 @@ FdBuffer::readProcessedDataInStream(int fd, int toFd, int fromFd, int64_t timeou
         // wait for any pfds to be ready to perform IO
         int count = poll(pfds, 3, remainingTime);
         if (count == 0) {
+            if (DEBUG) ALOGD("timed out due to block calling poll");
             mTimedOut = true;
             break;
         } else if (count < 0) {
diff --git a/cmds/incidentd/src/IncidentService.cpp b/cmds/incidentd/src/IncidentService.cpp
index c4b54bbbc022..a97eb861578e 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -43,8 +43,9 @@ String16 const DUMP_PERMISSION("android.permission.DUMP");
 String16 const USAGE_STATS_PERMISSION("android.permission.PACKAGE_USAGE_STATS");
 
 static Status
-checkIncidentPermissions()
+checkIncidentPermissions(const IncidentReportArgs& args)
 {
+    // checking calling permission.
     if (!checkCallingPermission(DUMP_PERMISSION)) {
         ALOGW("Calling pid %d and uid %d does not have permission: android.permission.DUMP",
                 IPCThreadState::self()->getCallingPid(), IPCThreadState::self()->getCallingUid());
@@ -57,10 +58,24 @@ checkIncidentPermissions()
         return Status::fromExceptionCode(Status::EX_SECURITY,
                 "Calling process does not have permission: android.permission.USAGE_STATS");
     }
+
+    // checking calling request uid permission.
+    uid_t callingUid = IPCThreadState::self()->getCallingUid();
+    switch (args.dest()) {
+        case DEST_LOCAL:
+            if (callingUid != AID_SHELL || callingUid != AID_ROOT) {
+                return Status::fromExceptionCode(Status::EX_SECURITY,
+                    "Calling process does not have permission to get local data.");
+            }
+        case DEST_EXPLICIT:
+            if (callingUid != AID_SHELL || callingUid != AID_ROOT ||
+                callingUid != AID_STATSD || callingUid != AID_SYSTEM) {
+                return Status::fromExceptionCode(Status::EX_SECURITY,
+                    "Calling process does not have permission to get explicit data.");
+            }
+    }
     return Status::ok();
 }
-
-
 // ================================================================================
 ReportRequestQueue::ReportRequestQueue()
 {
@@ -71,7 +86,7 @@ ReportRequestQueue::~ReportRequestQueue()
 }
 
 void
-ReportRequestQueue::addRequest(const sp<ReportRequest>& request) 
+ReportRequestQueue::addRequest(const sp<ReportRequest>& request)
 {
     unique_lock<mutex> lock(mLock);
     mQueue.push_back(request);
@@ -196,7 +211,7 @@ IncidentService::reportIncident(const IncidentReportArgs& args)
 {
     ALOGI("reportIncident");
 
-    Status status = checkIncidentPermissions();
+    Status status = checkIncidentPermissions(args);
     if (!status.isOk()) {
         return status;
     }
@@ -212,7 +227,7 @@ IncidentService::reportIncidentToStream(const IncidentReportArgs& args,
 {
     ALOGI("reportIncidentToStream");
 
-    Status status = checkIncidentPermissions();
+    Status status = checkIncidentPermissions(args);
     if (!status.isOk()) {
         return status;
     }
diff --git a/cmds/incidentd/src/Reporter.cpp b/cmds/incidentd/src/Reporter.cpp
index 34930aa57321..bd559d6980f1 100644
--- a/cmds/incidentd/src/Reporter.cpp
+++ b/cmds/incidentd/src/Reporter.cpp
@@ -251,7 +251,7 @@ Reporter::create_file(int* fd)
     // Override umask. Not super critical. If it fails go on with life.
     chmod(filename, 0660);
 
-    if (chown(filename, AID_SYSTEM, AID_SYSTEM)) {
+    if (chown(filename, AID_INCIDENTD, AID_INCIDENTD)) {
         ALOGE("Unable to change ownership of incident file %s: %s\n", filename, strerror(errno));
         status_t err = -errno;
         unlink(mFilename.c_str());
diff --git a/cmds/incidentd/src/Section.cpp b/cmds/incidentd/src/Section.cpp
index 61d16f815e65..0827785811b6 100644
--- a/cmds/incidentd/src/Section.cpp
+++ b/cmds/incidentd/src/Section.cpp
@@ -19,6 +19,7 @@
 #include "Section.h"
 
 #include <errno.h>
+#include <sys/prctl.h>
 #include <unistd.h>
 #include <wait.h>
 
@@ -30,7 +31,6 @@
 #include <log/log_event_list.h>
 #include <log/logprint.h>
 #include <log/log_read.h>
-#include <private/android_filesystem_config.h> // for AID_NOBODY
 #include <private/android_logger.h>
 
 #include "FdBuffer.h"
@@ -55,26 +55,20 @@ static pid_t
 fork_execute_incident_helper(const int id, const char* name, Fpipe& p2cPipe, Fpipe& c2pPipe)
 {
     const char* ihArgs[] { INCIDENT_HELPER, "-s", String8::format("%d", id).string(), NULL };
-
     // fork used in multithreaded environment, avoid adding unnecessary code in child process
     pid_t pid = fork();
     if (pid == 0) {
-        // child process executes incident helper as nobody
-        if (setgid(AID_NOBODY) == -1) {
-            ALOGW("%s can't change gid: %s", name, strerror(errno));
-            _exit(EXIT_FAILURE);
-        }
-        if (setuid(AID_NOBODY) == -1) {
-            ALOGW("%s can't change uid: %s", name, strerror(errno));
-            _exit(EXIT_FAILURE);
-        }
-
-        if (dup2(p2cPipe.readFd(),  STDIN_FILENO)  != 0 || !p2cPipe.close() ||
-            dup2(c2pPipe.writeFd(), STDOUT_FILENO) != 1 || !c2pPipe.close()) {
+        if (TEMP_FAILURE_RETRY(dup2(p2cPipe.readFd(),  STDIN_FILENO))  != 0
+            || !p2cPipe.close()
+            || TEMP_FAILURE_RETRY(dup2(c2pPipe.writeFd(), STDOUT_FILENO)) != 1
+            || !c2pPipe.close()) {
             ALOGW("%s can't setup stdin and stdout for incident helper", name);
             _exit(EXIT_FAILURE);
         }
 
+        /* make sure the child dies when incidentd dies */
+        prctl(PR_SET_PDEATHSIG, SIGKILL);
+
         execv(INCIDENT_HELPER, const_cast<char**>(ihArgs));
 
         ALOGW("%s failed in incident helper process: %s", name, strerror(errno));
@@ -87,11 +81,23 @@ fork_execute_incident_helper(const int id, const char* name, Fpipe& p2cPipe, Fpi
 }
 
 // ================================================================================
+static status_t statusCode(int status) {
+    if (WIFSIGNALED(status)) {
+      ALOGD("return by signal: %s", strerror(WTERMSIG(status)));
+      return -WTERMSIG(status);
+    } else if (WIFEXITED(status) && WEXITSTATUS(status) > 0) {
+      ALOGD("return by exit: %s", strerror(WEXITSTATUS(status)));
+      return -WEXITSTATUS(status);
+    }
+    return NO_ERROR;
+}
+
 static status_t kill_child(pid_t pid) {
     int status;
+    ALOGD("try to kill child process %d", pid);
     kill(pid, SIGKILL);
     if (waitpid(pid, &status, 0) == -1) return -1;
-    return WIFEXITED(status) == 0 ? NO_ERROR : -WEXITSTATUS(status);
+    return statusCode(status);
 }
 
 static status_t wait_child(pid_t pid) {
@@ -104,7 +110,7 @@ static status_t wait_child(pid_t pid) {
         nanosleep(&WAIT_INTERVAL_NS, NULL);
     }
     if (!died) return kill_child(pid);
-    return WIFEXITED(status) == 0 ? NO_ERROR : -WEXITSTATUS(status);
+    return statusCode(status);
 }
 // ================================================================================
 static const Privacy*
@@ -275,9 +281,9 @@ FileSection::Execute(ReportRequestSet* requests) const
     status_t readStatus = buffer.readProcessedDataInStream(fd, p2cPipe.writeFd(), c2pPipe.readFd(),
             this->timeoutMs, mIsSysfs);
     if (readStatus != NO_ERROR || buffer.timedOut()) {
-        ALOGW("FileSection '%s' failed to read data from incident helper: %s, timedout: %s, kill: %s",
-            this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false",
-            strerror(-kill_child(pid)));
+        ALOGW("FileSection '%s' failed to read data from incident helper: %s, timedout: %s",
+            this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false");
+        kill_child(pid);
         return readStatus;
     }
 
@@ -543,10 +549,10 @@ CommandSection::Execute(ReportRequestSet* requests) const
     close(cmdPipe.writeFd());
     status_t readStatus = buffer.read(ihPipe.readFd(), this->timeoutMs);
     if (readStatus != NO_ERROR || buffer.timedOut()) {
-        ALOGW("CommandSection '%s' failed to read data from incident helper: %s, "
-            "timedout: %s, kill command: %s, kill incident helper: %s",
-            this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false",
-            strerror(-kill_child(cmdPid)), strerror(-kill_child(ihPid)));
+        ALOGW("CommandSection '%s' failed to read data from incident helper: %s, timedout: %s",
+            this->name.string(), strerror(-readStatus), buffer.timedOut() ? "true" : "false");
+        kill_child(cmdPid);
+        kill_child(ihPid);
         return readStatus;
     }
 
diff --git a/cmds/incidentd/src/io_util.cpp b/cmds/incidentd/src/io_util.cpp
index af4a35cc0015..90f543e30ff7 100644
--- a/cmds/incidentd/src/io_util.cpp
+++ b/cmds/incidentd/src/io_util.cpp
@@ -23,7 +23,7 @@
 status_t write_all(int fd, uint8_t const* buf, size_t size)
 {
     while (size > 0) {
-        ssize_t amt = ::write(fd, buf, size);
+        ssize_t amt = TEMP_FAILURE_RETRY(::write(fd, buf, size));
         if (amt < 0) {
             return -errno;
         }
diff --git a/cmds/incidentd/src/report_directory.cpp b/cmds/incidentd/src/report_directory.cpp
index 65030b3a1799..20111d8ae89a 100644
--- a/cmds/incidentd/src/report_directory.cpp
+++ b/cmds/incidentd/src/report_directory.cpp
@@ -58,26 +58,9 @@ create_directory(const char* directory)
                 goto done;
             }
         } else {
-            if (mkdir(dir, 0770)) {
-                ALOGE("No incident reports today. "
-                        "Unable to create incident report dir %s: %s", dir,
-                        strerror(errno));
-                err = -errno;
-                goto done;
-            }
-            if (chmod(dir, 0770)) {
-                ALOGE("No incident reports today. "
-                        "Unable to set permissions for incident report dir %s: %s", dir,
-                        strerror(errno));
-                err = -errno;
-                goto done;
-            }
-            if (chown(dir, AID_SYSTEM, AID_SYSTEM)) {
-                ALOGE("No incident reports today. Unable to change ownership of dir %s: %s\n",
-                        dir, strerror(errno));
-                err = -errno;
-                goto done;
-            }
+            ALOGE("No such directory %s, something wrong.", dir);
+            err = -1;
+            goto done;
         }
         if (!last) {
             *d++ = '/';
@@ -97,8 +80,7 @@ create_directory(const char* directory)
         err = BAD_VALUE;
         goto done;
     }
-    if ((st.st_uid != AID_SYSTEM && st.st_uid != AID_ROOT) ||
-        (st.st_gid != AID_SYSTEM && st.st_gid != AID_ROOT)) {
+    if (st.st_uid != AID_INCIDENTD || st.st_gid != AID_INCIDENTD) {
         ALOGE("No incident reports today. Owner is %d and group is %d on report directory %s",
                 st.st_uid, st.st_gid, directory);
         err = BAD_VALUE;
diff --git a/core/java/android/os/Process.java b/core/java/android/os/Process.java
index 0874d93e8262..3c782fbacb02 100644
--- a/core/java/android/os/Process.java
+++ b/core/java/android/os/Process.java
@@ -151,6 +151,12 @@ public class Process {
      */
     public static final int OTA_UPDATE_UID = 1061;
 
+    /**
+     * Defines the UID used for incidentd.
+     * @hide
+     */
+    public static final int INCIDENTD_UID = 1067;
+
     /** {@hide} */
     public static final int NOBODY_UID = 9999;
 
diff --git a/core/java/com/android/internal/util/DumpUtils.java b/core/java/com/android/internal/util/DumpUtils.java
index 66b777e8e8e6..2b5103377ecb 100644
--- a/core/java/com/android/internal/util/DumpUtils.java
+++ b/core/java/com/android/internal/util/DumpUtils.java
@@ -102,6 +102,7 @@ public final class DumpUtils {
             case android.os.Process.ROOT_UID:
             case android.os.Process.SYSTEM_UID:
             case android.os.Process.SHELL_UID:
+            case android.os.Process.INCIDENTD_UID:
                 return true;
         }
 
diff --git a/data/etc/platform.xml b/data/etc/platform.xml
index f169f225e279..622fbf643a56 100644
--- a/data/etc/platform.xml
+++ b/data/etc/platform.xml
@@ -165,6 +165,9 @@
 
     <assign-permission name="android.permission.ACCESS_SURFACE_FLINGER" uid="graphics" />
 
+    <assign-permission name="android.permission.DUMP" uid="incidentd" />
+    <assign-permission name="android.permission.PACKAGE_USAGE_STATS" uid="incidentd" />
+
     <assign-permission name="android.permission.ACCESS_LOWPAN_STATE" uid="lowpan" />
     <assign-permission name="android.permission.MANAGE_LOWPAN_INTERFACES" uid="lowpan" />