logd: refine permissions to access /data/system/packages.list
Refine DAC security surrounding logd.daemon worker thread and add a
positive test for logd failure to access /data/system/packages.list.
- Add AID_PACKAGE_INFO to groups of worker thread.
- Move AID_SYSTEM to groups, setgid to AID_LOGD.
- Do not drop capabilities until after setting the uid and gids.
- Add a test that is part of logd.statistics test to check when
packagelistparser appears broken.
- If /data/system/packages.list is encrypted, ensure we do not pick
up the existing inode to ensure strong positive when finding access
problems.
- Replace all occurrences of NULL with nullptr in gTest code for
compliance with best practices.
Test: gTest logd-unit-tests --gtest_filter=logd.statistics
(expect consistent failure, later CLs fix)
Bug: 37751120
Bug: 36645158
Change-Id: I01b26fe5e25203246ae432d272c8daa9c07cab54
diff --git a/logd/logd.rc b/logd/logd.rc
index ee89b83..7494d8f 100644
--- a/logd/logd.rc
+++ b/logd/logd.rc
@@ -5,7 +5,7 @@
file /proc/kmsg r
file /dev/kmsg w
user logd
- group logd system readproc
+ group logd system package_info readproc
writepid /dev/cpuset/system-background/tasks
service logd-reinit /system/bin/logd --reinit
diff --git a/logd/main.cpp b/logd/main.cpp
index 18029eb..c8183f0 100644
--- a/logd/main.cpp
+++ b/logd/main.cpp
@@ -240,23 +240,36 @@
set_sched_policy(0, SP_BACKGROUND);
setpriority(PRIO_PROCESS, 0, ANDROID_PRIORITY_BACKGROUND);
+ // We should drop to AID_LOGD, if we are anything else, we have
+ // even lesser privileges and accept our fate.
+ gid_t groups[] = {
+ AID_SYSTEM, // search access to /data/system path
+ AID_PACKAGE_INFO, // readonly access to /data/system/packages.list
+ };
+ if (setgroups(arraysize(groups), groups) == -1) {
+ android::prdebug(
+ "logd.daemon: failed to set AID_SYSTEM AID_PACKAGE_INFO groups");
+ }
+ if (setgid(AID_LOGD) != 0) {
+ android::prdebug("logd.daemon: failed to set AID_LOGD gid");
+ }
+ if (setuid(AID_LOGD) != 0) {
+ android::prdebug("logd.daemon: failed to set AID_LOGD uid");
+ }
+
cap_t caps = cap_init();
(void)cap_clear(caps);
(void)cap_set_proc(caps);
(void)cap_free(caps);
- // If we are AID_ROOT, we should drop to AID_LOGD+AID_SYSTEM, if we are
- // anything else, we have even lesser privileges and accept our fate. Not
- // worth checking for error returns setting this thread's privileges.
- (void)setgid(AID_SYSTEM); // readonly access to /data/system/packages.list
- (void)setuid(AID_LOGD); // access to everything logd, eg /data/misc/logd
-
while (reinit_running && !sem_wait(&reinit) && reinit_running) {
// uidToName Privileged Worker
if (uid) {
name = nullptr;
- packagelist_parse(package_list_parser_cb, nullptr);
+ // if we got the perms wrong above, this would spam if we reported
+ // problems with acquisition of an uid name from the packages.
+ (void)packagelist_parse(package_list_parser_cb, nullptr);
uid = 0;
sem_post(&uidName);
diff --git a/logd/tests/logd_test.cpp b/logd/tests/logd_test.cpp
index bd9fa93..f90c9fb 100644
--- a/logd/tests/logd_test.cpp
+++ b/logd/tests/logd_test.cpp
@@ -14,6 +14,7 @@
* limitations under the License.
*/
+#include <ctype.h>
#include <fcntl.h>
#include <inttypes.h>
#include <poll.h>
@@ -26,6 +27,7 @@
#include <string>
+#include <android-base/file.h>
#include <android-base/macros.h>
#include <android-base/stringprintf.h>
#include <cutils/sockets.h>
@@ -85,7 +87,7 @@
size_t ret = atol(buf) + 1;
if (ret < 4) {
delete[] buf;
- buf = NULL;
+ buf = nullptr;
break;
}
bool check = ret <= len;
@@ -107,7 +109,7 @@
// UID PACKAGE BYTES LINES
// 0 root 54164 147569
//
- char* benchmark = NULL;
+ char* benchmark = nullptr;
do {
static const char signature[] = "\n0 root ";
@@ -122,7 +124,7 @@
benchmark = cp;
#ifdef DEBUG
char* end = strstr(benchmark, "\n");
- if (end == NULL) {
+ if (end == nullptr) {
end = benchmark + strlen(benchmark);
}
fprintf(stderr, "parse for spam counter in \"%.*s\"\n",
@@ -154,7 +156,7 @@
if (value > 10UL) {
break;
}
- benchmark = NULL;
+ benchmark = nullptr;
} while (*cp);
return benchmark;
}
@@ -165,9 +167,14 @@
size_t len;
char* buf;
+ // Drop cache so that any access problems can be discovered.
+ if (!android::base::WriteStringToFile("3\n", "/proc/sys/vm/drop_caches")) {
+ GTEST_LOG_(INFO) << "Could not open trigger dropping inode cache";
+ }
+
alloc_statistics(&buf, &len);
- ASSERT_TRUE(NULL != buf);
+ ASSERT_TRUE(nullptr != buf);
// remove trailing FF
char* cp = buf + len - 1;
@@ -192,19 +199,38 @@
EXPECT_EQ(0, truncated);
char* main_logs = strstr(cp, "\nChattiest UIDs in main ");
- EXPECT_TRUE(NULL != main_logs);
+ EXPECT_TRUE(nullptr != main_logs);
char* radio_logs = strstr(cp, "\nChattiest UIDs in radio ");
if (!radio_logs)
- GTEST_LOG_(INFO) << "Value of: NULL != radio_logs\n"
+ GTEST_LOG_(INFO) << "Value of: nullptr != radio_logs\n"
"Actual: false\n"
"Expected: false\n";
char* system_logs = strstr(cp, "\nChattiest UIDs in system ");
- EXPECT_TRUE(NULL != system_logs);
+ EXPECT_TRUE(nullptr != system_logs);
char* events_logs = strstr(cp, "\nChattiest UIDs in events ");
- EXPECT_TRUE(NULL != events_logs);
+ EXPECT_TRUE(nullptr != events_logs);
+
+ // Check if there is any " u0_a#### " as this means packagelistparser broken
+ char* used_getpwuid = nullptr;
+ int used_getpwuid_len;
+ char* uid_name = cp;
+ static const char getpwuid_prefix[] = " u0_a";
+ while ((uid_name = strstr(uid_name, getpwuid_prefix)) != nullptr) {
+ used_getpwuid = uid_name + 1;
+ uid_name += strlen(getpwuid_prefix);
+ while (isdigit(*uid_name)) ++uid_name;
+ used_getpwuid_len = uid_name - used_getpwuid;
+ if (isspace(*uid_name)) break;
+ used_getpwuid = nullptr;
+ }
+ EXPECT_TRUE(nullptr == used_getpwuid);
+ if (used_getpwuid) {
+ fprintf(stderr, "libpackagelistparser failed to pick up %.*s\n",
+ used_getpwuid_len, used_getpwuid);
+ }
delete[] buf;
#else
@@ -352,7 +378,7 @@
}
alarm(old_alarm);
- sigaction(SIGALRM, &old_sigaction, NULL);
+ sigaction(SIGALRM, &old_sigaction, nullptr);
close(fd);
}
@@ -434,7 +460,7 @@
// Introduce some extreme spam for the worst UID filter
ASSERT_TRUE(
- NULL !=
+ nullptr !=
(fp = popen("/data/nativetest/liblog-benchmarks/liblog-benchmarks"
" BM_log_maximum_retry"
" BM_log_maximum"
@@ -505,10 +531,10 @@
bool collected_statistics = !!buf;
EXPECT_EQ(true, collected_statistics);
- ASSERT_TRUE(NULL != buf);
+ ASSERT_TRUE(nullptr != buf);
char* benchmark_statistics_found = find_benchmark_spam(buf);
- ASSERT_TRUE(benchmark_statistics_found != NULL);
+ ASSERT_TRUE(benchmark_statistics_found != nullptr);
// Check how effective the SPAM filter is, parse out Now size.
// 0 root 54164 147569
@@ -603,7 +629,7 @@
written = write(fd, ask.c_str(), len) == (ssize_t)len;
if (!written) {
alarm(old_alarm);
- sigaction(SIGALRM, &old_sigaction, NULL);
+ sigaction(SIGALRM, &old_sigaction, nullptr);
close(fd);
continue;
}
@@ -625,7 +651,7 @@
: (old_alarm > (1 + 3 - alarm_wrap))
? old_alarm - 3 + alarm_wrap
: 2);
- sigaction(SIGALRM, &old_sigaction, NULL);
+ sigaction(SIGALRM, &old_sigaction, nullptr);
close(fd);
@@ -648,7 +674,7 @@
EXPECT_TRUE(content_timeout);
EXPECT_NE(0U, alarm_timeout);
#else
- command = NULL;
+ command = nullptr;
GTEST_LOG_(INFO) << "This test does nothing.\n";
#endif
}
@@ -713,7 +739,7 @@
written = write(fd, ask.c_str(), len) == (ssize_t)len;
if (!written) {
alarm(old_alarm);
- sigaction(SIGALRM, &old_sigaction, NULL);
+ sigaction(SIGALRM, &old_sigaction, nullptr);
close(fd);
continue;
}
@@ -735,7 +761,7 @@
: (old_alarm > (1 + 3 - alarm_wrap))
? old_alarm - 3 + alarm_wrap
: 2);
- sigaction(SIGALRM, &old_sigaction, NULL);
+ sigaction(SIGALRM, &old_sigaction, nullptr);
close(fd);
@@ -837,7 +863,7 @@
int save_errno = (recv_ret < 0) ? errno : 0;
EXPECT_NE(0U, alarm(old_alarm));
- sigaction(SIGALRM, &old_sigaction, NULL);
+ sigaction(SIGALRM, &old_sigaction, nullptr);
EXPECT_EQ(0, recv_ret);
if (recv_ret > 0) {
@@ -876,8 +902,8 @@
char* cp;
long ret = strtol(buffer, &cp, 10);
EXPECT_GT(ret, 16);
- EXPECT_TRUE(strstr(buffer, "\t(to life the universe etc|3)") != NULL);
- EXPECT_TRUE(strstr(buffer, "answer") != NULL);
+ EXPECT_TRUE(strstr(buffer, "\t(to life the universe etc|3)") != nullptr);
+ EXPECT_TRUE(strstr(buffer, "answer") != nullptr);
#else
GTEST_LOG_(INFO) << "This test does nothing.\n";
#endif
@@ -897,8 +923,8 @@
char* cp;
long ret = strtol(buffer, &cp, 10);
EXPECT_GT(ret, 16);
- EXPECT_TRUE(strstr(buffer, "\t(new|1)") != NULL);
- EXPECT_TRUE(strstr(buffer, name) != NULL);
+ EXPECT_TRUE(strstr(buffer, "\t(new|1)") != nullptr);
+ EXPECT_TRUE(strstr(buffer, name) != nullptr);
// ToDo: also look for this in /data/misc/logd/event-log-tags and
// /dev/event-log-tags.
#else
@@ -942,7 +968,7 @@
ASSERT_EQ(0, info.si_status);
struct logger_list* logger_list;
- ASSERT_TRUE(NULL !=
+ ASSERT_TRUE(nullptr !=
(logger_list = android_logger_list_open(
LOG_ID_EVENTS, ANDROID_LOG_RDONLY | ANDROID_LOG_NONBLOCK,
0, pid)));