commit 72f911c08532b575717968e0dc189f675ad6268d
author Yi-An Chen <chenyian@google.com> Mon Jun 06 12:05:11 2022 +0800
committer Chromeos LUCI <chromeos-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Jun 23 19:37:27 2022 +0000
tree 9094ef88eec54c0a937af8c1715e90382db1f3b0
parent 8d42edc7231cddc308a49ee355aca79991f3cf51

debugd: Pass absolute path to rma_fw_keeper

Original code verifies the absolute path of image_file and ro_db_dir.
But then provides the original image_file and ro_db_dir to rma_fw_keeper
as arguments. This allows for malicious filenames to be passed to the
script as long as it is a symlink which resolves to a valid firmware
file path.
BUG=chromium:1329946, b:234192237
TEST=FEATURES=test emerge-${BOARD} debugd
TEST=Verify the commands in crbug/1329946 used to exploit the bug no
     longer works.

Change-Id: I50f9e4b52e8744393e0d970ab9901de7f668b882
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3691194
Tested-by: Yi-An Chen <chenyian@google.com>
Commit-Queue: Yi-An Chen <chenyian@google.com>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Aashay Shringarpure <aashay@google.com>
(cherry picked from commit c5f3d339d8f0a05d420e01ba41b339f43301a58d)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3717710
Commit-Queue: Aashay Shringarpure <aashay@google.com>

debugd/src/verify_ro_tool.cc

diff --git a/debugd/src/verify_ro_tool.cc b/debugd/src/verify_ro_tool.cc
index b7db0f6..8d9f19d 100644
--- a/debugd/src/verify_ro_tool.cc
+++ b/debugd/src/verify_ro_tool.cc
@@ -39,15 +39,20 @@
                                           const std::string& image_file,
                                           const std::string& ro_db_dir,
                                           std::string* handle) {
-  if (!CheckCr50ResourceLocation(image_file, false /* is_dir */)) {
+  base::FilePath image_absolute_path =
+      base::MakeAbsoluteFilePath(base::FilePath(image_file));
+  base::FilePath ro_db_absolute_path =
+      base::MakeAbsoluteFilePath(base::FilePath(ro_db_dir));
+
+  if (!CheckCr50ResourceLocation(image_absolute_path, false /* is_dir */)) {
     DEBUGD_ADD_ERROR(error, kVerifyRoToolErrorString,
-                     "Bad FW image file: " + image_file);
+                     "Bad FW image file: " + image_absolute_path.value());
     return false;
   }
 
-  if (!CheckCr50ResourceLocation(ro_db_dir, true /* is_dir */)) {
+  if (!CheckCr50ResourceLocation(ro_db_absolute_path, true /* is_dir */)) {
     DEBUGD_ADD_ERROR(error, kVerifyRoToolErrorString,
-                     "Bad ro descriptor dir: " + ro_db_dir);
+                     "Bad ro descriptor dir: " + ro_db_absolute_path.value());
     return false;
   }
 
@@ -61,8 +66,8 @@
   }
 
   p->AddArg(kCr50VerifyRoScript);
-  p->AddArg(image_file);
-  p->AddArg(ro_db_dir);
+  p->AddArg(image_absolute_path.value());
+  p->AddArg(ro_db_absolute_path.value());
 
   p->BindFd(outfd.get(), STDOUT_FILENO);
   p->BindFd(outfd.get(), STDERR_FILENO);
@@ -84,10 +89,8 @@
   return true;
 }
 
-bool VerifyRoTool::CheckCr50ResourceLocation(const std::string& path,
-                                             bool is_dir) {
-  base::FilePath absolute_path =
-      base::MakeAbsoluteFilePath(base::FilePath(path));
+bool VerifyRoTool::CheckCr50ResourceLocation(
+    const base::FilePath& absolute_path, bool is_dir) {
   if (absolute_path.empty()) {
     // |path| doesn't exist.
     return false;

debugd/src/verify_ro_tool.h

diff --git a/debugd/src/verify_ro_tool.h b/debugd/src/verify_ro_tool.h
index be89259..67a249a 100644
--- a/debugd/src/verify_ro_tool.h
+++ b/debugd/src/verify_ro_tool.h
@@ -35,10 +35,11 @@
                               std::string* handle);
 
  private:
-  // Checks and returns if |path| points to a valid cr50 resource location,
-  // i.e., a file or dir under /opt/google/cr50. If |is_dir| is set, returns
-  // false if |path| isn't a dir.
-  bool CheckCr50ResourceLocation(const std::string& path, bool is_dir);
+  // Checks and returns if |absolute_path| points to a valid cr50 resource
+  // location, i.e., a file or dir under /opt/google/cr50. If |is_dir| is set,
+  // returns false if |absolute_path| isn't a dir.
+  bool CheckCr50ResourceLocation(const base::FilePath& absolute_path,
+                                 bool is_dir);
 };
 
 }  // namespace debugd