From 305374cf0f8cf28b58a108cf4f45df92fc0dde86 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Thu, 18 Aug 2016 14:59:41 -0700 Subject: [PATCH] logger: validate hdr_size field in logger entry - check hdr_size to make sure it is in the expected range from sizeof entry_v1 to entry (entry_v4). - alter msg() method to report NULL on invalid hdr_size - alter all users of msg() method. Bug: 30947841 Change-Id: I9bc1740d7aa9f37df5be966c18de1fb9de63d5dd --- debuggerd/tombstone.cpp | 4 +++ include/log/logger.h | 9 ++++- liblog/logger_read.c | 4 +++ liblog/logprint.c | 10 ++++++ liblog/pmsg_reader.c | 4 +++ liblog/tests/liblog_benchmark.cpp | 4 +-- liblog/tests/liblog_test.cpp | 60 +++++++++++++++++++++---------- logd/tests/logd_test.cpp | 15 ++++++-- 8 files changed, 86 insertions(+), 24 deletions(-) diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp index 935967812..e80bc254e 100644 --- a/debuggerd/tombstone.cpp +++ b/debuggerd/tombstone.cpp @@ -539,6 +539,10 @@ static void dump_log_file( if (!hdr_size) { hdr_size = sizeof(log_entry.entry_v1); } + if ((hdr_size < sizeof(log_entry.entry_v1)) || + (hdr_size > sizeof(log_entry.entry))) { + continue; + } char* msg = reinterpret_cast(log_entry.buf) + hdr_size; char timeBuf[32]; diff --git a/include/log/logger.h b/include/log/logger.h index 60d47a2da..256fdd17f 100644 --- a/include/log/logger.h +++ b/include/log/logger.h @@ -143,7 +143,14 @@ struct log_msg { } char *msg() { - return entry.hdr_size ? (char *) buf + entry.hdr_size : entry_v1.msg; + unsigned short hdr_size = entry.hdr_size; + if (!hdr_size) { + hdr_size = sizeof(entry_v1); + } + if ((hdr_size < sizeof(entry_v1)) || (hdr_size > sizeof(entry))) { + return NULL; + } + return (char *) buf + hdr_size; } unsigned int len() { diff --git a/liblog/logger_read.c b/liblog/logger_read.c index 00157b793..0d5545302 100644 --- a/liblog/logger_read.c +++ b/liblog/logger_read.c @@ -367,6 +367,10 @@ static int android_transport_read(struct android_log_logger_list *logger_list, if (log_msg->entry_v2.hdr_size == 0) { log_msg->entry_v2.hdr_size = sizeof(struct logger_entry); } + if ((log_msg->entry_v2.hdr_size < sizeof(log_msg->entry_v1)) || + (log_msg->entry_v2.hdr_size > sizeof(log_msg->entry))) { + return -EINVAL; + } /* len validation */ if (ret <= log_msg->entry_v2.hdr_size) { diff --git a/liblog/logprint.c b/liblog/logprint.c index 59bd47917..e21a9c46e 100644 --- a/liblog/logprint.c +++ b/liblog/logprint.c @@ -496,6 +496,11 @@ LIBLOG_ABI_PUBLIC int android_log_processLogBuffer( char *msg = buf->msg; struct logger_entry_v2 *buf2 = (struct logger_entry_v2 *)buf; if (buf2->hdr_size) { + if ((buf2->hdr_size < sizeof(((struct log_msg *)NULL)->entry_v1)) || + (buf2->hdr_size > sizeof(((struct log_msg *)NULL)->entry))) { + fprintf(stderr, "+++ LOG: entry illegal hdr_size\n"); + return -1; + } msg = ((char *)buf2) + buf2->hdr_size; if (buf2->hdr_size >= sizeof(struct logger_entry_v4)) { entry->uid = ((struct logger_entry_v4 *)buf)->uid; @@ -775,6 +780,11 @@ LIBLOG_ABI_PUBLIC int android_log_processBinaryLogBuffer( eventData = (const unsigned char*) buf->msg; struct logger_entry_v2 *buf2 = (struct logger_entry_v2 *)buf; if (buf2->hdr_size) { + if ((buf2->hdr_size < sizeof(((struct log_msg *)NULL)->entry_v1)) || + (buf2->hdr_size > sizeof(((struct log_msg *)NULL)->entry))) { + fprintf(stderr, "+++ LOG: entry illegal hdr_size\n"); + return -1; + } eventData = ((unsigned char *)buf2) + buf2->hdr_size; if ((buf2->hdr_size >= sizeof(struct logger_entry_v3)) && (((struct logger_entry_v3 *)buf)->lid == LOG_ID_SECURITY)) { diff --git a/liblog/pmsg_reader.c b/liblog/pmsg_reader.c index a4eec65a5..679c15957 100644 --- a/liblog/pmsg_reader.c +++ b/liblog/pmsg_reader.c @@ -343,6 +343,10 @@ LIBLOG_ABI_PRIVATE ssize_t __android_log_pmsg_file_read( char *msg = (char *)&transp.logMsg + hdr_size; char *split = NULL; + if ((hdr_size < sizeof(transp.logMsg.entry_v1)) || + (hdr_size > sizeof(transp.logMsg.entry))) { + continue; + } /* Check for invalid sequence number */ if ((transp.logMsg.entry.nsec % ANDROID_LOG_PMSG_FILE_SEQUENCE) || ((transp.logMsg.entry.nsec / ANDROID_LOG_PMSG_FILE_SEQUENCE) >= diff --git a/liblog/tests/liblog_benchmark.cpp b/liblog/tests/liblog_benchmark.cpp index 1b66a562a..f4e3089f6 100644 --- a/liblog/tests/liblog_benchmark.cpp +++ b/liblog/tests/liblog_benchmark.cpp @@ -542,7 +542,7 @@ static void BM_log_latency(int iters) { char* eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_LONG) { + if (!eventData || (eventData[4] != EVENT_TYPE_LONG)) { continue; } log_time tx(eventData + 4 + 1); @@ -622,7 +622,7 @@ static void BM_log_delay(int iters) { char* eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_LONG) { + if (!eventData || (eventData[4] != EVENT_TYPE_LONG)) { continue; } log_time tx(eventData + 4 + 1); diff --git a/liblog/tests/liblog_test.cpp b/liblog/tests/liblog_test.cpp index df2c76603..8f6f07a16 100644 --- a/liblog/tests/liblog_test.cpp +++ b/liblog/tests/liblog_test.cpp @@ -154,7 +154,7 @@ TEST(liblog, __android_log_btwrite__android_logger_list_read) { char *eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_LONG) { + if (!eventData || (eventData[4] != EVENT_TYPE_LONG)) { continue; } @@ -227,7 +227,7 @@ static void bswrite_test(const char *message) { char *eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_STRING) { + if (!eventData || (eventData[4] != EVENT_TYPE_STRING)) { continue; } @@ -506,7 +506,7 @@ TEST(liblog, __security_buffer) { char *eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_LONG) { + if (!eventData || (eventData[4] != EVENT_TYPE_LONG)) { continue; } @@ -637,7 +637,7 @@ TEST(liblog, android_logger_list_read__cpu_signal) { char *eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_LONG) { + if (!eventData || (eventData[4] != EVENT_TYPE_LONG)) { continue; } @@ -788,7 +788,7 @@ TEST(liblog, android_logger_list_read__cpu_thread) { char *eventData = log_msg.msg(); - if (eventData[4] != EVENT_TYPE_LONG) { + if (!eventData || (eventData[4] != EVENT_TYPE_LONG)) { continue; } @@ -990,9 +990,9 @@ TEST(liblog, max_payload) { continue; } - char *data = log_msg.msg() + 1; + char *data = log_msg.msg(); - if (strcmp(data, tag)) { + if (!data || strcmp(++data, tag)) { continue; } @@ -1107,9 +1107,9 @@ TEST(liblog, too_big_payload) { continue; } - char *data = log_msg.msg() + 1; + char *data = log_msg.msg(); - if (strcmp(data, tag)) { + if (!data || strcmp(++data, tag)) { continue; } @@ -1606,6 +1606,9 @@ TEST(liblog, android_errorWriteWithInfoLog__android_logger_list_read__typical) { } char *eventData = log_msg.msg(); + if (!eventData) { + continue; + } // Tag int tag = get4LE(eventData); @@ -1687,6 +1690,10 @@ TEST(liblog, android_errorWriteWithInfoLog__android_logger_list_read__data_too_l } char *eventData = log_msg.msg(); + if (!eventData) { + continue; + } + char *original = eventData; // Tag @@ -1774,6 +1781,9 @@ TEST(liblog, android_errorWriteWithInfoLog__android_logger_list_read__null_data) } char *eventData = log_msg.msg(); + if (!eventData) { + continue; + } // Tag int tag = get4LE(eventData); @@ -1817,6 +1827,9 @@ TEST(liblog, android_errorWriteWithInfoLog__android_logger_list_read__subtag_too } char *eventData = log_msg.msg(); + if (!eventData) { + continue; + } // Tag int tag = get4LE(eventData); @@ -1904,6 +1917,9 @@ TEST(liblog, android_errorWriteLog__android_logger_list_read__success) { } char *eventData = log_msg.msg(); + if (!eventData) { + continue; + } // Tag int tag = get4LE(eventData); @@ -1961,6 +1977,9 @@ TEST(liblog, android_errorWriteLog__android_logger_list_read__null_subtag) { } char *eventData = log_msg.msg(); + if (!eventData) { + continue; + } // Tag int tag = get4LE(eventData); @@ -2449,16 +2468,19 @@ static void create_android_logger(const char *(*fn)(uint32_t tag, size_t &expect android_log_format_free(logformat); // test buffer reading API - snprintf(msgBuf, sizeof(msgBuf), "I/[%d]", get4LE(eventData)); - print_barrier(); - fprintf(stderr, "%-10s(%5u): ", msgBuf, pid); - memset(msgBuf, 0, sizeof(msgBuf)); - int buffer_to_string = android_log_buffer_to_string( - eventData + sizeof(uint32_t), - log_msg.entry.len - sizeof(uint32_t), - msgBuf, sizeof(msgBuf)); - fprintf(stderr, "%s\n", msgBuf); - print_barrier(); + int buffer_to_string = -1; + if (eventData) { + snprintf(msgBuf, sizeof(msgBuf), "I/[%d]", get4LE(eventData)); + print_barrier(); + fprintf(stderr, "%-10s(%5u): ", msgBuf, pid); + memset(msgBuf, 0, sizeof(msgBuf)); + buffer_to_string = android_log_buffer_to_string( + eventData + sizeof(uint32_t), + log_msg.entry.len - sizeof(uint32_t), + msgBuf, sizeof(msgBuf)); + fprintf(stderr, "%s\n", msgBuf); + print_barrier(); + } EXPECT_EQ(0, buffer_to_string); EXPECT_EQ(strlen(expected_string), strlen(msgBuf)); EXPECT_EQ(0, strcmp(expected_string, msgBuf)); diff --git a/logd/tests/logd_test.cpp b/logd/tests/logd_test.cpp index 2014374e7..301ede92b 100644 --- a/logd/tests/logd_test.cpp +++ b/logd/tests/logd_test.cpp @@ -213,9 +213,15 @@ static void dump_log_msg(const char *prefix, version = 1; break; - case sizeof(msg->entry_v2): + case sizeof(msg->entry_v2): /* PLUS case sizeof(msg->entry_v3): */ if (version == 0) { - version = 2; + version = (msg->entry_v3.lid < LOG_ID_MAX) ? 3 : 2; + } + break; + + case sizeof(msg->entry_v4): + if (version == 0) { + version = 4; } break; } @@ -269,6 +275,11 @@ static void dump_log_msg(const char *prefix, unsigned int len = msg->entry.len; fprintf(stderr, "msg[%u]={", len); unsigned char *cp = reinterpret_cast(msg->msg()); + if (!cp) { + static const unsigned char garbage[] = ""; + cp = const_cast(garbage); + len = strlen(reinterpret_cast(garbage)); + } while(len) { unsigned char *p = cp; while (*p && (((' ' <= *p) && (*p < 0x7F)) || (*p == '\n'))) {