fix: reject duplicate validators and a zero expiration; restrict output files before writing

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

@@ -155,6 +155,15 @@ TEST_F(CommandsTest, create_token)
         std::filesystem::perms::none);
     EXPECT_EQ(keys(options).sequence(), 2u);
 
+    // A symlink is not written through
+    ToolOptions linked = options;
+    linked.outFile = file("link.txt");
+    std::filesystem::create_symlink(file("elsewhere.txt"), *linked.outFile);
+    EXPECT_EQ(
+        commandError("create_token", {}, linked),
+        "Refusing to write through a symlink: " + linked.outFile->string());
+    EXPECT_EQ(keys(options).sequence(), 2u);
+
     // An unwritable output path fails before the sequence is consumed
     ToolOptions unwritable = options;
     unwritable.outFile = file("missing/token.txt");

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

@@ -124,7 +124,7 @@ TEST_F(ListSigningTest, parse_unsigned_list)
     }
 
     std::string const sequenceError = "\"sequence\" must be an integer from 1 to 2147483647";
-    std::string const expirationError = "\"expiration\" must be an integer from 0 to 2147483647";
+    std::string const expirationError = "\"expiration\" must be an integer from 1 to 2147483647";
     EXPECT_EQ(errorOfParse("{\"expiration\": 1, \"validators\": []}"), sequenceError);
     EXPECT_EQ(
         errorOfParse("{\"sequence\": 0, \"expiration\": 1, \"validators\": []}"), sequenceError);
@@ -136,6 +136,17 @@ TEST_F(ListSigningTest, parse_unsigned_list)
     EXPECT_EQ(errorOfParse("{\"sequence\": 1, \"validators\": []}"), expirationError);
     EXPECT_EQ(
         errorOfParse("{\"sequence\": 1, \"expiration\": -1, \"validators\": []}"), expirationError);
+    EXPECT_EQ(
+        errorOfParse("{\"sequence\": 1, \"expiration\": 0, \"validators\": []}"), expirationError);
+    {
+        auto twice = validators;
+        twice.push_back(validators[0]);
+        auto const key =
+            strHex(deserializeManifest(base64Decode(validators[0].manifest))->masterKey);
+        EXPECT_EQ(
+            errorOfParse(unsignedListText(twice, 1, 1000)),
+            "\"validators\" lists " + key + " more than once");
+    }
     EXPECT_EQ(
         errorOfParse(
             "{\"sequence\": 1, \"effective\": 1000, \"expiration\": 1000, \"validators\": []}"),

src/tools/validator-keys/Commands.cpp

@@ -67,9 +67,17 @@ public:
     {
         if (file_)
         {
+            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);
         }
     }
 
@@ -87,8 +95,7 @@ public:
     Output&
     operator=(Output const&) = delete;
 
-    // A config block in 72-character lines; a file gets owner-only permissions
-    // because a token holds a secret.
+    // A config block in 72-character lines.
     void
     block(std::string const& section, std::string const& publicKey, std::string const& body)
     {
@@ -103,9 +110,6 @@ public:
             return;
         }
         write(text, "[" + section + "]");
-        std::error_code ec;
-        std::filesystem::permissions(
-            *file_, std::filesystem::perms::owner_read | std::filesystem::perms::owner_write, ec);
     }
 
     void

src/tools/validator-keys/ListSigning.cpp

@@ -9,6 +9,7 @@
 
 #include <boost/algorithm/string.hpp>
 
+#include <algorithm>
 #include <chrono>
 #include <set>
 
@@ -85,9 +86,10 @@ checkedList(std::string canonical, json::Value const& jv)
         throw std::runtime_error(integerError(jss::sequence, 1));
     list.sequence = *sequence;
 
+    // A server takes a missing effective time as 0 and needs expiration after it.
     auto const expiration = listInteger(jv, jss::expiration);
-    if (!expiration)
-        throw std::runtime_error(integerError(jss::expiration, 0));
+    if (!expiration || *expiration == 0)
+        throw std::runtime_error(integerError(jss::expiration, 1));
     list.expiration = *expiration;
 
     if (jv.isMember(jss::effective))
@@ -127,6 +129,9 @@ checkedList(std::string canonical, json::Value const& jv)
                 throw std::runtime_error("\"manifest\" belongs to another key than " + keyText);
         }
 
+        if (std::find(list.validators.begin(), list.validators.end(), *key) !=
+            list.validators.end())
+            throw std::runtime_error("\"validators\" lists " + keyText + " more than once");
         list.validators.push_back(*key);
     }
 

src/tools/validator-keys/SigningKeys.cpp

@@ -269,17 +269,18 @@ SigningKeys::writeToFile(std::filesystem::path const& keyFile) const
     auto const temp = fs::path(keyFile.string() + ".tmp");
     {
         std::ofstream o(temp, std::ios_base::trunc);
-        o << jv.toStyledString();
+        if (!o.fail())
+            fs::permissions(temp, fs::perms::owner_read | fs::perms::owner_write, ec);
+        if (!ec)
+            o << jv.toStyledString();
         o.close();
-        if (o.fail())
+        if (ec || o.fail())
         {
             fs::remove(temp, ec);
             throw std::runtime_error("Cannot write key file: " + keyFile.string());
         }
     }
-    fs::permissions(temp, fs::perms::owner_read | fs::perms::owner_write, ec);
-    if (!ec)
-        fs::rename(temp, keyFile, ec);
+    fs::rename(temp, keyFile, ec);
     if (ec)
     {
         fs::remove(temp, ec);

src/tools/validator-keys/doc/validator-keys-tool-guide.md

@@ -53,14 +53,7 @@ Sample output:
   # validator public key: nHUtNnLVx7odrz5dnfb2xpIgbEeJPbzJWfdicSkGyVw1eE5GpjQr
 
   [validator_token]
-  eyJ2YWxpZGF0aW9uX3NlY3J|dF9rZXkiOiI5ZWQ0NWY4NjYyNDFjYzE4YTI3NDdiNT
-  QzODdjMDYyNTkwNzk3MmY0ZTcxOTAyMzFmYWE5Mzc0NTdmYT|kYWY2IiwibWFuaWZl
-  c3QiOiJKQUFBQUFGeEllMUZ0d21pbXZHdEgyaUNjTUpxQzlnVkZLaWxHZncxL3ZDeE
-  hYWExwbGMyR25NaEFrRTFhZ3FYeEJ3RHdEYklENk9NU1l1TTBGREFscEFnTms4U0tG
-  bjdNTzJmZGtjd1JRSWhBT25ndTlzQUtxWFlvdUorbDJWMFcrc0FPa1ZCK1pSUzZQU2
-  hsSkFmVXNYZkFpQnNWSkdlc2FhZE9KYy9hQVpva1MxdnltR21WcmxIUEtXWDNZeXd1
-  NmluOEhBU1FLUHVnQkQ2N2tNYVJGR3ZtcEFUSGxHS0pkdkRGbFdQWXk1QXFEZWRGdj
-  VUSmEydzBpMjFlcTNNWXl3TFZKWm5GT3I3QzBrdzJBaVR6U0NqSXpkaXRROD0ifQ==
+  <base64 token, in 72-character lines>
 ```
 
 For a new validator, add the [validator_token] value to the xrpld config file.
@@ -91,8 +84,7 @@ Sample output:
   # validator public key: nHUtNnLVx7odrz5dnfb2xpIgbEeJPbzJWfdicSkGyVw1eE5GpjQr
 
   [validator_key_revocation]
-  JP////9xIe0hvssbqmgzFH4/NDp1z|3ShkmCtFXuC5A0IUocppHopnASQN2MuMD1Puoyjvnr
-  jQ2KJSO/2tsjRhjO6q0QQHppslQsKNSXWxjGQNIEa6nPisBOKlDDcJVZAMP4QcIyNCadzgM=
+  <base64 revocation, in 72-character lines>
 ```
 
 Add the `[validator_key_revocation]` value to this validator's config and