fix: replace output files whole and refuse an output that names an input

src/tests/tools/validator-keys/Commands.cpp

@@ -184,7 +184,15 @@ TEST_F(CommandsTest, create_token)
     unwritable.outFile = file("missing/token.txt");
     EXPECT_EQ(
         commandError("create_token", {}, unwritable),
-        "Cannot open output file: " + unwritable.outFile->string());
+        "Cannot write output file: " + unwritable.outFile->string());
+    EXPECT_EQ(keys(options_).sequence(), 2u);
+
+    // The output may not replace the key file
+    ToolOptions aliased = options_;
+    aliased.outFile = options_.keyFile;
+    EXPECT_EQ(
+        commandError("create_token", {}, aliased),
+        "--out names an input file: " + options_.keyFile.string());
     EXPECT_EQ(keys(options_).sequence(), 2u);
 
     {
@@ -410,7 +418,22 @@ TEST_F(CommandsTest, list_commands)
         bad.outFile = file("missing/vl.json");
         EXPECT_EQ(
             commandError("sign_list", {unsignedList.string()}, bad),
-            "Cannot open output file: " + bad.outFile->string());
+            "Cannot write output file: " + bad.outFile->string());
+
+        // The output may not replace the list it signs
+        bad.outFile = unsignedList;
+        EXPECT_EQ(
+            commandError("sign_list", {unsignedList.string()}, bad),
+            "--out names an input file: " + unsignedList.string());
+
+        // A failed command leaves an existing output as it was
+        bad.outFile = file("kept.json");
+        writeFile(*bad.outFile, "previous");
+        EXPECT_EQ(
+            commandError("sign_list", {file("missing.json").string()}, bad),
+            "Failed to open file: " + file("missing.json").string());
+        EXPECT_EQ(readFile(*bad.outFile), "previous");
+        EXPECT_FALSE(std::filesystem::exists(file("kept.json.tmp")));
     }
     EXPECT_EQ(
         commandError("verify_list", {file("missing.json").string()}, verifier),

src/tests/tools/validator-keys/SigningKeys.cpp

@@ -145,6 +145,10 @@ TEST_F(SigningKeysTest, write_to_file_errors)
     auto const nested = dir_.file("a/b/c/validator_keys.json");
     keys.writeToFile(nested);
     EXPECT_TRUE(keys == SigningKeys::makeSigningKeys(nested));
+    EXPECT_EQ(
+        std::filesystem::status(std::filesystem::path(nested).parent_path()).permissions() &
+            (std::filesystem::perms::group_all | std::filesystem::perms::others_all),
+        std::filesystem::perms::none);
 
     // The parent path is a file
     auto const blocked = std::filesystem::path(keyFile_.string() + "/keys.json");

src/tools/validator-keys/CMakeLists.txt

@@ -6,7 +6,7 @@ include(GNUInstallDirs)
 add_library(xrpl.validator-keys STATIC)
 target_sources(
     xrpl.validator-keys
-    PRIVATE Commands.cpp ListSigning.cpp SigningKeys.cpp
+    PRIVATE Commands.cpp ListSigning.cpp OwnerOnlyFile.cpp SigningKeys.cpp
 )
 target_include_directories(
     xrpl.validator-keys

src/tools/validator-keys/Commands.cpp

@@ -19,6 +19,7 @@
 #include <boost/preprocessor/stringize.hpp>
 
 #include <tools/validator-keys/ListSigning.h>
+#include <tools/validator-keys/OwnerOnlyFile.h>
 #include <tools/validator-keys/SigningKeys.h>
 
 #include <algorithm>
@@ -26,8 +27,7 @@
 #include <cstddef>
 #include <cstdlib>
 #include <filesystem>
-#include <fstream>
-#include <ios>
+#include <memory>
 #include <optional>
 #include <ostream>
 #include <stdexcept>
@@ -72,58 +72,50 @@ struct Context
 };
 
 /**
- * Where a command's result goes: the output file named by `--out`, opened
- * before the command changes any state so an unwritable path fails first, or
- * the output stream. A file that receives nothing is removed again.
+ * Where a command's result goes: the file named by `--out`, replaced whole
+ * once the command has succeeded, or the output stream. The file is prepared
+ * before the command changes any state so an unwritable path fails first.
  */
 class Output
 {
-    // Empty when the result goes to the output stream.
-    std::filesystem::path file_;
-    std::ofstream stream_;
+    // Null when the result goes to the output stream.
+    std::unique_ptr<OwnerOnlyFile> file_;
     std::ostream& out_;
-    bool written_ = false;
 
 public:
-    Output(std::optional<std::filesystem::path> const& file, std::ostream& out)
-        : file_(file.value_or(std::filesystem::path{})), out_(out)
+    /**
+     * @param inputs Files the command reads besides those in the options;
+     *        `--out` may not name any input, since the output replaces it.
+     */
+    // A result that only goes to the output stream.
+    explicit Output(std::ostream& out) : out_(out)
     {
-        if (!file_.empty())
-        {
-            if (std::filesystem::is_symlink(file_))
-                throw std::runtime_error("Refusing to write through a symlink: " + file_.string());
-            stream_.open(file_, std::ios_base::trunc);
-            if (stream_.fail())
-                throw std::runtime_error("Cannot open output file: " + file_.string());
-            // A token holds a secret: restrict the file before anything is written.
-            std::error_code ec;
-            std::filesystem::permissions(
-                file_,
-                std::filesystem::perms::owner_read | std::filesystem::perms::owner_write,
-                ec);
-            if (ec)
-            {
-                stream_.close();                     // LCOV_EXCL_LINE
-                std::filesystem::remove(file_, ec);  // LCOV_EXCL_LINE
-                throw std::runtime_error(  // LCOV_EXCL_LINE
-                    "Cannot restrict output file: " + file_.string());  // LCOV_EXCL_LINE
-            }
-        }
     }
 
-    ~Output()
+    Output(Context const& ctx, std::vector<std::filesystem::path> const& inputs = {})
+        : out_(ctx.out)
     {
-        if (!file_.empty() && !written_)
-        {
-            stream_.close();
-            std::error_code ec;
-            std::filesystem::remove(file_, ec);
-        }
-    }
+        auto const& options = ctx.options;
+        if (!options.outFile)
+            return;
 
-    Output(Output const&) = delete;
-    Output&
-    operator=(Output const&) = delete;
+        std::vector<std::filesystem::path> read(inputs);
+        read.push_back(options.keyFile);
+        for (auto const& file :
+             {options.tokenFile, options.manifestFile, options.appendFile, options.validatorsFile})
+        {
+            if (file)
+                read.push_back(*file);
+        }
+        for (auto const& file : read)
+        {
+            std::error_code ec;
+            if (std::filesystem::equivalent(file, *options.outFile, ec))
+                throw std::runtime_error("--out names an input file: " + options.outFile->string());
+        }
+
+        file_ = std::make_unique<OwnerOnlyFile>(*options.outFile, "output file");
+    }
 
     // A config block in 72-character lines.
     void
@@ -133,7 +125,7 @@ public:
         for (std::size_t i = 0; i < body.size(); i += kBlockLineLength)
             text.append(body, i, kBlockLineLength).push_back('\n');
 
-        if (file_.empty())
+        if (!file_)
         {
             out_ << "Update xrpld.cfg file with these values and restart xrpld:\n\n"
                  << text << std::endl;
@@ -145,7 +137,7 @@ public:
     void
     json(json::Value const& jv)
     {
-        if (file_.empty())
+        if (!file_)
         {
             out_ << jv.toStyledString() << std::endl;
             return;
@@ -157,15 +149,9 @@ private:
     void
     write(std::string const& text, std::string const& what)
     {
-        stream_ << text;
-        stream_.close();
-        if (stream_.fail())
-        {
-            throw std::runtime_error(  // LCOV_EXCL_LINE
-                "Cannot write output file: " + file_.string());  // LCOV_EXCL_LINE
-        }
-        written_ = true;
-        out_ << what << " written to " << file_.string() << "\n";
+        file_->write(text);
+        file_->commit();
+        out_ << what << " written to " << file_->target().string() << "\n";
     }
 };
 
@@ -325,7 +311,7 @@ cmdCreateExternal(Args const& args, Context& ctx)
 int
 cmdCreateToken(Args const&, Context& ctx)
 {
-    Output output(ctx.options.outFile, ctx.out);
+    Output output(ctx);
     auto keys = SigningKeys::makeSigningKeys(ctx.options.keyFile);
     auto const token = keys.createToken(ctx.options.tokenKeyType);
     keys.writeToFile(ctx.options.keyFile);
@@ -346,7 +332,7 @@ cmdStartToken(Args const&, Context& ctx)
 int
 cmdFinishToken(Args const& args, Context& ctx)
 {
-    Output output(ctx.options.outFile, ctx.out);
+    Output output(ctx);
     auto keys = SigningKeys::makeSigningKeys(ctx.options.keyFile);
     std::optional<Blob> signingSig;
     if (args.size() == 2)
@@ -364,7 +350,7 @@ cmdRevokeKeys(Args const&, Context& ctx)
     warnRevocation(keys, ctx.err);
     auto const revocation = keys.revoke();
     keys.writeToFile(ctx.options.keyFile);
-    Output(std::nullopt, ctx.out).block("validator_key_revocation", nodePublic(keys), revocation);
+    Output(ctx.out).block("validator_key_revocation", nodePublic(keys), revocation);
     return EXIT_SUCCESS;
 }
 
@@ -384,14 +370,14 @@ cmdFinishRevokeKeys(Args const& args, Context& ctx)
     warnRevocation(keys, ctx.err);
     auto const revocation = keys.finishRevoke(decodeSignature(args[0]));
     keys.writeToFile(ctx.options.keyFile);
-    Output(std::nullopt, ctx.out).block("validator_key_revocation", nodePublic(keys), revocation);
+    Output(ctx.out).block("validator_key_revocation", nodePublic(keys), revocation);
     return EXIT_SUCCESS;
 }
 
 int
 setDomain(std::string const& domain, Context& ctx)
 {
-    Output output(ctx.options.outFile, ctx.out);
+    Output output(ctx);
     auto keys = loadUnrevoked(ctx.options.keyFile);
 
     if (domain == keys.domain())
@@ -529,7 +515,7 @@ cmdSignList(Args const& args, Context& ctx)
 {
     if (!ctx.options.tokenFile)
         throw std::runtime_error("sign_list needs --token-file");
-    Output output(ctx.options.outFile, ctx.out);
+    Output output(ctx, {args[0]});
 
     auto const token = loadTokenFile(*ctx.options.tokenFile);
     auto const manifest = deserializeManifest(base64Decode(token.manifest));
@@ -563,7 +549,7 @@ int
 cmdFinishSignList(Args const& args, Context& ctx)
 {
     auto const [manifest, signingKey] = loadSigningManifest(ctx, "finish_sign_list");
-    Output output(ctx.options.outFile, ctx.out);
+    Output output(ctx, {args[1]});
 
     auto const list = loadUnsignedList(args[1]);
     auto const sig = decodeSignature(args[0]);

src/tools/validator-keys/OwnerOnlyFile.cpp

@@ -0,0 +1,70 @@
+#include <tools/validator-keys/OwnerOnlyFile.h>
+
+#include <filesystem>
+#include <initializer_list>
+#include <ios>
+#include <stdexcept>
+#include <string>
+#include <system_error>
+#include <utility>
+
+namespace xrpl {
+
+namespace fs = std::filesystem;
+
+OwnerOnlyFile::OwnerOnlyFile(fs::path target, std::string what)
+    : target_(std::move(target)), temp_(target_.string() + ".tmp"), what_(std::move(what))
+{
+    for (auto const& path : {target_, temp_})
+    {
+        if (fs::is_symlink(path))
+            throw std::runtime_error("Refusing to write through a symlink: " + path.string());
+    }
+
+    stream_.open(temp_, std::ios_base::trunc);
+    if (stream_.fail())
+        throw std::runtime_error("Cannot write " + what_ + ": " + target_.string());
+
+    std::error_code ec;
+    fs::permissions(temp_, fs::perms::owner_read | fs::perms::owner_write, ec);
+    if (ec)
+    {
+        stream_.close();        // LCOV_EXCL_LINE
+        fs::remove(temp_, ec);  // LCOV_EXCL_LINE
+        throw std::runtime_error(                                 // LCOV_EXCL_LINE
+            "Cannot write " + what_ + ": " + target_.string());  // LCOV_EXCL_LINE
+    }
+}
+
+OwnerOnlyFile::~OwnerOnlyFile()
+{
+    if (!committed_)
+    {
+        stream_.close();
+        std::error_code ec;
+        fs::remove(temp_, ec);
+    }
+}
+
+void
+OwnerOnlyFile::write(std::string const& text)
+{
+    stream_ << text;
+}
+
+void
+OwnerOnlyFile::commit()
+{
+    stream_.close();
+    std::error_code ec;
+    if (!stream_.fail())
+        fs::rename(temp_, target_, ec);
+    if (stream_.fail() || ec)
+    {
+        fs::remove(temp_, ec);
+        throw std::runtime_error("Cannot write " + what_ + ": " + target_.string());
+    }
+    committed_ = true;
+}
+
+}  // namespace xrpl

src/tools/validator-keys/OwnerOnlyFile.h

@@ -0,0 +1,59 @@
+#pragma once
+
+#include <filesystem>
+#include <fstream>
+#include <string>
+
+namespace xrpl {
+
+/**
+ * A file written for its owner only and replaced whole.
+ *
+ * The content goes to a temporary beside the target, restricted to the owner
+ * before the first byte, and the target is replaced in one step by `commit`.
+ * Without `commit` the temporary is removed, so a failed command leaves the
+ * previous target as it was.
+ */
+class OwnerOnlyFile
+{
+    std::filesystem::path target_;
+    std::filesystem::path temp_;
+    // Names the file in errors: "key file", "output file".
+    std::string what_;
+    std::ofstream stream_;
+    bool committed_ = false;
+
+public:
+    /**
+     * Opens the temporary.
+     *
+     * @throws std::runtime_error if the target or the temporary is a symlink,
+     *         or the temporary cannot be opened or restricted
+     */
+    OwnerOnlyFile(std::filesystem::path target, std::string what);
+    ~OwnerOnlyFile();
+
+    OwnerOnlyFile(OwnerOnlyFile const&) = delete;
+    OwnerOnlyFile&
+    operator=(OwnerOnlyFile const&) = delete;
+
+    void
+    write(std::string const& text);
+
+    /**
+     * Replaces the target with what was written.
+     *
+     * @throws std::runtime_error if the content could not be written or the
+     *         target could not be replaced
+     */
+    void
+    commit();
+
+    [[nodiscard]] std::filesystem::path const&
+    target() const
+    {
+        return target_;
+    }
+};
+
+}  // namespace xrpl

src/tools/validator-keys/SigningKeys.cpp

@@ -24,13 +24,13 @@
 
 #include <boost/algorithm/string.hpp>
 
+#include <tools/validator-keys/OwnerOnlyFile.h>
+
 #include <algorithm>
 #include <array>
 #include <cstddef>
 #include <cstdint>
 #include <filesystem>
-#include <fstream>
-#include <ios>
 #include <limits>
 #include <optional>
 #include <stdexcept>
@@ -290,35 +290,16 @@ SigningKeys::writeToFile(std::filesystem::path const& keyFile) const
     std::error_code ec;
     if (auto const parent = keyFile.parent_path(); !parent.empty())
     {
-        fs::create_directories(parent, ec);
+        // A directory made here is the owner's alone.
+        if (fs::create_directories(parent, ec) && !ec)
+            fs::permissions(parent, fs::perms::owner_all, ec);
         if (ec || !fs::is_directory(parent))
             throw std::runtime_error("Cannot create directory: " + parent.string());
     }
 
-    // Write beside the key file, restrict it to the owner, then replace the
-    // key file in one step.
-    auto const temp = fs::path(keyFile.string() + ".tmp");
-    if (fs::is_symlink(temp))
-        throw std::runtime_error("Refusing to write through a symlink: " + temp.string());
-    {
-        std::ofstream o(temp, std::ios_base::trunc);
-        if (!o.fail())
-            fs::permissions(temp, fs::perms::owner_read | fs::perms::owner_write, ec);
-        if (!ec)
-            o << jv.toStyledString();
-        o.close();
-        if (ec || o.fail())
-        {
-            fs::remove(temp, ec);
-            throw std::runtime_error("Cannot write key file: " + keyFile.string());
-        }
-    }
-    fs::rename(temp, keyFile, ec);
-    if (ec)
-    {
-        fs::remove(temp, ec);
-        throw std::runtime_error("Cannot write key file: " + keyFile.string());
-    }
+    OwnerOnlyFile file(keyFile, "key file");
+    file.write(jv.toStyledString());
+    file.commit();
 }
 
 STObject