Potential fix for code scanning alert no. 974: Restrict permissions on host-created files

CHANGELOG.md

@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [7.0.5]
+
+[7.0.5]: https://github.com/microsoft/CCF/releases/tag/ccf-7.0.5
+
+### Security
+
+- Host-created files (ledger chunks, snapshots, PID file, and node certificate/key files) are now created with restrictive permissions (`0600`) instead of relying on the process `umask`. Existing deployments will not see existing files affected; only newly created files will have these restricted permissions (#7916).
+
 ## [7.0.4]
 
 [7.0.4]: https://github.com/microsoft/CCF/releases/tag/ccf-7.0.4

python/pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "ccf"
-version = "7.0.4"
+version = "7.0.5"
 authors = [
   { name="CCF Team", email="CCF-Sec@microsoft.com" },
 ]

src/ds/files.h

@@ -2,14 +2,23 @@
 // Licensed under the Apache 2.0 License.
 #pragma once
 
+#include <cerrno>
+#include <cstddef>
+#include <cstdio>
 #include <cstring>
+#include <fcntl.h>
+#include <filesystem>
 #include <fstream>
 #include <glob.h>
 #include <iostream>
 #include <nlohmann/json.hpp>
 #include <optional>
+#include <span>
 #include <sstream>
 #include <string>
+#include <string_view>
+#include <sys/stat.h>
+#include <unistd.h>
 #include <vector>
 
 #define FMT_HEADER_ONLY
@@ -19,6 +28,45 @@
 namespace files
 {
   namespace fs = std::filesystem;
+  static constexpr mode_t private_file_permissions = S_IRUSR | S_IWUSR;
+
+  static int open_fd(
+    const fs::path& file,
+    int flags,
+    mode_t permissions = private_file_permissions)
+  {
+    return ::open(file.c_str(), flags, permissions);
+  }
+
+  static FILE* open_file(
+    const fs::path& file,
+    int flags,
+    const char* mode,
+    mode_t permissions = private_file_permissions)
+  {
+    const auto fd = open_fd(file, flags, permissions);
+    if (fd == -1)
+    {
+      return nullptr;
+    }
+
+    errno = 0;
+    // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
+    auto* f = fdopen(fd, mode);
+    if (f == nullptr)
+    {
+      auto fdopen_errno = errno;
+      // Preserve the original fdopen() failure when it set errno, and only
+      // fall back to close()'s errno if fdopen() did not.
+      if (close(fd) != 0 && fdopen_errno == 0)
+      {
+        fdopen_errno = errno;
+      }
+      errno = fdopen_errno != 0 ? fdopen_errno : EIO;
+    }
+
+    return f;
+  }
 
   /**
    * @brief Checks if a path exists
@@ -114,32 +162,62 @@ namespace files
     return nlohmann::json::parse(v.begin(), v.end());
   }
 
+  static void dump(std::span<const std::byte> data, const fs::path& file)
+  {
+    auto* f = open_file(file, O_WRONLY | O_CREAT | O_TRUNC, "wb");
+    if (f == nullptr)
+    {
+      throw std::logic_error(fmt::format(
+        "Failed to open file {} for writing: {}",
+        file.string(),
+        std::strerror(errno))); // NOLINT(concurrency-mt-unsafe)
+    }
+
+    errno = 0;
+    const auto bytes_written =
+      fwrite(data.data(), sizeof(std::byte), data.size(), f);
+    const auto write_errno = errno;
+    errno = 0;
+    // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
+    const auto close_rc = fclose(f);
+    const auto close_errno = errno;
+    if (bytes_written != data.size() || close_rc != 0)
+    {
+      if (bytes_written != data.size())
+      {
+        errno = write_errno != 0 ? write_errno : EIO;
+      }
+      else
+      {
+        errno = close_errno != 0 ? close_errno : EIO;
+      }
+      throw std::logic_error(fmt::format(
+        "Failed to write to file {}: {}",
+        file.string(),
+        std::strerror(errno))); // NOLINT(concurrency-mt-unsafe)
+    }
+  }
+
   /**
-   * @brief Writes the content of a vector to a file
+   * @brief Writes the content of a byte span to a file
    *
-   * @param data vector to write
+   * @param data bytes to write
    * @param file the path
    */
-  static void dump(const std::vector<uint8_t>& data, const std::string& file)
+  static void dump(std::span<const uint8_t> data, const fs::path& file)
   {
-    using namespace std;
-    ofstream f(file, ios::binary | ios::trunc);
-    f.write(reinterpret_cast<const char*>(data.data()), data.size());
-    if (!f)
-    {
-      throw logic_error("Failed to write to file: " + file);
-    }
+    dump(std::as_bytes(data), file);
   }
 
   /**
-   * @brief Writes the content of a string to a file
+   * @brief Writes the content of a string view to a file
    *
-   * @param data string to write
+   * @param data string view to write
    * @param file the path
    */
-  static void dump(const std::string& data, const std::string& file)
+  static void dump(std::string_view data, const fs::path& file)
   {
-    dump(std::vector<uint8_t>(data.begin(), data.end()), file);
+    dump(std::as_bytes(std::span(data)), file);
   }
 
   static void rename(const fs::path& src, const fs::path& dst)

src/host/ledger.h

@@ -21,7 +21,6 @@
 #include <map>
 #include <string>
 #include <sys/types.h>
-#include <unistd.h>
 #include <uv.h>
 #include <vector>
 
@@ -120,13 +119,12 @@ namespace asynchost
 
       auto file_path = dir / file_name;
 
-      // Use exclusive-create mode ("x") to atomically fail if the file
-      // already exists, avoiding a separate fs::exists() stat call.
+      // Use O_EXCL to atomically fail if the file already exists, and create
+      // with restrictive permissions (0600) rather than relying on umask.
       {
         TimeBoundLogger log_if_slow(
-          fmt::format("Creating ledger file - fopen({})", file_path));
-        // NOLINTNEXTLINE(cppcoreguidelines-owning-memory)
-        file = fopen(file_path.c_str(), "w+bx");
+          fmt::format("Creating ledger file - open({})", file_path));
+        file = files::open_file(file_path, O_RDWR | O_CREAT | O_EXCL, "w+b");
       }
       if (file == nullptr)
       {

src/host/lfs_file_handler.h

@@ -2,12 +2,12 @@
 // Licensed under the Apache 2.0 License.
 #pragma once
 
+#include "ds/files.h"
 #include "ds/messaging.h"
 #include "indexing/lfs_ringbuffer_types.h"
 #include "time_bound_logger.h"
 
 #include <filesystem>
-#include <fstream>
 
 namespace asynchost
 {
@@ -50,16 +50,12 @@ namespace asynchost
           const auto target_path = root_dir / key;
           {
             TimeBoundLogger log_if_slow(fmt::format(
-              "Writing LFS file ({} bytes) - ofstream({})",
+              "Writing LFS file ({} bytes) - {}",
               encrypted.size(),
               target_path));
-            std::ofstream f(target_path, std::ios::trunc | std::ios::binary);
             LOG_TRACE_FMT(
               "Writing {} byte file to {}", encrypted.size(), target_path);
-            f.write(
-              reinterpret_cast<char const*>(encrypted.data()),
-              encrypted.size());
-            f.close();
+            files::dump(encrypted, target_path);
           }
         });
 

src/snapshots/snapshot_manager.h

@@ -4,6 +4,7 @@
 
 #include "ccf/ds/nonstd.h"
 #include "consensus/ledger_enclave_types.h"
+#include "ds/files.h"
 #include "host/time_bound_logger.h"
 #include "snapshots/filenames.h"
 
@@ -185,8 +186,8 @@ namespace snapshots
             {
               asynchost::TimeBoundLogger log_open_if_slow(
                 fmt::format("Opening snapshot file - open({})", file_name));
-              snapshot_fd = open(
-                full_snapshot_path.c_str(), O_CREAT | O_EXCL | O_WRONLY, 0664);
+              snapshot_fd =
+                files::open_fd(full_snapshot_path, O_CREAT | O_EXCL | O_WRONLY);
             }
             if (snapshot_fd == -1)
             {