From 587505ef186c3dc1937570a5911caab851c467e2 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 17 Jul 2026 19:07:57 +0100 Subject: [PATCH 1/6] fix: Bound untrusted manifest cache --- include/xrpl/server/Manifest.h | 91 +++++++++++++++-- src/libxrpl/server/Manifest.cpp | 105 ++++++++++++++++---- src/libxrpl/server/Wallet.cpp | 21 +++- src/test/app/Manifest_test.cpp | 72 ++++++++++---- src/test/app/ValidatorList_test.cpp | 59 +++++++---- src/xrpld/app/misc/detail/ValidatorList.cpp | 10 +- src/xrpld/overlay/detail/OverlayImpl.cpp | 30 ++++-- 7 files changed, 310 insertions(+), 78 deletions(-) diff --git a/include/xrpl/server/Manifest.h b/include/xrpl/server/Manifest.h index eed1c14dae..ddd4f503fe 100644 --- a/include/xrpl/server/Manifest.h +++ b/include/xrpl/server/Manifest.h @@ -35,12 +35,15 @@ namespace xrpl { dynamically generates the signatureless form when it needs to verify the signature. - An instance of ManifestCache stores, for each trusted validator, (a) its + An instance of ManifestCache stores, for each known validator, (a) its master public key, and (b) the most senior of all valid manifests it has seen for that validator, if any. On startup, the [validator_token] config entry (which contains the manifest for this validator) is decoded and added to the manifest cache. Other manifests are added as "gossip" - received from xrpld peers. + received from xrpld peers, including ones for validators this node does not + list. Manifests for unlisted validators are capped (kMaxUntrustedCount) + so peer gossip cannot grow the cache without bound; listed validators are + not capped. Entries are never evicted, so a stored revocation is permanent. When an ephemeral key is compromised, a new signing key pair is created, along with a new manifest vouching for it (with a higher sequence number), @@ -206,7 +209,10 @@ enum class ManifestDisposition { BadEphemeralKey, /// Timely, but invalid signature - Invalid + Invalid, + + /// Unlisted and limit reached + UntrustedCapacity }; inline std::string @@ -224,11 +230,25 @@ to_string(ManifestDisposition m) return "badEphemeralKey"; case ManifestDisposition::Invalid: return "invalid"; + case ManifestDisposition::UntrustedCapacity: + return "untrustedCapacity"; default: return "unknown"; } } +/** + * Whether a manifest counts against the 'untrusted' cache cap. + * + * Passed to `ManifestCache::applyManifest` with no default, so every caller + * must choose. `Capped` is the safe, flood-resistant value; only listed or + * configured keys should use `Uncapped`. + */ +enum class ManifestRateLimitCap : std::uint8_t { + Capped, ///< Subject to the untrusted cap (unlisted peer gossip) + Uncapped ///< Bypasses the cap (listed/trusted or config manifests) +}; + class DatabaseCon; /** Remembers manifests with the highest sequence number. */ @@ -246,6 +266,38 @@ private: std::atomic seq_{0}; + /** + * Master keys of cached manifests for validators this node does not list. + * + * One entry per capped key in `map_`; its size enforces the cap below. + * A key is added when first cached under `Capped` and removed when it + * becomes listed (see `promoteToTrusted`) or an `Uncapped` update arrives, + * never re-added on de-listing. Uncapped keys are not tracked here. + */ + hash_set untrustedKeys_; + + /** + * Maximum number of untrusted master keys kept in the cache. + * + * Once reached, a manifest for a brand-new unlisted key is rejected. + */ + static constexpr std::size_t kMaxUntrustedCount = 50000; + + /** + * Running count of manifests rejected because the untrusted cap was full. + * + * Drives throttled logging (see `kUntrustedRejectCount`). Atomic because + * `applyManifest` may run concurrently. + */ + std::atomic untrustedRejectCount_{0}; + + /** + * Number of cap rejections between summary warnings. + * + * @see untrustedRejectCount_ + */ + static constexpr std::uint64_t kUntrustedRejectCount = 10000; + public: explicit ManifestCache(beast::Journal j = beast::Journal(beast::Journal::getNullSink())) : j_(j) { @@ -321,17 +373,44 @@ public: /** Add manifest to cache. + A brand-new unlisted key is rejected once the untrusted cap is full; + updates to a cached key and `Uncapped` manifests bypass the cap. The + caller decides `cap` before calling so the cache lock is not held while + consulting the validator list, which would risk a lock-ordering deadlock. + @param m Manifest to add - @return `ManifestDisposition::accepted` if successful, or - `stale` or `invalid` otherwise + @param cap `Uncapped` skips the untrusted cap; use it for keys that are + listed, configured, or loaded from the DB. Note `Uncapped` does + not assert the key is currently trusted (a DB entry may predate a + de-listing). Callers must state this explicitly so a manifest is + never left uncapped by omission. + + @return `Accepted` if stored, `Stale` if superseded, `Invalid`/ + `BadEphemeralKey` if malformed, or `UntrustedCapacity` if the + untrusted cap is full. @par Thread Safety May be called concurrently */ ManifestDisposition - applyManifest(Manifest m); + applyManifest(Manifest m, ManifestRateLimitCap cap); + + /** + * Stop counting a master key against the untrusted cap. + * + * Called when a cached untrusted key becomes listed, freeing its slot. + * Idempotent and a no-op for keys that were never counted. + * + * @param pk Master public key that is now listed/trusted + * + * @par Thread Safety + * + * May be called concurrently + */ + void + promoteToTrusted(PublicKey const& pk); /** Populate manifest cache with manifests in database and config. diff --git a/src/libxrpl/server/Manifest.cpp b/src/libxrpl/server/Manifest.cpp index b26c67e531..b18fdadbe2 100644 --- a/src/libxrpl/server/Manifest.cpp +++ b/src/libxrpl/server/Manifest.cpp @@ -377,16 +377,20 @@ ManifestCache::revoked(PublicKey const& pk) const } ManifestDisposition -ManifestCache::applyManifest(Manifest m) +ManifestCache::applyManifest(Manifest m, ManifestRateLimitCap const cap) { + bool const uncapped = cap == ManifestRateLimitCap::Uncapped; + + // The signature is checked only on the first `prewriteCheck` run (under the + // read lock). It is expensive, so `checkSignature` is cleared the first + // time it is read; the second run (under the write lock) skips it. + bool checkSignature = true; + // Check the manifest against the conditions that do not require a - // `unique_lock` (write lock) on the `mutex_`. Since the signature can be - // relatively expensive, the `checkSignature` parameter determines if the - // signature should be checked. Since `prewriteCheck` is run twice (see - // comment below), `checkSignature` only needs to be set to true on the - // first run. - auto prewriteCheck = [this, &m](auto const& iter, bool checkSignature, auto const& lock) - -> std::optional { + // `unique_lock` (write lock) on the `mutex_`. + auto prewriteCheck = [this, &m, &checkSignature]( + auto const& iter, + auto const& lock) -> std::optional { XRPL_ASSERT(lock.owns_lock(), "xrpl::ManifestCache::applyManifest::prewriteCheck : locked"); (void)lock; // not used. parameter is present to ensure the mutex is // locked when the lambda is called. @@ -401,11 +405,15 @@ ManifestCache::applyManifest(Manifest m) return ManifestDisposition::Stale; } - if (checkSignature && !m.verify()) + if (checkSignature) { - if (auto stream = j_.warn()) - logMftAct(stream, "Invalid", m.masterKey, m.sequence); - return ManifestDisposition::Invalid; + checkSignature = false; + if (!m.verify()) + { + if (auto stream = j_.warn()) + logMftAct(stream, "Invalid", m.masterKey, m.sequence); + return ManifestDisposition::Invalid; + } } // If the master key associated with a manifest is or might be @@ -465,14 +473,51 @@ ManifestCache::applyManifest(Manifest m) return std::nullopt; }; + // Reject a brand-new manifest for an unlisted key once the untrusted cap + // is full. Updates to a cached key and uncapped manifests always pass. + // Called under both the read and write lock, since the cap can be reached + // between the two. The lock param enforces that. + auto atUntrustedCap = [this, &m, uncapped](auto const& iter, auto const& lock) { + XRPL_ASSERT( + lock.owns_lock(), "xrpl::ManifestCache::applyManifest::atUntrustedCap : locked"); + (void)lock; // not used. parameter is present to ensure the mutex is + // locked when the lambda is called. + if (iter == map_.end() && !uncapped && untrustedKeys_.size() >= kMaxUntrustedCount) + { + // Log each rejection at debug, but warn only once per interval so a + // flood does not fill the log. + if (auto stream = j_.debug()) + logMftAct(stream, "UntrustedCapacity", m.masterKey, m.sequence); + if (auto const n = untrustedRejectCount_.fetch_add(1) + 1; + n % kUntrustedRejectCount == 0) + { + JLOG(j_.warn()) << "Untrusted manifest cap reached; " << n + << " manifests rejected so far"; + } + return true; + } + return false; + }; + { std::shared_lock const sl{mutex_}; - if (auto d = prewriteCheck(map_.find(m.masterKey), /*checkSig*/ true, sl)) + auto const iter = map_.find(m.masterKey); + + if (atUntrustedCap(iter, sl)) + return ManifestDisposition::UntrustedCapacity; + + if (auto d = prewriteCheck(iter, sl); d.has_value()) return *d; } std::unique_lock const sl{mutex_}; auto const iter = map_.find(m.masterKey); + + // Re-check the cap under the write lock: the cache may have grown while the + // read lock above was released. + if (atUntrustedCap(iter, sl)) + return ManifestDisposition::UntrustedCapacity; + // Since we released the previously held read lock, it's possible that the // collections have been written to. This means we need to run // `prewriteCheck` again. This re-does work, but `prewriteCheck` is @@ -482,7 +527,7 @@ ManifestCache::applyManifest(Manifest m) // doesn't need to happen again (signature checks are somewhat expensive). // Note: It's a mistake to use an upgradable lock. This is a recipe for // deadlock. - if (auto d = prewriteCheck(iter, /*checkSig*/ false, sl)) + if (auto d = prewriteCheck(iter, sl); d.has_value()) return *d; bool const revoked = m.revoked(); @@ -501,6 +546,12 @@ ManifestCache::applyManifest(Manifest m) } auto masterKey = m.masterKey; + + // Count this key against the untrusted cap. Uncapped keys (listed, + // configured, or DB-loaded) are not tracked. + if (!uncapped) + untrustedKeys_.insert(masterKey); + map_.emplace(std::move(masterKey), std::move(m)); // Something has changed. Keep track of it. @@ -514,6 +565,11 @@ ManifestCache::applyManifest(Manifest m) if (auto stream = j_.info()) logMftAct(stream, "AcceptedUpdate", m.masterKey, m.sequence, iter->second.sequence); + // If this key was counted against the cap but now arrives uncapped, free + // its slot without waiting for promoteToTrusted. + if (uncapped) + untrustedKeys_.erase(m.masterKey); + signingToMasterKeys_.erase( *iter->second.signingKey); // NOLINT(bugprone-unchecked-optional-access) prewriteCheck // ensures old manifest is not revoked @@ -521,8 +577,8 @@ ManifestCache::applyManifest(Manifest m) if (!revoked) { signingToMasterKeys_.emplace( - *m.signingKey, m.masterKey); // NOLINT(bugprone-unchecked-optional-access) non-revoked - // manifest always has signingKey + *m.signingKey, m.masterKey); // NOLINT(bugprone-unchecked-optional-access) + // non-revoked manifest always has signingKey } iter->second = std::move(m); @@ -533,6 +589,16 @@ ManifestCache::applyManifest(Manifest m) return ManifestDisposition::Accepted; } +void +ManifestCache::promoteToTrusted(PublicKey const& pk) +{ + // Frees the key's untrusted slot; a no-op (and idempotent) if the key was + // never counted. Not re-added on de-listing, so list/de-list cannot grow + // the count. + std::unique_lock const sl{mutex_}; + untrustedKeys_.erase(pk); +} + void ManifestCache::load(DatabaseCon& dbCon, std::string const& dbTable) { @@ -563,7 +629,8 @@ ManifestCache::load( JLOG(j_.warn()) << "Configured manifest revokes public key"; } - if (applyManifest(std::move(*mo)) == ManifestDisposition::Invalid) + if (applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped) == + ManifestDisposition::Invalid) { JLOG(j_.error()) << "Manifest in config was rejected"; return false; @@ -585,7 +652,9 @@ ManifestCache::load( auto mo = deserializeManifest(base64Decode(revocationStr)); - if (!mo || !mo->revoked() || applyManifest(std::move(*mo)) == ManifestDisposition::Invalid) + if (!mo || !mo->revoked() || + applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped) == + ManifestDisposition::Invalid) { JLOG(j_.error()) << "Invalid validator key revocation in config"; return false; diff --git a/src/libxrpl/server/Wallet.cpp b/src/libxrpl/server/Wallet.cpp index f3ae9dc925..f3ef3cf832 100644 --- a/src/libxrpl/server/Wallet.cpp +++ b/src/libxrpl/server/Wallet.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -75,7 +76,9 @@ getManifests( continue; } - cache.applyManifest(std::move(*mo)); + // Only trusted manifests are persisted (see saveManifests), so + // anything loaded from the DB bypasses the untrusted cap. + cache.applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped); } else { @@ -105,19 +108,27 @@ saveManifests( { soci::transaction tr(session); session << "DELETE FROM " << dbTable; + // Count skipped untrusted manifests and log one summary afterwards, since + // the cache can hold many and per-entry logging would flood at shutdown. + std::size_t skipped = 0; for (auto const& v : map) { - // Save all revocation manifests, - // but only save trusted non-revocation manifests. - if (!v.second.revoked() && !isTrusted(v.second.masterKey)) + // Persist only trusted keys. Untrusted gossip is left out so a flood + // cannot survive a restart on disk. + if (!isTrusted(v.second.masterKey)) { - JLOG(j.info()) << "Untrusted manifest in cache not saved to db"; + ++skipped; continue; } saveManifest(session, dbTable, v.second.serialized); } tr.commit(); + + if (skipped != 0) + { + JLOG(j.info()) << skipped << " untrusted manifest(s) in cache not saved to db"; + } } void diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index d559ecd7b5..c1c55c4a5d 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -399,7 +399,8 @@ public: BEAST_EXPECT( ManifestDisposition::Accepted == cache.applyManifest( - makeManifest(sk, KeyType::Ed25519, kp0.second, KeyType::Secp256k1, 0))); + makeManifest(sk, KeyType::Ed25519, kp0.second, KeyType::Secp256k1, 0), + ManifestRateLimitCap::Capped)); BEAST_EXPECT(cache.getSigningKey(pk) == kp0.first); BEAST_EXPECT(cache.getMasterKey(kp0.first) == pk); @@ -411,7 +412,8 @@ public: BEAST_EXPECT( ManifestDisposition::Accepted == cache.applyManifest( - makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 1))); + makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 1), + ManifestRateLimitCap::Capped)); BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first); BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); @@ -421,7 +423,8 @@ public: BEAST_EXPECT( ManifestDisposition::BadEphemeralKey == cache.applyManifest( - makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 2))); + makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 2), + ManifestRateLimitCap::Capped)); BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first); BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); @@ -431,7 +434,8 @@ public: // key from a revoked master public key BEAST_EXPECT( ManifestDisposition::Accepted == - cache.applyManifest(makeRevocation(sk, KeyType::Ed25519))); + cache.applyManifest( + makeRevocation(sk, KeyType::Ed25519), ManifestRateLimitCap::Capped)); BEAST_EXPECT(cache.revoked(pk)); BEAST_EXPECT(cache.getSigningKey(pk) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); @@ -902,39 +906,69 @@ public: // applyManifest should accept new manifests with // higher sequence numbers auto const seq0 = cache.sequence(); - BEAST_EXPECT(cache.applyManifest(clone(sA0)) == ManifestDisposition::Accepted); + BEAST_EXPECT( + cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); BEAST_EXPECT(cache.sequence() > seq0); auto const seq1 = cache.sequence(); - BEAST_EXPECT(cache.applyManifest(clone(sA0)) == ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); BEAST_EXPECT(cache.sequence() == seq1); - BEAST_EXPECT(cache.applyManifest(clone(sA1)) == ManifestDisposition::Accepted); - BEAST_EXPECT(cache.applyManifest(clone(sA1)) == ManifestDisposition::Stale); - BEAST_EXPECT(cache.applyManifest(clone(sA0)) == ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); + BEAST_EXPECT( + cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); - BEAST_EXPECT(cache.applyManifest(clone(sA2)) == ManifestDisposition::BadEphemeralKey); + BEAST_EXPECT( + cache.applyManifest(clone(sA2), ManifestRateLimitCap::Capped) == + ManifestDisposition::BadEphemeralKey); // applyManifest should accept manifests with max sequence numbers // that revoke the master public key BEAST_EXPECT(!cache.revoked(pkA)); BEAST_EXPECT(sAMax.revoked()); - BEAST_EXPECT(cache.applyManifest(clone(sAMax)) == ManifestDisposition::Accepted); - BEAST_EXPECT(cache.applyManifest(clone(sAMax)) == ManifestDisposition::Stale); - BEAST_EXPECT(cache.applyManifest(clone(sA1)) == ManifestDisposition::Stale); - BEAST_EXPECT(cache.applyManifest(clone(sA0)) == ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sAMax), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); + BEAST_EXPECT( + cache.applyManifest(clone(sAMax), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); BEAST_EXPECT(cache.revoked(pkA)); // applyManifest should reject manifests with invalid signatures - BEAST_EXPECT(cache.applyManifest(clone(sB0)) == ManifestDisposition::Accepted); - BEAST_EXPECT(cache.applyManifest(clone(sB0)) == ManifestDisposition::Stale); + BEAST_EXPECT( + cache.applyManifest(clone(sB0), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); + BEAST_EXPECT( + cache.applyManifest(clone(sB0), ManifestRateLimitCap::Capped) == + ManifestDisposition::Stale); BEAST_EXPECT(!deserializeManifest(fake)); - BEAST_EXPECT(cache.applyManifest(clone(sB1)) == ManifestDisposition::Invalid); - BEAST_EXPECT(cache.applyManifest(clone(sB2)) == ManifestDisposition::Accepted); + BEAST_EXPECT( + cache.applyManifest(clone(sB1), ManifestRateLimitCap::Capped) == + ManifestDisposition::Invalid); + BEAST_EXPECT( + cache.applyManifest(clone(sB2), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); auto const sC0 = makeManifest( kpB2.second, KeyType::Ed25519, randomSecretKey(), KeyType::Ed25519, 47); - BEAST_EXPECT(cache.applyManifest(clone(sC0)) == ManifestDisposition::BadMasterKey); + BEAST_EXPECT( + cache.applyManifest(clone(sC0), ManifestRateLimitCap::Capped) == + ManifestDisposition::BadMasterKey); } testLoadStore(cache); diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 20a3557db5..3fede86637 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -277,8 +277,10 @@ private: trustedKeys->load(localSigningPublicOuter, emptyCfgKeys, emptyCfgPublishers)); BEAST_EXPECT(trustedKeys->listed(localSigningPublicOuter)); - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - manifests.applyManifest(*deserializeManifest(cfgManifest)); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + manifests.applyManifest( + *deserializeManifest(cfgManifest), ManifestRateLimitCap::Capped); + // NOLINTEND(bugprone-unchecked-optional-access) BEAST_EXPECT( trustedKeys->load(localSigningPublicOuter, emptyCfgKeys, emptyCfgPublishers)); @@ -368,8 +370,10 @@ private: app.config().legacy("database_path"), env.journal); - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - manifests.applyManifest(*deserializeManifest(cfgManifest)); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + manifests.applyManifest( + *deserializeManifest(cfgManifest), ManifestRateLimitCap::Capped); + // NOLINTEND(bugprone-unchecked-optional-access) BEAST_EXPECT(trustedKeys->load(localSigningPublicOuter, cfgKeys, emptyCfgPublishers)); @@ -454,13 +458,16 @@ private: auto const pubRevokedSigning = randomKeyPair(KeyType::Secp256k1); // make this manifest revoked (seq num = max) // -- thus should not be loaded - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - pubManifests.applyManifest(*deserializeManifest(makeManifestString( - pubRevokedPublic, - pubRevokedSecret, - pubRevokedSigning.first, - pubRevokedSigning.second, - std::numeric_limits::max()))); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + pubManifests.applyManifest( + *deserializeManifest(makeManifestString( + pubRevokedPublic, + pubRevokedSecret, + pubRevokedSigning.first, + pubRevokedSigning.second, + std::numeric_limits::max())), + ManifestRateLimitCap::Capped); + // NOLINTEND(bugprone-unchecked-optional-access) // these two are not revoked (and not in the manifest cache at all.) auto legitKey1 = randomMasterKey(); @@ -493,13 +500,16 @@ private: auto const pubRevokedSigning = randomKeyPair(KeyType::Secp256k1); // make this manifest revoked (seq num = max) // -- thus should not be loaded - // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - pubManifests.applyManifest(*deserializeManifest(makeManifestString( - pubRevokedPublic, - pubRevokedSecret, - pubRevokedSigning.first, - pubRevokedSigning.second, - std::numeric_limits::max()))); + // NOLINTBEGIN(bugprone-unchecked-optional-access) + pubManifests.applyManifest( + *deserializeManifest(makeManifestString( + pubRevokedPublic, + pubRevokedSecret, + pubRevokedSigning.first, + pubRevokedSigning.second, + std::numeric_limits::max())), + ManifestRateLimitCap::Capped); + // NOLINTEND(bugprone-unchecked-optional-access) // this one is not revoked (and not in the manifest cache at all.) auto legitKey = randomMasterKey(); @@ -1164,7 +1174,8 @@ private: BEAST_EXPECT( // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - manifestsOuter.applyManifest(std::move(*m1)) == ManifestDisposition::Accepted); + manifestsOuter.applyManifest(std::move(*m1), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); BEAST_EXPECT(trustedKeysOuter->listed(masterPublic)); BEAST_EXPECT(trustedKeysOuter->trusted(masterPublic)); BEAST_EXPECT(trustedKeysOuter->listed(signingPublic1)); @@ -1178,7 +1189,8 @@ private: masterPublic, masterPrivate, signingPublic2, signingKeys2.second, 2)); BEAST_EXPECT( // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - manifestsOuter.applyManifest(std::move(*m2)) == ManifestDisposition::Accepted); + manifestsOuter.applyManifest(std::move(*m2), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); BEAST_EXPECT(trustedKeysOuter->listed(masterPublic)); BEAST_EXPECT(trustedKeysOuter->trusted(masterPublic)); BEAST_EXPECT(trustedKeysOuter->listed(signingPublic2)); @@ -1195,7 +1207,8 @@ private: // NOLINTBEGIN(bugprone-unchecked-optional-access) BEAST_EXPECT(max->revoked()); BEAST_EXPECT( - manifestsOuter.applyManifest(std::move(*max)) == ManifestDisposition::Accepted); + manifestsOuter.applyManifest(std::move(*max), ManifestRateLimitCap::Capped) == + ManifestDisposition::Accepted); // NOLINTEND(bugprone-unchecked-optional-access) BEAST_EXPECT(manifestsOuter.getSigningKey(masterPublic) == masterPublic); @@ -2612,7 +2625,9 @@ private: auto threshold = listThreshold > 0 ? std::optional(listThreshold) : std::nullopt; if (self) { - valManifests.applyManifest(*deserializeManifest(base64Decode(self->manifest))); + valManifests.applyManifest( + *deserializeManifest(base64Decode(self->manifest)), + ManifestRateLimitCap::Capped); BEAST_EXPECT( result->load(self->signingPublic, emptyCfgKeys, cfgPublishers, threshold)); } diff --git a/src/xrpld/app/misc/detail/ValidatorList.cpp b/src/xrpld/app/misc/detail/ValidatorList.cpp index 57b65814e1..ce07e0ec77 100644 --- a/src/xrpld/app/misc/detail/ValidatorList.cpp +++ b/src/xrpld/app/misc/detail/ValidatorList.cpp @@ -1065,6 +1065,8 @@ ValidatorList::updatePublisherList( { // Increment list count for added keys ++keyListings_[*iNew]; + // Key is now listed: free its untrusted slot if it had one. + validatorManifests_.promoteToTrusted(*iNew); ++iNew; } else if (iNew == publisherList.end() || (iOld != oldList.end() && *iOld < *iNew)) @@ -1103,7 +1105,8 @@ ValidatorList::updatePublisherList( continue; } - if (auto const r = validatorManifests_.applyManifest(std::move(*m)); + if (auto const r = + validatorManifests_.applyManifest(std::move(*m), ManifestRateLimitCap::Uncapped); r == ManifestDisposition::Invalid) { JLOG(j_.warn()) << "List for " << strHex(pubKey) @@ -1348,7 +1351,10 @@ ValidatorList::verify( PublicKey masterPubKey = manifest.masterKey; auto const revoked = manifest.revoked(); - auto const result = publisherManifests_.applyManifest(std::move(manifest)); + // Publisher keys are configured/trusted (checked above), so bypass the + // untrusted cap. + auto const result = + publisherManifests_.applyManifest(std::move(manifest), ManifestRateLimitCap::Uncapped); if (revoked && result == ManifestDisposition::Accepted) { diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index b31f54058a..452d0f0339 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -673,13 +673,22 @@ OverlayImpl::onManifests( if (auto mo = deserializeManifest(s)) { auto const serialized = mo->serialized; + // Resolve trust before applyManifest takes the manifest-cache + // lock: listed() takes the validator-list lock, so ordering it + // first avoids holding the two locks in opposite orders. + bool const isTrusted = app_.getValidators().listed(mo->masterKey); + // Updates to a known key are relayed even when untrusted. Use + // getSequence, not getManifest, to avoid copying the cached payload + // on this hot path. + bool const isKnown = + app_.getValidatorManifests().getSequence(mo->masterKey).has_value(); - auto const result = app_.getValidatorManifests().applyManifest(std::move(*mo)); + auto const result = app_.getValidatorManifests().applyManifest( + std::move(*mo), + isTrusted ? ManifestRateLimitCap::Uncapped : ManifestRateLimitCap::Capped); if (result == ManifestDisposition::Accepted) { - relay.add_list()->set_stobject(s); - // N.B.: this is important; the applyManifest call above moves // the loaded Manifest out of the optional so we need to // reload it here. @@ -691,10 +700,19 @@ OverlayImpl::onManifests( // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above app_.getOPs().pubManifest(*mo); - if (app_.getValidators().listed(mo->masterKey)) + // Relay only trusted manifests or updates to known keys, so + // untrusted gossip for a brand-new key cannot be amplified. + // Persist to the wallet DB only for trusted keys, so untrusted + // gossip never survives a restart. + if (isTrusted || isKnown) { - auto db = app_.getWalletDB().checkoutDb(); - addValidatorManifest(*db, serialized); + relay.add_list()->set_stobject(s); + + if (isTrusted) + { + auto db = app_.getWalletDB().checkoutDb(); + addValidatorManifest(*db, serialized); + } } // NOLINTEND(bugprone-unchecked-optional-access) } From 32a9cc4038e62ee87c7a2eb5e03f1f028507a8a7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 23 Jul 2026 21:27:45 +0100 Subject: [PATCH 2/6] fix: Reduce untrusted manifest cache cap to 100 --- include/xrpl/server/Manifest.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/xrpl/server/Manifest.h b/include/xrpl/server/Manifest.h index ddd4f503fe..70fc8f6f8f 100644 --- a/include/xrpl/server/Manifest.h +++ b/include/xrpl/server/Manifest.h @@ -281,7 +281,7 @@ private: * * Once reached, a manifest for a brand-new unlisted key is rejected. */ - static constexpr std::size_t kMaxUntrustedCount = 50000; + static constexpr std::size_t kMaxUntrustedCount = 100; /** * Running count of manifests rejected because the untrusted cap was full. From 0cce5a06d994cb1c45419e2b7016843ba5817748 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 16 Jul 2026 16:21:56 -0400 Subject: [PATCH 3/6] fix: Reject oversized validator manifest before decoding --- include/xrpl/basics/base64.h | 29 ++++++++++++ include/xrpl/server/Manifest.h | 52 ++++++++++++++++++--- src/libxrpl/basics/base64.cpp | 14 ------ src/libxrpl/server/Manifest.cpp | 5 ++ src/xrpld/app/misc/detail/ValidatorList.cpp | 9 ++++ 5 files changed, 88 insertions(+), 21 deletions(-) diff --git a/include/xrpl/basics/base64.h b/include/xrpl/basics/base64.h index ed30e40a36..6b4cd26604 100644 --- a/include/xrpl/basics/base64.h +++ b/include/xrpl/basics/base64.h @@ -39,6 +39,35 @@ namespace xrpl { +namespace base64 { + +/** + * Returns the maximum number of characters needed to base64-encode @p nBytes bytes. + * + * @param nBytes Number of input bytes. + * @return Size of the encoded string, including padding. + */ +constexpr std::size_t +encodedSize(std::size_t const nBytes) +{ + return 4 * ((nBytes + 2) / 3); +} + +/** + * Returns the maximum number of bytes a base64 string of @p nChars characters + * decodes to. + * + * @param nChars Number of base64 characters. + * @return Upper bound on the number of decoded bytes. + */ +constexpr std::size_t +decodedSize(std::size_t const nChars) +{ + return ((nChars / 4) * 3) + 2; +} + +} // namespace base64 + std::string base64Encode(std::uint8_t const* data, std::size_t len); diff --git a/include/xrpl/server/Manifest.h b/include/xrpl/server/Manifest.h index 70fc8f6f8f..fe5f1db985 100644 --- a/include/xrpl/server/Manifest.h +++ b/include/xrpl/server/Manifest.h @@ -1,10 +1,16 @@ #pragma once #include +#include +#include #include #include #include +#include +#include +#include +#include #include #include #include @@ -135,15 +141,47 @@ struct Manifest std::string to_string(Manifest const& m); -/** Constructs Manifest from serialized string +/** + *Largest a valid manifest can be, in decoded bytes. + * + * A manifest has a fixed set of fields. Each is serialized as a field header + * (1-2 bytes), an optional length prefix (1 byte for these sizes), and the + * field body. Taking every field at its largest gives the maximum below, so + * anything larger cannot be a valid manifest. + * + * Field header + length + body = bytes + * sfVersion (U16) 2 0 2 4 + * sfSequence (U32) 1 0 4 5 + * sfPublicKey (33) 1 1 33 35 + * sfSigningPubKey (33) 1 1 33 35 + * sfSignature (72) 1 1 72 74 + * sfMasterSignature (72) 2 1 72 75 + * sfDomain (128) 1 1 128 130 + * ----- + * 358 + */ +constexpr std::size_t kMaxManifestBytes = 358; - @param s Serialized manifest string +/** + * Largest a valid manifest can be, in base64 characters. + * + * base64 encodes 3 bytes as 4 characters, so this is the encoded form of + * @ref kMaxManifestBytes. Callers that receive a base64 manifest should + * reject anything longer than this before decoding, to avoid allocating + * memory for an oversized input. + */ +constexpr std::size_t kMaxManifestBase64 = base64::encodedSize(kMaxManifestBytes); - @return `std::nullopt` if string is invalid - - @note This does not verify manifest signatures. - `Manifest::verify` should be called after constructing manifest. -*/ +/** + * Constructs Manifest from serialized string + * + * @param s Serialized manifest string + * + * @return `std::nullopt` if string is invalid + * + * @note This does not verify manifest signatures. + * `Manifest::verify` should be called after constructing manifest. + */ /** @{ */ std::optional deserializeManifest(Slice s, beast::Journal journal); diff --git a/src/libxrpl/basics/base64.cpp b/src/libxrpl/basics/base64.cpp index 541ddd0839..7772752f40 100644 --- a/src/libxrpl/basics/base64.cpp +++ b/src/libxrpl/basics/base64.cpp @@ -76,20 +76,6 @@ getInverse() return &kTab[0]; } -/// Returns max chars needed to encode a base64 string -constexpr std::size_t -encodedSize(std::size_t n) -{ - return 4 * ((n + 2) / 3); -} - -/// Returns max bytes needed to decode a base64 string -constexpr std::size_t -decodedSize(std::size_t n) -{ - return ((n / 4) * 3) + 2; -} - /** Encode a series of octets as a padded, base64 string. The resulting string will not be null terminated. diff --git a/src/libxrpl/server/Manifest.cpp b/src/libxrpl/server/Manifest.cpp index b18fdadbe2..b34955dc28 100644 --- a/src/libxrpl/server/Manifest.cpp +++ b/src/libxrpl/server/Manifest.cpp @@ -62,6 +62,11 @@ deserializeManifest(Slice s, beast::Journal journal) if (s.empty()) return std::nullopt; + // A valid manifest has a fixed maximum size, so reject anything larger + // before parsing it. + if (s.size() > kMaxManifestBytes) + return std::nullopt; + static SOTemplate const kManifestFormat{ // A manifest must include: // - the master public key diff --git a/src/xrpld/app/misc/detail/ValidatorList.cpp b/src/xrpld/app/misc/detail/ValidatorList.cpp index ce07e0ec77..5c95eb553c 100644 --- a/src/xrpld/app/misc/detail/ValidatorList.cpp +++ b/src/xrpld/app/misc/detail/ValidatorList.cpp @@ -1130,6 +1130,15 @@ ValidatorList::applyList( json::Value list; auto const& manifest = localManifest ? *localManifest : globalManifest; + // Reject an oversized manifest before decoding it, so we do not allocate + // memory for an input that cannot be a valid manifest. deserializeManifest + // also enforces the decoded-byte limit, but checking here avoids the + // base64 decode entirely. + if (manifest.size() > kMaxManifestBase64) + { + JLOG(j_.warn()) << "UNL manifest exceeds maximum size"; + return PublisherListStats{ListDisposition::Invalid}; + } auto m = deserializeManifest(base64Decode(manifest)); if (!m) { From 4bd1d1ca2f01952b9ef533bc4bd8abc2046bfce8 Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:18:28 +0100 Subject: [PATCH 4/6] fix: Cap untrusted manifests per message and drop oversized ones Bound the number of manifests carried in a single TMManifests message (kMaxManifestsPerMessage). Trusted manifests are always included and processed; untrusted gossip is capped per message on both send and receive, and the sender is charged only when untrusted entries are actually skipped. Oversized TMManifests messages are dropped without penalty at the protocol layer so an unpatched peer is not disconnected. Complements the cache bound from #276/#323. --- include/xrpl/basics/base64.h | 8 +- include/xrpl/server/Manifest.h | 45 ++++++---- src/libxrpl/server/Manifest.cpp | 8 +- src/libxrpl/server/Wallet.cpp | 2 +- src/test/app/Manifest_test.cpp | 38 ++++----- src/test/app/ValidatorList_test.cpp | 16 ++-- src/xrpld/app/misc/detail/ValidatorList.cpp | 8 +- src/xrpld/overlay/Message.h | 8 ++ src/xrpld/overlay/detail/OverlayImpl.cpp | 95 +++++++++++++++++++-- src/xrpld/overlay/detail/PeerImp.cpp | 4 + src/xrpld/overlay/detail/ProtocolMessage.h | 10 +++ 11 files changed, 178 insertions(+), 64 deletions(-) diff --git a/include/xrpl/basics/base64.h b/include/xrpl/basics/base64.h index 6b4cd26604..34b2cbc5a0 100644 --- a/include/xrpl/basics/base64.h +++ b/include/xrpl/basics/base64.h @@ -54,16 +54,16 @@ encodedSize(std::size_t const nBytes) } /** - * Returns the maximum number of bytes a base64 string of @p nChars characters + * Returns the maximum number of bytes a base64 string of @p numChars characters * decodes to. * - * @param nChars Number of base64 characters. + * @param numChars Number of base64 characters. * @return Upper bound on the number of decoded bytes. */ constexpr std::size_t -decodedSize(std::size_t const nChars) +decodedSize(std::size_t const numChars) { - return ((nChars / 4) * 3) + 2; + return ((numChars / 4) * 3) + 2; } } // namespace base64 diff --git a/include/xrpl/server/Manifest.h b/include/xrpl/server/Manifest.h index fe5f1db985..eec07ac3ee 100644 --- a/include/xrpl/server/Manifest.h +++ b/include/xrpl/server/Manifest.h @@ -47,8 +47,8 @@ namespace xrpl { entry (which contains the manifest for this validator) is decoded and added to the manifest cache. Other manifests are added as "gossip" received from xrpld peers, including ones for validators this node does not - list. Manifests for unlisted validators are capped (kMaxUntrustedCount) - so peer gossip cannot grow the cache without bound; listed validators are + trust. Manifests for untrusted validators are capped (kMaxUntrustedCount) + so peer gossip cannot grow the cache without bound; trusted validators are not capped. Entries are never evicted, so a stored revocation is permanent. When an ephemeral key is compromised, a new signing key pair is created, @@ -149,7 +149,7 @@ to_string(Manifest const& m); * field body. Taking every field at its largest gives the maximum below, so * anything larger cannot be a valid manifest. * - * Field header + length + body = bytes + * Field header + length + body = bytes * sfVersion (U16) 2 0 2 4 * sfSequence (U32) 1 0 4 5 * sfPublicKey (33) 1 1 33 35 @@ -172,16 +172,31 @@ constexpr std::size_t kMaxManifestBytes = 358; */ constexpr std::size_t kMaxManifestBase64 = base64::encodedSize(kMaxManifestBytes); -/** - * Constructs Manifest from serialized string - * - * @param s Serialized manifest string - * - * @return `std::nullopt` if string is invalid - * - * @note This does not verify manifest signatures. - * `Manifest::verify` should be called after constructing manifest. - */ +/** Maximum number of manifests carried in a single TMManifests message. + + Outbound, the TMManifests message sent to a peer includes every trusted + manifest and fills the rest of this budget with untrusted gossip, so it + never exceeds this size. Inbound, trusted manifests are always processed + and untrusted ones are processed up to this many, so a peer sending its + whole cache cannot force unbounded work. + + The trusted set is tiny relative to this bound, so trusted manifests are + not dropped in practice. This is a transitional per-message cap; the cache + already bounds untrusted manifests (see kMaxUntrustedCount), so it is no + longer needed once the network has upgraded past nodes that send their + whole cache in one message. +*/ +constexpr std::size_t kMaxManifestsPerMessage = 200; + +/** Constructs Manifest from serialized string + + @param s Serialized manifest string + + @return `std::nullopt` if string is invalid + + @note This does not verify manifest signatures. + `Manifest::verify` should be called after constructing manifest. +*/ /** @{ */ std::optional deserializeManifest(Slice s, beast::Journal journal); @@ -282,7 +297,7 @@ to_string(ManifestDisposition m) * must choose. `Capped` is the safe, flood-resistant value; only listed or * configured keys should use `Uncapped`. */ -enum class ManifestRateLimitCap : std::uint8_t { +enum class ManifestRateLimitCapPolicy : std::uint8_t { Capped, ///< Subject to the untrusted cap (unlisted peer gossip) Uncapped ///< Bypasses the cap (listed/trusted or config manifests) }; @@ -433,7 +448,7 @@ public: May be called concurrently */ ManifestDisposition - applyManifest(Manifest m, ManifestRateLimitCap cap); + applyManifest(Manifest m, ManifestRateLimitCapPolicy cap); /** * Stop counting a master key against the untrusted cap. diff --git a/src/libxrpl/server/Manifest.cpp b/src/libxrpl/server/Manifest.cpp index b34955dc28..3da3f9e9cd 100644 --- a/src/libxrpl/server/Manifest.cpp +++ b/src/libxrpl/server/Manifest.cpp @@ -382,9 +382,9 @@ ManifestCache::revoked(PublicKey const& pk) const } ManifestDisposition -ManifestCache::applyManifest(Manifest m, ManifestRateLimitCap const cap) +ManifestCache::applyManifest(Manifest m, ManifestRateLimitCapPolicy const cap) { - bool const uncapped = cap == ManifestRateLimitCap::Uncapped; + bool const uncapped = cap == ManifestRateLimitCapPolicy::Uncapped; // The signature is checked only on the first `prewriteCheck` run (under the // read lock). It is expensive, so `checkSignature` is cleared the first @@ -634,7 +634,7 @@ ManifestCache::load( JLOG(j_.warn()) << "Configured manifest revokes public key"; } - if (applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped) == + if (applyManifest(std::move(*mo), ManifestRateLimitCapPolicy::Uncapped) == ManifestDisposition::Invalid) { JLOG(j_.error()) << "Manifest in config was rejected"; @@ -658,7 +658,7 @@ ManifestCache::load( auto mo = deserializeManifest(base64Decode(revocationStr)); if (!mo || !mo->revoked() || - applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped) == + applyManifest(std::move(*mo), ManifestRateLimitCapPolicy::Uncapped) == ManifestDisposition::Invalid) { JLOG(j_.error()) << "Invalid validator key revocation in config"; diff --git a/src/libxrpl/server/Wallet.cpp b/src/libxrpl/server/Wallet.cpp index f3ef3cf832..ac5f0ace76 100644 --- a/src/libxrpl/server/Wallet.cpp +++ b/src/libxrpl/server/Wallet.cpp @@ -78,7 +78,7 @@ getManifests( // Only trusted manifests are persisted (see saveManifests), so // anything loaded from the DB bypasses the untrusted cap. - cache.applyManifest(std::move(*mo), ManifestRateLimitCap::Uncapped); + cache.applyManifest(std::move(*mo), ManifestRateLimitCapPolicy::Uncapped); } else { diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index c1c55c4a5d..9218b073d4 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -400,7 +400,7 @@ public: ManifestDisposition::Accepted == cache.applyManifest( makeManifest(sk, KeyType::Ed25519, kp0.second, KeyType::Secp256k1, 0), - ManifestRateLimitCap::Capped)); + ManifestRateLimitCapPolicy::Capped)); BEAST_EXPECT(cache.getSigningKey(pk) == kp0.first); BEAST_EXPECT(cache.getMasterKey(kp0.first) == pk); @@ -413,7 +413,7 @@ public: ManifestDisposition::Accepted == cache.applyManifest( makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 1), - ManifestRateLimitCap::Capped)); + ManifestRateLimitCapPolicy::Capped)); BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first); BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); @@ -424,7 +424,7 @@ public: ManifestDisposition::BadEphemeralKey == cache.applyManifest( makeManifest(sk, KeyType::Ed25519, kp1.second, KeyType::Secp256k1, 2), - ManifestRateLimitCap::Capped)); + ManifestRateLimitCapPolicy::Capped)); BEAST_EXPECT(cache.getSigningKey(pk) == kp1.first); BEAST_EXPECT(cache.getMasterKey(kp1.first) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); @@ -435,7 +435,7 @@ public: BEAST_EXPECT( ManifestDisposition::Accepted == cache.applyManifest( - makeRevocation(sk, KeyType::Ed25519), ManifestRateLimitCap::Capped)); + makeRevocation(sk, KeyType::Ed25519), ManifestRateLimitCapPolicy::Capped)); BEAST_EXPECT(cache.revoked(pk)); BEAST_EXPECT(cache.getSigningKey(pk) == pk); BEAST_EXPECT(cache.getMasterKey(kp0.first) == kp0.first); @@ -907,28 +907,28 @@ public: // higher sequence numbers auto const seq0 = cache.sequence(); BEAST_EXPECT( - cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); BEAST_EXPECT(cache.sequence() > seq0); auto const seq1 = cache.sequence(); BEAST_EXPECT( - cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT(cache.sequence() == seq1); BEAST_EXPECT( - cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA1), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); BEAST_EXPECT( - cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA1), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT( - cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT( - cache.applyManifest(clone(sA2), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA2), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::BadEphemeralKey); // applyManifest should accept manifests with max sequence numbers @@ -936,38 +936,38 @@ public: BEAST_EXPECT(!cache.revoked(pkA)); BEAST_EXPECT(sAMax.revoked()); BEAST_EXPECT( - cache.applyManifest(clone(sAMax), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sAMax), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); BEAST_EXPECT( - cache.applyManifest(clone(sAMax), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sAMax), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT( - cache.applyManifest(clone(sA1), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA1), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT( - cache.applyManifest(clone(sA0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sA0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT(cache.revoked(pkA)); // applyManifest should reject manifests with invalid signatures BEAST_EXPECT( - cache.applyManifest(clone(sB0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sB0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); BEAST_EXPECT( - cache.applyManifest(clone(sB0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sB0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Stale); BEAST_EXPECT(!deserializeManifest(fake)); BEAST_EXPECT( - cache.applyManifest(clone(sB1), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sB1), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Invalid); BEAST_EXPECT( - cache.applyManifest(clone(sB2), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sB2), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); auto const sC0 = makeManifest( kpB2.second, KeyType::Ed25519, randomSecretKey(), KeyType::Ed25519, 47); BEAST_EXPECT( - cache.applyManifest(clone(sC0), ManifestRateLimitCap::Capped) == + cache.applyManifest(clone(sC0), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::BadMasterKey); } diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 3fede86637..d39789f183 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -279,7 +279,7 @@ private: // NOLINTBEGIN(bugprone-unchecked-optional-access) manifests.applyManifest( - *deserializeManifest(cfgManifest), ManifestRateLimitCap::Capped); + *deserializeManifest(cfgManifest), ManifestRateLimitCapPolicy::Capped); // NOLINTEND(bugprone-unchecked-optional-access) BEAST_EXPECT( trustedKeys->load(localSigningPublicOuter, emptyCfgKeys, emptyCfgPublishers)); @@ -372,7 +372,7 @@ private: // NOLINTBEGIN(bugprone-unchecked-optional-access) manifests.applyManifest( - *deserializeManifest(cfgManifest), ManifestRateLimitCap::Capped); + *deserializeManifest(cfgManifest), ManifestRateLimitCapPolicy::Capped); // NOLINTEND(bugprone-unchecked-optional-access) BEAST_EXPECT(trustedKeys->load(localSigningPublicOuter, cfgKeys, emptyCfgPublishers)); @@ -466,7 +466,7 @@ private: pubRevokedSigning.first, pubRevokedSigning.second, std::numeric_limits::max())), - ManifestRateLimitCap::Capped); + ManifestRateLimitCapPolicy::Capped); // NOLINTEND(bugprone-unchecked-optional-access) // these two are not revoked (and not in the manifest cache at all.) @@ -508,7 +508,7 @@ private: pubRevokedSigning.first, pubRevokedSigning.second, std::numeric_limits::max())), - ManifestRateLimitCap::Capped); + ManifestRateLimitCapPolicy::Capped); // NOLINTEND(bugprone-unchecked-optional-access) // this one is not revoked (and not in the manifest cache at all.) @@ -1174,7 +1174,7 @@ private: BEAST_EXPECT( // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - manifestsOuter.applyManifest(std::move(*m1), ManifestRateLimitCap::Capped) == + manifestsOuter.applyManifest(std::move(*m1), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); BEAST_EXPECT(trustedKeysOuter->listed(masterPublic)); BEAST_EXPECT(trustedKeysOuter->trusted(masterPublic)); @@ -1189,7 +1189,7 @@ private: masterPublic, masterPrivate, signingPublic2, signingKeys2.second, 2)); BEAST_EXPECT( // NOLINTNEXTLINE(bugprone-unchecked-optional-access) - manifestsOuter.applyManifest(std::move(*m2), ManifestRateLimitCap::Capped) == + manifestsOuter.applyManifest(std::move(*m2), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); BEAST_EXPECT(trustedKeysOuter->listed(masterPublic)); BEAST_EXPECT(trustedKeysOuter->trusted(masterPublic)); @@ -1207,7 +1207,7 @@ private: // NOLINTBEGIN(bugprone-unchecked-optional-access) BEAST_EXPECT(max->revoked()); BEAST_EXPECT( - manifestsOuter.applyManifest(std::move(*max), ManifestRateLimitCap::Capped) == + manifestsOuter.applyManifest(std::move(*max), ManifestRateLimitCapPolicy::Capped) == ManifestDisposition::Accepted); // NOLINTEND(bugprone-unchecked-optional-access) @@ -2627,7 +2627,7 @@ private: { valManifests.applyManifest( *deserializeManifest(base64Decode(self->manifest)), - ManifestRateLimitCap::Capped); + ManifestRateLimitCapPolicy::Capped); BEAST_EXPECT( result->load(self->signingPublic, emptyCfgKeys, cfgPublishers, threshold)); } diff --git a/src/xrpld/app/misc/detail/ValidatorList.cpp b/src/xrpld/app/misc/detail/ValidatorList.cpp index 5c95eb553c..0e29149e9c 100644 --- a/src/xrpld/app/misc/detail/ValidatorList.cpp +++ b/src/xrpld/app/misc/detail/ValidatorList.cpp @@ -1105,8 +1105,8 @@ ValidatorList::updatePublisherList( continue; } - if (auto const r = - validatorManifests_.applyManifest(std::move(*m), ManifestRateLimitCap::Uncapped); + if (auto const r = validatorManifests_.applyManifest( + std::move(*m), ManifestRateLimitCapPolicy::Uncapped); r == ManifestDisposition::Invalid) { JLOG(j_.warn()) << "List for " << strHex(pubKey) @@ -1362,8 +1362,8 @@ ValidatorList::verify( // Publisher keys are configured/trusted (checked above), so bypass the // untrusted cap. - auto const result = - publisherManifests_.applyManifest(std::move(manifest), ManifestRateLimitCap::Uncapped); + auto const result = publisherManifests_.applyManifest( + std::move(manifest), ManifestRateLimitCapPolicy::Uncapped); if (revoked && result == ManifestDisposition::Accepted) { diff --git a/src/xrpld/overlay/Message.h b/src/xrpld/overlay/Message.h index cd21ca40c6..30c30a5f2c 100644 --- a/src/xrpld/overlay/Message.h +++ b/src/xrpld/overlay/Message.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -13,6 +14,13 @@ namespace xrpl { constexpr std::size_t kMaximumMessageSize = megabytes(64); +// Upper bound on the wire size of a TMManifests message: kMaxManifestsPerMessage entries +// of at most kMaxManifestBytes each, plus a small allowance for protobuf +// framing per entry. +constexpr std::size_t kManifestFramingBytes = 8; +constexpr std::size_t kMaximumManifestsMessageSize = + kMaxManifestsPerMessage * (kMaxManifestBytes + kManifestFramingBytes); + // VFALCO NOTE If we forward declare Message and write out shared_ptr // instead of using the in-class type alias, we can remove the // entire ripple.pb.h from the main headers. diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index 452d0f0339..a82f7a286d 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include #include #include @@ -661,12 +662,17 @@ OverlayImpl::onManifests( std::shared_ptr const& m, std::shared_ptr const& from) { - auto const n = m->list_size(); auto const& journal = from->pJournal(); + // Process every trusted manifest, but stop processing untrusted ones once + // kMaxManifestsPerMessage of them have been handled, so the work stays bounded. + auto const total = static_cast(m->list_size()); + std::size_t untrusted = 0; + bool skippedUntrusted = false; + protocol::TMManifests relay; - for (std::size_t i = 0; i < n; ++i) + for (std::size_t i = 0; i < total; ++i) { auto& s = m->list().Get(i).stobject(); @@ -677,6 +683,19 @@ OverlayImpl::onManifests( // lock: listed() takes the validator-list lock, so ordering it // first avoids holding the two locks in opposite orders. bool const isTrusted = app_.getValidators().listed(mo->masterKey); + + // Bound untrusted work: process at most kMaxManifestsPerMessage + // untrusted manifests, but never skip a trusted one. Trusted + // manifests are not counted against the cap. + if (!isTrusted) + { + if (untrusted >= kMaxManifestsPerMessage) + { + skippedUntrusted = true; + continue; + } + ++untrusted; + } // Updates to a known key are relayed even when untrusted. Use // getSequence, not getManifest, to avoid copying the cached payload // on this hot path. @@ -685,7 +704,8 @@ OverlayImpl::onManifests( auto const result = app_.getValidatorManifests().applyManifest( std::move(*mo), - isTrusted ? ManifestRateLimitCap::Uncapped : ManifestRateLimitCap::Capped); + isTrusted ? ManifestRateLimitCapPolicy::Uncapped + : ManifestRateLimitCapPolicy::Capped); if (result == ManifestDisposition::Accepted) { @@ -724,6 +744,18 @@ OverlayImpl::onManifests( } } + if (skippedUntrusted) + { + // The sender exceeded the untrusted per-message cap. Charge it (once, + // here) so a flood of untrusted manifests is penalized, while an honest + // message of trusted manifests never is. + from->charge(Resource::kFeeMalformedRequest, "too many untrusted manifests"); + + JLOG(journal.warn()) << "Manifests: message had " << total + << " entries; processed all trusted plus the first " + << kMaxManifestsPerMessage << " untrusted"; + } + if (!relay.list().empty()) { forEach([m2 = std::make_shared(relay, protocol::mtMANIFESTS)]( @@ -1225,15 +1257,60 @@ OverlayImpl::getManifestsMessage() if (auto seq = app_.getValidatorManifests().sequence(); seq != manifestListSeq_) { - protocol::TMManifests tm; - + // Phase 1: snapshot the cache under its own lock. Do not call + // Validators::listed() here — that takes the validator-list lock, and + // forEachManifest holds the manifest-cache lock, so consulting trust + // inside the callback would invert the lock order used elsewhere + // (see onManifests) and risk deadlock. Capture the manifest hash now, + // while we have the Manifest object, for the suppression key. + struct CachedManifest + { + PublicKey masterKey; + std::string serialized; + uint256 hash; + }; + std::vector cached; app_.getValidatorManifests().forEachManifest( - [&tm](std::size_t s) { tm.mutable_list()->Reserve(s); }, - [&tm, &hr = app_.getHashRouter()](Manifest const& manifest) { - tm.add_list()->set_stobject(manifest.serialized.data(), manifest.serialized.size()); - hr.addSuppression(manifest.hash()); + [&cached](std::size_t s) { cached.reserve(s); }, + [&cached](Manifest const& manifest) { + cached.push_back({manifest.masterKey, manifest.serialized, manifest.hash()}); }); + // Phase 2: no cache lock held, so trust checks are safe. Include every + // trusted manifest, then fill any remaining headroom up to + // kMaxManifestsPerMessage with untrusted gossip, so the whole message + // stays within the per-message cap the receiver enforces (trusted + // count is tiny in practice, so this effectively never drops trusted). + std::vector selected; + std::vector untrusted; + for (auto const& e : cached) + { + if (app_.getValidators().listed(e.masterKey)) + { + selected.push_back(&e); + } + else + { + untrusted.push_back(&e); + } + } + + // Cap untrusted only; trusted manifests are all included above. + auto const take = std::min(kMaxManifestsPerMessage, untrusted.size()); + selected.insert(selected.end(), untrusted.begin(), untrusted.begin() + take); + + // Shuffle the order. Cryptographic randomness is not needed here. + std::shuffle(selected.begin(), selected.end(), defaultPrng()); + + protocol::TMManifests tm; + auto& hr = app_.getHashRouter(); + tm.mutable_list()->Reserve(static_cast(selected.size())); + for (auto const* e : selected) + { + tm.add_list()->set_stobject(e->serialized.data(), e->serialized.size()); + hr.addSuppression(e->hash); + } + manifestMessage_.reset(); if (tm.list_size() != 0) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 822ef05304..7de96aa459 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -61,6 +61,7 @@ #include #include #include +#include #include #include #include @@ -1131,6 +1132,9 @@ PeerImp::onMessage(std::shared_ptr const& m) if (s > 100) fee_.update(Resource::kFeeModerateBurdenPeer, "oversize"); + // OverlayImpl::onManifests bounds the untrusted work and charges the fee + // if the untrusted count exceeds the per-message cap; trusted manifests + // are always processed and not counted against it. app_.getJobQueue().addJob(JtManifest, "RcvManifests", [this, that = shared_from_this(), m]() { overlay_.onManifests(m, that); }); diff --git a/src/xrpld/overlay/detail/ProtocolMessage.h b/src/xrpld/overlay/detail/ProtocolMessage.h index b1a30bad10..b181ea6307 100644 --- a/src/xrpld/overlay/detail/ProtocolMessage.h +++ b/src/xrpld/overlay/detail/ProtocolMessage.h @@ -351,6 +351,16 @@ invokeProtocolMessage(Buffers const& buffers, Handler& handler, std::size_t& hin return result; } + // Drop an oversized TMManifests without penalty: consume the bytes and + // return no error, so the connection is preserved. + if (header->messageType == protocol::mtMANIFESTS && + (header->payloadWireSize > kMaximumManifestsMessageSize || + header->uncompressedSize > kMaximumManifestsMessageSize)) + { + result.first = header->totalWireSize; + return result; + } + bool success = false; switch (header->messageType) From a88ba66fcea9b635a0e31df3b03ede0bc65c8a07 Mon Sep 17 00:00:00 2001 From: Bart Date: Fri, 31 Jul 2026 16:55:12 -0400 Subject: [PATCH 5/6] chore: Bump version to 3.2.1-rc1 --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index c488bb20de..323a5e628b 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -23,7 +23,7 @@ namespace { //------------------------------------------------------------------------------ // clang-format off // NOLINTNEXTLINE(readability-identifier-naming) -char const* const versionString = "3.2.0" +char const* const versionString = "3.2.1-rc1" // clang-format on ; From d4c1359921f34a4e96c5c8483119e59f0e30e4df Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 31 Jul 2026 19:46:17 -0400 Subject: [PATCH 6/6] chore: Bump version to 3.2.1 --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index 323a5e628b..48bcf2f598 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -23,7 +23,7 @@ namespace { //------------------------------------------------------------------------------ // clang-format off // NOLINTNEXTLINE(readability-identifier-naming) -char const* const versionString = "3.2.1-rc1" +char const* const versionString = "3.2.1" // clang-format on ;