Fix permissions problems of incidentd.
Test: manual
Change-Id: I4ee0d1f2349ee1a25a422cabf1b5b87c612710d2
diff --git a/cmds/incident_helper/src/TextParserBase.h b/cmds/incident_helper/src/TextParserBase.h
index c41612d..1667966 100644
--- a/cmds/incident_helper/src/TextParserBase.h
+++ b/cmds/incident_helper/src/TextParserBase.h
@@ -68,4 +68,4 @@
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 ad0fa08..71c6deb 100644
--- a/cmds/incidentd/README.md
+++ b/cmds/incidentd/README.md
@@ -12,8 +12,8 @@
```
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 66667dc..1bd1468 100644
--- a/cmds/incidentd/incidentd.rc
+++ b/cmds/incidentd/incidentd.rc
@@ -19,4 +19,4 @@
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 30dd339..0fff4e6 100644
--- a/cmds/incidentd/src/FdBuffer.cpp
+++ b/cmds/incidentd/src/FdBuffer.cpp
@@ -63,12 +63,14 @@
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 @@
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 @@
// 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 c4b54bb..a97eb86 100644
--- a/cmds/incidentd/src/IncidentService.cpp
+++ b/cmds/incidentd/src/IncidentService.cpp
@@ -43,8 +43,9 @@
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 @@
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 @@
}
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 @@
{
ALOGI("reportIncident");
- Status status = checkIncidentPermissions();
+ Status status = checkIncidentPermissions(args);
if (!status.isOk()) {
return status;
}
@@ -212,7 +227,7 @@
{
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 34930aa..bd559d6 100644
--- a/cmds/incidentd/src/Reporter.cpp
+++ b/cmds/incidentd/src/Reporter.cpp
@@ -251,7 +251,7 @@
// 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 61d16f8..0827785 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 @@
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 @@
}
// ================================================================================
+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 @@
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 @@
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 @@
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 af4a35c..90f543e 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 65030b3..20111d8 100644
--- a/cmds/incidentd/src/report_directory.cpp
+++ b/cmds/incidentd/src/report_directory.cpp
@@ -58,26 +58,9 @@
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 @@
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 0874d93..3c782fb 100644
--- a/core/java/android/os/Process.java
+++ b/core/java/android/os/Process.java
@@ -151,6 +151,12 @@
*/
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 66b777e..2b51033 100644
--- a/core/java/com/android/internal/util/DumpUtils.java
+++ b/core/java/com/android/internal/util/DumpUtils.java
@@ -102,6 +102,7 @@
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 f169f22..622fbf6 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" />
