From 6d659a5283da67837e0b0ea81991d71ae068ac1c Mon Sep 17 00:00:00 2001
From: Jordan Rome <jordalgo@meta.com>
Date: Wed, 6 Mar 2024 13:59:05 -0500
Subject: [PATCH] Fix security hole checking unpacked kernel headers (#3033)

Make sure to check that the unpacked kheaders tar
is owned by root to prevent bpftrace from loading
compromised linux headers.

Co-authored-by: Jordan Rome <jordalgo@fedoraproject.org>
---
 src/utils.cpp   | 26 ++++++++++++++++++++++----
 src/utils.h     |  1 +
 tests/utils.cpp | 21 +++++++++++++++++++++
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/src/utils.cpp b/src/utils.cpp
index 426644e8..872014ad 100644
--- a/src/utils.cpp
+++ b/src/utils.cpp
@@ -111,6 +111,8 @@ const struct vmlinux_location vmlinux_locs[] = {
   { nullptr, false },
 };
 
+constexpr std::string_view PROC_KHEADERS_PATH = "/sys/kernel/kheaders.tar.xz";
+
 static bool pid_in_different_mountns(int pid);
 static std::vector<std::string>
 resolve_binary_path(const std::string &cmd, const char *env_paths, int pid);
@@ -503,6 +505,20 @@ bool is_dir(const std::string& path)
   return std_filesystem::is_directory(buf, ec);
 }
 
+bool file_exists_and_ownedby_root(const char *f)
+{
+  struct stat st;
+  if (stat(f, &st) == 0) {
+    if (st.st_uid != 0) {
+      LOG(ERROR) << "header file ownership expected to be root: "
+                 << std::string(f);
+      return false;
+    }
+    return true;
+  }
+  return false;
+}
+
 namespace {
   struct KernelHeaderTmpDir {
     KernelHeaderTmpDir(const std::string& prefix) : path{prefix + "XXXXXX"}
@@ -535,14 +551,14 @@ namespace {
   {
     std::error_code ec;
     std_filesystem::path path_prefix{ "/tmp" };
-    std_filesystem::path path_kheaders{ "/sys/kernel/kheaders.tar.xz" };
+    std_filesystem::path path_kheaders{ PROC_KHEADERS_PATH };
     if (const char* tmpdir = ::getenv("TMPDIR")) {
       path_prefix = tmpdir;
     }
     path_prefix /= "kheaders-";
     std_filesystem::path shared_path{ path_prefix.string() + utsname.release };
 
-    if (std_filesystem::exists(shared_path, ec))
+    if (file_exists_and_ownedby_root(shared_path.c_str()))
     {
       // already unpacked
       return shared_path.string();
@@ -565,8 +581,10 @@ namespace {
     }
 
     KernelHeaderTmpDir tmpdir{path_prefix};
-
-    FILE* tar = ::popen(("tar xf /sys/kernel/kheaders.tar.xz -C " + tmpdir.path).c_str(), "w");
+    FILE *tar = ::popen(("tar xf " + std::string(PROC_KHEADERS_PATH) + " -C " +
+                         tmpdir.path)
+                            .c_str(),
+                        "w");
     if (!tar) {
       return "";
     }
diff --git a/src/utils.h b/src/utils.h
index 9b96be9f..103af0d3 100644
--- a/src/utils.h
+++ b/src/utils.h
@@ -156,6 +156,7 @@ std::vector<std::string> get_wildcard_tokens(const std::string &input,
 std::vector<int> get_online_cpus();
 std::vector<int> get_possible_cpus();
 bool is_dir(const std::string &path);
+bool file_exists_and_ownedby_root(const char *f);
 std::tuple<std::string, std::string> get_kernel_dirs(
     const struct utsname &utsname,
     bool unpack_kheaders);
diff --git a/tests/utils.cpp b/tests/utils.cpp
index 9ca4ace5..5244874f 100644
--- a/tests/utils.cpp
+++ b/tests/utils.cpp
@@ -222,6 +222,27 @@ TEST(utils, get_cgroup_path_in_hierarchy)
   }
 }
 
+TEST(utils, file_exists_and_ownedby_root)
+{
+  std::string tmpdir = "/tmp/bpftrace-test-utils-XXXXXX";
+  std::string file1 = "/ownedby-user";
+  std::string file2 = "/no-exists";
+  if (::mkdtemp(tmpdir.data()) == nullptr) {
+    throw std::runtime_error("creating temporary path for tests failed");
+  }
+
+  int fd;
+  fd = open((tmpdir + file1).c_str(), O_CREAT, S_IRUSR);
+  close(fd);
+  ASSERT_GE(fd, 0);
+
+  EXPECT_FALSE(file_exists_and_ownedby_root((tmpdir + file1).c_str()));
+  EXPECT_FALSE(file_exists_and_ownedby_root((tmpdir + file2).c_str()));
+  EXPECT_TRUE(file_exists_and_ownedby_root("/proc/1/maps"));
+
+  EXPECT_GT(std_filesystem::remove_all(tmpdir), 0);
+}
+
 } // namespace utils
 } // namespace test
 } // namespace bpftrace
-- 
2.44.0