bug: explicitly set safe permissions on osquery dbs (#5229)

osquery/database/plugins/rocksdb.cpp

@@ -164,7 +164,7 @@ Status RocksDBDatabasePlugin::setUp() {
   }
 
   // RocksDB may not create/append a directory with acceptable permissions.
-  if (!read_only_ && platformChmod(path_, S_IRWXU) == false) {
+  if (!read_only_ && platformSetSafeDbPerms(path_) == false) {
     return Status(1, "Cannot set permissions on RocksDB path: " + path_);
   }
   return Status(0);

osquery/database/plugins/sqlite.cpp

@@ -92,7 +92,7 @@ Status SQLiteDatabasePlugin::setUp() {
   }
 
   // RocksDB may not create/append a directory with acceptable permissions.
-  if (!read_only_ && platformChmod(path_, S_IRWXU) == false) {
+  if (!read_only_ && platformSetSafeDbPerms(path_) == false) {
     close();
     return Status(1, "Cannot set permissions on database path: " + path_);
   }

osquery/dispatcher/scheduler.cpp

@@ -142,8 +142,8 @@ Status launchQuery(const std::string& name, const ScheduledQuery& query) {
     status = dbQuery.addNewResults(
         std::move(sql.rows()), item.epoch, item.counter, diff_results);
     if (!status.ok()) {
-      std::string line =
-          "Error adding new results to database: " + status.what();
+      std::string line = "Error adding new results to database for query " +
+                         name + ": " + status.what();
       LOG(ERROR) << line;
 
       // If the database is not available then the daemon cannot continue.

osquery/filesystem/fileops.h

@@ -336,6 +336,15 @@ boost::optional<std::string> getHomeDirectory();
  */
 bool platformChmod(const std::string& path, mode_t perms);
 
+/**
+ * @brief Sets 'safe' permissions for the database backing osquery
+ *
+ * @note Safe DB perms are equivalent to a chmod 0700 for root on posix
+ * so we emulate this by granting Full perms to SYSTEM and Administrators
+ * only.
+ */
+bool platformSetSafeDbPerms(const std::string& path);
+
 /**
  * @brief Multi-platform implementation of glob.
  * @note glob support is not 100% congruent with Linux glob. There are slight

osquery/filesystem/posix/fileops.cpp

@@ -281,6 +281,10 @@ boost::optional<std::string> getHomeDirectory() {
   }
 }
 
+bool platformSetSafeDbPerms(const std::string& path) {
+  return platformChmod(path, S_IRWXU);
+}
+
 bool platformChmod(const std::string& path, mode_t perms) {
   return (::chmod(path.c_str(), perms) == 0);
 }

osquery/filesystem/tests/fileops_tests.cpp

@@ -550,9 +550,10 @@ TEST_F(FileOpsTests, test_access) {
 }
 
 TEST_F(FileOpsTests, test_safe_permissions) {
-  const auto root_dir =
-      (fs::temp_directory_path() / "safe-perms-test").string();
-  const auto temp_file = root_dir + "/test";
+  const auto root_path = fs::temp_directory_path() / "safe-perms-test";
+  const auto temp_file = (root_path / "test").string();
+  const auto root_dir = root_path.string();
+
   const int all_access = S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP |
                          S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH;
 
@@ -632,6 +633,41 @@ TEST_F(FileOpsTests, test_safe_permissions) {
   fs::remove_all(root_dir);
 }
 
+TEST_F(FileOpsTests, test_safe_db_permissions) {
+  const auto db_path = fs::temp_directory_path() / "safe-db-perms-test.db";
+  const auto sst_file = (db_path / "1234.sst").string();
+  const auto db = db_path.string();
+
+  fs::create_directories(db);
+
+  // Ensure that 'safe' permissions get applied correctly
+  {
+    EXPECT_TRUE(platformSetSafeDbPerms(db));
+
+    PlatformFile fd(sst_file, PF_CREATE_ALWAYS | PF_WRITE);
+    ASSERT_TRUE(fd.isValid());
+
+    // The 'hasSafePermissions' function ensures no low priv writes can occur
+    auto status = fd.hasSafePermissions();
+
+    EXPECT_TRUE(fd.hasSafePermissions().ok());
+    EXPECT_EQ(0, status.getCode());
+  }
+
+  // Ensure that we still have read and write access to the db
+  {
+    EXPECT_EQ(0, platformAccess(db, R_OK | W_OK));
+    EXPECT_EQ(0, platformAccess(sst_file, R_OK | W_OK));
+  }
+
+  // Tear down our mock DB files
+  const int all_access = S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IWGRP |
+                         S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH;
+  EXPECT_TRUE(platformChmod(db, all_access));
+  EXPECT_TRUE(platformChmod(sst_file, all_access));
+  fs::remove_all(db);
+}
+
 TEST_F(FileOpsTests, test_glob) {
   {
     std::vector<fs::path> expected{kFakeDirectory + "/door.txt",
@@ -739,4 +775,4 @@ TEST_F(FileOpsTests, test_zero_permissions_file) {
     EXPECT_EQ(boost::none, platformFopen(path, "r"));
   }
 }
-}
+} // namespace osquery

osquery/filesystem/windows/fileops.cpp

@@ -1082,6 +1082,96 @@ size_t PlatformFile::size() const {
   return ::GetFileSize(handle_, nullptr);
 }
 
+bool platformSetSafeDbPerms(const std::string& path) {
+  unsigned long sid_size = SECURITY_MAX_SID_SIZE;
+  std::vector<char> admins_buf(sid_size);
+  PSID admins = static_cast<PSID>(admins_buf.data());
+  if (!::CreateWellKnownSid(
+          WinBuiltinAdministratorsSid, nullptr, admins, &sid_size)) {
+    return false;
+  }
+
+  std::vector<char> system_buf(sid_size);
+  PSID system = static_cast<PSID>(system_buf.data());
+  if (!::CreateWellKnownSid(WinLocalSystemSid, nullptr, system, &sid_size)) {
+    return false;
+  }
+
+  std::vector<char> world_buf(sid_size);
+  PSID world = static_cast<PSID>(world_buf.data());
+  if (!::CreateWellKnownSid(WinWorldSid, nullptr, world, &sid_size)) {
+    return false;
+  }
+
+  EXPLICIT_ACCESS admins_ea;
+  admins_ea.grfAccessMode = SET_ACCESS;
+  admins_ea.grfAccessPermissions = GENERIC_ALL;
+  admins_ea.grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
+
+  PTRUSTEE_A trust = static_cast<PTRUSTEE_A>(malloc(sizeof(TRUSTEE_A)));
+  BuildTrusteeWithSidA(trust, admins);
+  admins_ea.Trustee = *trust;
+
+  // Set the Administrators ACE
+  PACL new_dacl = nullptr;
+  auto ret = SetEntriesInAcl(1, &admins_ea, nullptr, &new_dacl);
+  if (ret != ERROR_SUCCESS) {
+    VLOG(1) << "Failed to set DB permissions for Administrators";
+    LocalFree(new_dacl);
+    free(trust);
+    return false;
+  }
+
+  // Set the SYSTEM ACE
+  EXPLICIT_ACCESS sys_ea;
+  sys_ea.grfAccessMode = SET_ACCESS;
+  sys_ea.grfAccessPermissions = GENERIC_ALL;
+  sys_ea.grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
+
+  BuildTrusteeWithSidA(trust, system);
+  sys_ea.Trustee = *trust;
+  ret = SetEntriesInAcl(1, &sys_ea, new_dacl, &new_dacl);
+  if (ret != ERROR_SUCCESS) {
+    VLOG(1) << "Failed to set DB permissions for SYSTEM";
+    LocalFree(new_dacl);
+    free(trust);
+    return false;
+  }
+
+  // Grant Everyone the ability to read the DACL
+  EXPLICIT_ACCESS world_ea;
+  world_ea.grfAccessMode = SET_ACCESS;
+  world_ea.grfAccessPermissions = READ_CONTROL;
+  world_ea.grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
+
+  BuildTrusteeWithSidA(trust, world);
+  world_ea.Trustee = *trust;
+  ret = SetEntriesInAcl(1, &world_ea, new_dacl, &new_dacl);
+  if (ret != ERROR_SUCCESS) {
+    VLOG(1) << "Failed to set DB permissions for SYSTEM";
+    LocalFree(new_dacl);
+    free(trust);
+    return false;
+  }
+
+  // Apply 'safe' DACL and avoid returning to attempt applying the DACL
+  ret = SetNamedSecurityInfoA(
+      const_cast<char*>(path.c_str()),
+      SE_FILE_OBJECT,
+      OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION |
+          DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION,
+      admins,
+      admins,
+      new_dacl,
+      nullptr);
+  if (ret != ERROR_SUCCESS) {
+    LOG(WARNING) << "Failed to apply safe permssions to the database";
+  }
+  LocalFree(new_dacl);
+  free(trust);
+  return true;
+}
+
 bool platformChmod(const std::string& path, mode_t perms) {
   PACL dacl = nullptr;
   PSID owner = nullptr;
@@ -1179,10 +1269,7 @@ bool platformChmod(const std::string& path, mode_t perms) {
   }
 
   // Lastly, apply the permissions to the object
-  // SetNamedSecurityInfoA takes a mutable string for the path parameter
-  std::vector<char> mutable_path(path.begin(), path.end());
-  mutable_path.push_back('\0');
-  if (SetNamedSecurityInfoA(mutable_path.data(),
+  if (SetNamedSecurityInfoA(const_cast<char*>(path.c_str()),
                             SE_FILE_OBJECT,
                             DACL_SECURITY_INFORMATION,
                             nullptr,
@@ -1750,4 +1837,4 @@ fs::path getSystemRoot() {
 Status platformLstat(const std::string& path, struct stat& d_stat) {
   return Status(1);
 }
-}
+} // namespace osquery