commit b21b67ecac2cbd3966be89d139159e7d0966e804
author Ting Shen <phoenixshen@google.com> Fri Oct 16 16:07:41 2020 +0800
committer Commit Bot <commit-bot@chromium.org> Wed Oct 21 00:58:34 2020 +0000
tree 03570b50662a927a8ffb4ba215151f8ec024dba3
parent f09087c61f6f0f57db8ce5c8df4b3f75952104ba

timberslide: fix buffer overflow

The `buffer` array is used as a c-style string in ProcessLogBuffer. If
ReadAtCurrentPosNoBestEffort() reads 4096 bytes into the buffer, the
missing '\0' will cause istringstream accessing the buffer out of bound.

Change the c-style string to c++ string, and construct it with the
correct length information.

Also remove the memset() since we don't need trailing '\0' anymore.

BUG=b:167475144
TEST=1) no garbage in ec log
     2) assert length of string <= 4096

Change-Id: I43da3c1e28df11c4ad9c474687c6bfbd453485b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2478842
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
Tested-by: Ting Shen <phoenixshen@chromium.org>
Commit-Queue: Nicolas Boichat <drinkcat@chromium.org>

timberslide/timberslide.cc

diff --git a/timberslide/timberslide.cc b/timberslide/timberslide.cc
index 2a10759..09f83c1 100644
--- a/timberslide/timberslide.cc
+++ b/timberslide/timberslide.cc
@@ -167,7 +167,7 @@
   return (*ec_uptime_ms > 0);
 }
 
-std::string TimberSlide::ProcessLogBuffer(const char* buffer,
+std::string TimberSlide::ProcessLogBuffer(const std::string& buffer,
                                           const base::Time& now) {
   int64_t ec_current_uptime_ms = 0;
   std::istringstream iss(buffer);
@@ -200,8 +200,6 @@
   char buffer[4096];
   int ret;
 
-  memset(buffer, 0, sizeof(buffer));
-
   ret = TEMP_FAILURE_RETRY(
       device_file_.ReadAtCurrentPosNoBestEffort(buffer, sizeof(buffer)));
   if (ret == 0)
@@ -213,7 +211,8 @@
     return;
   }
 
-  std::string str = ProcessLogBuffer(buffer, base::Time::Now());
+  std::string str =
+      ProcessLogBuffer(std::string(buffer, ret), base::Time::Now());
   ret = str.size();
 
   if (!base::AppendToFile(current_log_, str.c_str(), ret)) {

timberslide/timberslide.h

diff --git a/timberslide/timberslide.h b/timberslide/timberslide.h
index 7738576..511182e 100644
--- a/timberslide/timberslide.h
+++ b/timberslide/timberslide.h
@@ -23,7 +23,8 @@
               base::File uptime_file,
               const base::FilePath& log_dir);
 
-  std::string ProcessLogBuffer(const char* buffer, const base::Time& now);
+  std::string ProcessLogBuffer(const std::string& buffer,
+                               const base::Time& now);
 
  protected:
   // For testing