From 0467fd7144431872308d672586df4fbfd3949124 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Mon, 14 Sep 2026 12:51:59 -0400 Subject: [PATCH] fix: reject duplicate validators and a zero expiration; restrict output files before writing --- src/tests/tools/validator-keys/Commands.cpp | 9 +++++++++ src/tests/tools/validator-keys/ListSigning.cpp | 13 ++++++++++++- src/tools/validator-keys/Commands.cpp | 14 +++++++++----- src/tools/validator-keys/ListSigning.cpp | 9 +++++++-- src/tools/validator-keys/SigningKeys.cpp | 11 ++++++----- .../doc/validator-keys-tool-guide.md | 12 ++---------- 6 files changed, 45 insertions(+), 23 deletions(-) diff --git a/src/tests/tools/validator-keys/Commands.cpp b/src/tests/tools/validator-keys/Commands.cpp index f6cecd18a0..a1b624474b 100644 --- a/src/tests/tools/validator-keys/Commands.cpp +++ b/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"); diff --git a/src/tests/tools/validator-keys/ListSigning.cpp b/src/tests/tools/validator-keys/ListSigning.cpp index fee181380b..5fe733e775 100644 --- a/src/tests/tools/validator-keys/ListSigning.cpp +++ b/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\": []}"), diff --git a/src/tools/validator-keys/Commands.cpp b/src/tools/validator-keys/Commands.cpp index fbd839a78d..05ca6570c4 100644 --- a/src/tools/validator-keys/Commands.cpp +++ b/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 diff --git a/src/tools/validator-keys/ListSigning.cpp b/src/tools/validator-keys/ListSigning.cpp index 854e97b668..6d5c885077 100644 --- a/src/tools/validator-keys/ListSigning.cpp +++ b/src/tools/validator-keys/ListSigning.cpp @@ -9,6 +9,7 @@ #include +#include #include #include @@ -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); } diff --git a/src/tools/validator-keys/SigningKeys.cpp b/src/tools/validator-keys/SigningKeys.cpp index a765c1880d..a780fa5b5e 100644 --- a/src/tools/validator-keys/SigningKeys.cpp +++ b/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); diff --git a/src/tools/validator-keys/doc/validator-keys-tool-guide.md b/src/tools/validator-keys/doc/validator-keys-tool-guide.md index 506f6e2aaa..81588600b5 100644 --- a/src/tools/validator-keys/doc/validator-keys-tool-guide.md +++ b/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== + ``` 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= + ``` Add the `[validator_key_revocation]` value to this validator's config and