From b825f1148cc78fa853da964de2e7e2de1b3b03b2 Mon Sep 17 00:00:00 2001 From: Connor O'Brien Date: Fri, 12 Aug 2016 11:52:46 -0700 Subject: Fix vold vulnerability in FrameworkListener Modify FrameworkListener to ignore commands that exceed the maximum buffer length and send an error message. Bug: 29831647 Change-Id: I9e57d1648d55af2ca0191bb47868e375ecc26950 Signed-off-by: Connor O'Brien (cherry picked from commit baa126dc158a40bc83c17c6d428c760e5b93fb1a) (cherry picked from commit 470484d2a25ad432190a01d1c763b4b36db33c7e) --- include/sysutils/FrameworkListener.h | 1 + libsysutils/src/FrameworkListener.cpp | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/include/sysutils/FrameworkListener.h b/include/sysutils/FrameworkListener.h index 18049cd..2137069 100644 --- a/include/sysutils/FrameworkListener.h +++ b/include/sysutils/FrameworkListener.h @@ -32,6 +32,7 @@ private: int mCommandCount; bool mWithSeq; FrameworkCommandCollection *mCommands; + bool mSkipToNextNullByte; public: FrameworkListener(const char *socketName); diff --git a/libsysutils/src/FrameworkListener.cpp b/libsysutils/src/FrameworkListener.cpp index e7b3dd6..579ead9 100644 --- a/libsysutils/src/FrameworkListener.cpp +++ b/libsysutils/src/FrameworkListener.cpp @@ -49,6 +49,7 @@ void FrameworkListener::init(const char *socketName UNUSED, bool withSeq) { errorRate = 0; mCommandCount = 0; mWithSeq = withSeq; + mSkipToNextNullByte = false; } bool FrameworkListener::onDataAvailable(SocketClient *c) { @@ -59,10 +60,15 @@ bool FrameworkListener::onDataAvailable(SocketClient *c) { if (len < 0) { SLOGE("read() failed (%s)", strerror(errno)); return false; - } else if (!len) + } else if (!len) { return false; - if(buffer[len-1] != '\0') + } else if (buffer[len-1] != '\0') { SLOGW("String is not zero-terminated"); + android_errorWriteLog(0x534e4554, "29831647"); + c->sendMsg(500, "Command too large for buffer", false); + mSkipToNextNullByte = true; + return false; + } int offset = 0; int i; @@ -70,11 +76,16 @@ bool FrameworkListener::onDataAvailable(SocketClient *c) { for (i = 0; i < len; i++) { if (buffer[i] == '\0') { /* IMPORTANT: dispatchCommand() expects a zero-terminated string */ - dispatchCommand(c, buffer + offset); + if (mSkipToNextNullByte) { + mSkipToNextNullByte = false; + } else { + dispatchCommand(c, buffer + offset); + } offset = i + 1; } } + mSkipToNextNullByte = false; return true; } -- cgit v1.1 From e8ee40302200c1bd1fd6537c80bf5e8aab744d42 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Tue, 23 Aug 2016 10:23:36 -0700 Subject: liblog: add __android_log_close() Bug: 30963384 (cherry picked from commit df7a4c6bae5f85532d79a93b7d9197a2aab17825) Change-Id: I2255486e84dd55af0f4e7fbbfb616c2deb1765d0 (cherry picked from commit 2d149691552676968b7de337f543463b920578b0) --- include/android/log.h | 5 +++++ liblog/logd_write.c | 39 +++++++++++++++++++++++++++++++++++++++ liblog/logd_write_kern.c | 35 +++++++++++++++++++++++++++++++++++ liblog/tests/liblog_test.cpp | 10 +++++++++- 4 files changed, 88 insertions(+), 1 deletion(-) diff --git a/include/android/log.h b/include/android/log.h index 1c171b7..391c826 100644 --- a/include/android/log.h +++ b/include/android/log.h @@ -89,6 +89,11 @@ typedef enum android_LogPriority { } android_LogPriority; /* + * Release any logger resources (a new log write will immediately re-acquire) + */ +void __android_log_close(); + +/* * Send a simple string to the log. */ int __android_log_write(int prio, const char *tag, const char *text); diff --git a/liblog/logd_write.c b/liblog/logd_write.c index bdee28f..64492f2 100644 --- a/liblog/logd_write.c +++ b/liblog/logd_write.c @@ -323,6 +323,45 @@ const char *android_log_id_to_name(log_id_t log_id) } #endif +/* + * Release any logger resources. A new log write will immediately re-acquire. + */ +void __android_log_close() +{ +#if FAKE_LOG_DEVICE + int i; +#endif + +#ifdef HAVE_PTHREADS + pthread_mutex_lock(&log_init_lock); +#endif + + write_to_log = __write_to_log_init; + + /* + * Threads that are actively writing at this point are not held back + * by a lock and are at risk of dropping the messages with a return code + * -EBADF. Prefer to return error code than add the overhead of a lock to + * each log writing call to guarantee delivery. In addition, anyone + * calling this is doing so to release the logging resources and shut down, + * for them to do so with outstanding log requests in other threads is a + * disengenuous use of this function. + */ +#if FAKE_LOG_DEVICE + for (i = 0; i < LOG_ID_MAX; i++) { + fakeLogClose(log_fds[i]); + log_fds[i] = -1; + } +#else + close(logd_fd); + logd_fd = -1; +#endif + +#ifdef HAVE_PTHREADS + pthread_mutex_unlock(&log_init_lock); +#endif +} + static int __write_to_log_init(log_id_t log_id, struct iovec *vec, size_t nr) { #if !defined(_WIN32) diff --git a/liblog/logd_write_kern.c b/liblog/logd_write_kern.c index 8742b34..abdbd03 100644 --- a/liblog/logd_write_kern.c +++ b/liblog/logd_write_kern.c @@ -104,6 +104,41 @@ static int __write_to_log_kernel(log_id_t log_id, struct iovec *vec, size_t nr) return ret; } +/* + * Release any logger resources. A new log write will immediately re-acquire. + */ +void __android_log_close() +{ +#ifdef HAVE_PTHREADS + pthread_mutex_lock(&log_init_lock); +#endif + + write_to_log = __write_to_log_init; + + /* + * Threads that are actively writing at this point are not held back + * by a lock and are at risk of dropping the messages with a return code + * -EBADF. Prefer to return error code than add the overhead of a lock to + * each log writing call to guarantee delivery. In addition, anyone + * calling this is doing so to release the logging resources and shut down, + * for them to do so with outstanding log requests in other threads is a + * disengenuous use of this function. + */ + + log_close(log_fds[LOG_ID_MAIN]); + log_fds[LOG_ID_MAIN] = -1; + log_close(log_fds[LOG_ID_RADIO]); + log_fds[LOG_ID_RADIO] = -1; + log_close(log_fds[LOG_ID_EVENTS]); + log_fds[LOG_ID_EVENTS] = -1; + log_close(log_fds[LOG_ID_SYSTEM]); + log_fds[LOG_ID_SYSTEM] = -1; + +#ifdef HAVE_PTHREADS + pthread_mutex_unlock(&log_init_lock); +#endif +} + static int __write_to_log_init(log_id_t log_id, struct iovec *vec, size_t nr) { pthread_mutex_lock(&log_init_lock); diff --git a/liblog/tests/liblog_test.cpp b/liblog/tests/liblog_test.cpp index c987041..0e7fec4 100644 --- a/liblog/tests/liblog_test.cpp +++ b/liblog/tests/liblog_test.cpp @@ -127,12 +127,17 @@ TEST(liblog, __android_log_btwrite__android_logger_list_read) { ASSERT_TRUE(NULL != (logger_list = android_logger_list_open( LOG_ID_EVENTS, ANDROID_LOG_RDONLY | ANDROID_LOG_NONBLOCK, 1000, pid))); + // Check that we can close and reopen the logger log_time ts(CLOCK_MONOTONIC); - ASSERT_LT(0, __android_log_btwrite(0, EVENT_TYPE_LONG, &ts, sizeof(ts))); + __android_log_close(); + + log_time ts1(CLOCK_MONOTONIC); + ASSERT_LT(0, __android_log_btwrite(0, EVENT_TYPE_LONG, &ts1, sizeof(ts1))); usleep(1000000); int count = 0; + int second_count = 0; for (;;) { log_msg log_msg; @@ -156,10 +161,13 @@ TEST(liblog, __android_log_btwrite__android_logger_list_read) { log_time tx(eventData + 4 + 1); if (ts == tx) { ++count; + } else if (ts1 == tx) { + ++second_count; } } EXPECT_EQ(1, count); + EXPECT_EQ(1, second_count); android_logger_list_close(logger_list); } -- cgit v1.1 From dc7cf2204e8dbb2c825bad52f8504921e4a0f2b9 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Tue, 23 Aug 2016 10:23:36 -0700 Subject: liblog: add __android_log_close() Bug: 30963384 (cherry picked from commit df7a4c6bae5f85532d79a93b7d9197a2aab17825) Change-Id: Ide70df3c04e29301649a1ca234b1b0af687bcbfb (cherry picked from commit b1b5d507cba4b37221016d308dbf9fdd39019108) --- liblog/logd_write.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/liblog/logd_write.c b/liblog/logd_write.c index 64492f2..667fdb3 100644 --- a/liblog/logd_write.c +++ b/liblog/logd_write.c @@ -355,6 +355,9 @@ void __android_log_close() #else close(logd_fd); logd_fd = -1; + + close(pstore_fd); + pstore_fd = -1; #endif #ifdef HAVE_PTHREADS -- cgit v1.1