From 6f0f065505c855b87bde46cb42b3ec71328eecb6 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sat, 1 Aug 2026 21:35:54 -0400 Subject: [PATCH] fix: Clamp the depth used to index selectBranch's key byte `selectBranch` reads the key byte at `depth / 2`, which is out of bounds for a 32-byte key once the depth reaches 64. Every branch selection in the map funnels through here, so this is the one place a stray depth can turn into a bad read. Assert the precondition for callers, then clamp anyway: a wrong answer for an input that should never occur is better than reading past the buffer. Verified under ASan that the unclamped form reads one byte past a 32-byte allocation while the clamped form does not. `depthMask`'s own 65-entry table had the same exposure one level up, reachable through the public `createID` factory rather than only from inside the map. A depth past `kLeafDepth` indexed that table out of bounds, confirmed under ASan as a 4-byte `global-buffer-overflow` immediately after `kMasks`. Both places that can set a depth now clamp it: the constructor, which is the single point every `SHAMapNodeID`'s `depth_` passes through, and `createID`, which needs its own bound because it picks the mask while evaluating the constructor's argument, before the constructor body could correct anything. Clamping rather than throwing, which is what `getChildNodeID` does for the analogous case: `createID` is reached from `getSHAMapNodeID` with a peer-supplied depth, and two of that function's three callers (`InboundTransactions::gotData`, `PeerImp::onMessage`) sit on paths with no handler between them and a thread boundary, so a throw there would end the process rather than the message. Clamping also has to fix up `id_` alongside `depth_`, since a node ID whose id and depth disagree fails the invariant every read of `id_` relies on. Leaving the depth unclamped would additionally let `getRawString` narrow it to a byte, turning depth 256 into a node claiming to be the root. `deserializeSHAMapNodeID` gets the same mask check `isPrefixOf` already performs, pulled into a shared `isPrefixOfAtDepth` helper, and the masking both it and `createID` perform is now a named `maskedToDepth` rather than a repeated bitwise-and. Tests cover the depth-sensitivity of `isPrefixOf`, the guards that must hold with asserts stripped, that `deserializeSHAMapNodeID` rejects an out-of-range depth, and the clamp itself under both build configurations (`EXPECT_DEATH` in a forked process when the assert is live, and the clamped result compared against depth 63 when it is not). The clamp test also gates on `ENABLE_VOIDSTAR`, not just `NDEBUG`: under Antithesis instrumentation `XRPL_ASSERT` routes to a handler that records the hit but never aborts, so a Debug build with voidstar enabled has `NDEBUG` undefined yet still hits the same non-fatal assert as a release build, and without this gate would send that configuration into the `EXPECT_DEATH` arm, where the forked child never dies and the test fails, breaking the CI job that runs this suite under `-Dvoidstar=ON`. --- src/libxrpl/shamap/SHAMapNodeID.cpp | 59 ++++++- src/tests/libxrpl/shamap/SHAMapNodeID.cpp | 190 ++++++++++++++++++++++ 2 files changed, 242 insertions(+), 7 deletions(-) create mode 100644 src/tests/libxrpl/shamap/SHAMapNodeID.cpp diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index 8fd7afe8fc..951a011567 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -40,9 +41,39 @@ depthMask(unsigned int depth) return kMasks.entry[depth]; } +// The prefix of `key` at `depth`: the leading nibbles naming the subtree a node at that depth +// identifies, with the remainder of the key masked off. +static uint256 +maskedToDepth(uint256 const& key, unsigned int depth) +{ + return key & depthMask(depth); +} + +// Whether `id` at `depth` is what `key` looks like once masked down to that depth, i.e. +// whether an ID with this depth and id names a subtree that `key` falls under. +static bool +isPrefixOfAtDepth(uint256 const& id, unsigned int depth, uint256 const& key) +{ + return maskedToDepth(key, depth) == id; +} + // canonicalize the hash to a node ID for this depth SHAMapNodeID::SHAMapNodeID(unsigned int depth, uint256 const& hash) : id_(hash), depth_(depth) { + // Every SHAMapNodeID's depth is stored here, so this is the one place that can stop an + // out-of-range one from being kept: a depth past kLeafDepth would go on to index depthMask + // out of bounds, and getRawString would narrow it to a byte, silently renaming the node. + // Clamp rather than throw, since node IDs are built from peer-supplied depths on the ledger + // data path, where no caller catches an exception before it reaches a thread boundary. + if (depth_ > SHAMap::kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMapNodeID::SHAMapNodeID : depth within tree"); + depth_ = SHAMap::kLeafDepth; + id_ = maskedToDepth(id_, depth_); + // LCOV_EXCL_STOP + } + XRPL_ASSERT( depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::SHAMapNodeID : maximum depth input"); XRPL_ASSERT( @@ -89,7 +120,7 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const bool SHAMapNodeID::isPrefixOf(uint256 const& key) const { - return (key & depthMask(depth_)) == id_; + return isPrefixOfAtDepth(id_, depth_, key); } [[nodiscard]] std::optional @@ -102,9 +133,9 @@ deserializeSHAMapNodeID(void const* data, std::size_t size) unsigned int const depth = *(static_cast(data) + 32); if (depth <= SHAMap::kLeafDepth) { - auto const id = uint256::fromVoid(data); - - if (id == (id & depthMask(depth))) + // Reject a serialized ID carrying bits below its own depth. Checked before + // constructing, since the constructor asserts that same property. + if (auto const id = uint256::fromVoid(data); isPrefixOfAtDepth(id, depth, id)) ret.emplace(depth, id); } } @@ -115,7 +146,11 @@ deserializeSHAMapNodeID(void const* data, std::size_t size) [[nodiscard]] unsigned int selectBranch(SHAMapNodeID const& id, uint256 const& hash) { - auto const depth = id.getDepth(); + XRPL_ASSERT(id.getDepth() < SHAMap::kLeafDepth, "xrpl::selectBranch : depth below leaf depth"); + + // A depth-64 ID has no nibble left to select. Callers must not ask, but clamp anyway to keep + // the read below the end of the 32-byte key. + auto const depth = std::min(id.getDepth(), SHAMap::kLeafDepth - 1u); auto branch = static_cast(*(hash.begin() + (depth / 2))); if ((depth & 1) != 0u) @@ -134,8 +169,18 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash) SHAMapNodeID SHAMapNodeID::createID(unsigned int depth, uint256 const& key) { - XRPL_ASSERT(depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); - return SHAMapNodeID(depth, key & depthMask(depth)); + // The mask is chosen here, before the constructor runs, so the clamp there cannot cover this + // call: an out-of-range depth would index depthMask's table while still evaluating this + // argument. A public factory has to hold its own bound. + if (depth > SHAMap::kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMapNodeID::createID : depth within tree"); + depth = SHAMap::kLeafDepth; + // LCOV_EXCL_STOP + } + + return SHAMapNodeID(depth, maskedToDepth(key, depth)); } } // namespace xrpl diff --git a/src/tests/libxrpl/shamap/SHAMapNodeID.cpp b/src/tests/libxrpl/shamap/SHAMapNodeID.cpp new file mode 100644 index 0000000000..f5fa476814 --- /dev/null +++ b/src/tests/libxrpl/shamap/SHAMapNodeID.cpp @@ -0,0 +1,190 @@ +#include + +#include +#include +#include + +#include + +#include + +namespace xrpl::tests { + +// An arbitrary 32-byte key reused across tests below that don't care about its specific value, +// only that it is a well-formed key. +constexpr uint256 kTestKey("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"); + +TEST(SHAMapNodeIDTest, root_is_prefix_of_every_key) +{ + SHAMapNodeID const root; + EXPECT_EQ(root.getDepth(), 0u); + EXPECT_TRUE(root.isPrefixOf(uint256{})); + EXPECT_TRUE(root.isPrefixOf(kTestKey)); +} + +TEST(SHAMapNodeIDTest, child_id_is_prefix_of_keys_in_that_branch) +{ + // Walking the branches spelled by the key's own nibbles must keep every + // intermediate ID a prefix of that key. + SHAMapNodeID id; + for (auto depth = 0u; depth < SHAMap::kLeafDepth; ++depth) + { + id = id.getChildNodeID(selectBranch(id, kTestKey)); + EXPECT_EQ(id.getDepth(), depth + 1); + EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << id.getDepth(); + } +} + +TEST(SHAMapNodeIDTest, wrong_branch_is_not_prefix_of_key) +{ + SHAMapNodeID const root; + auto const correct = selectBranch(root, kTestKey); + ASSERT_EQ(correct, 0xbu); + + // An ID built from the wrong branch still has a valid depth and a self-consistent mask, so + // isPrefixOf(kTestKey) below is what actually distinguishes the correct branch from the rest. + for (auto branch = 0u; branch < SHAMap::kBranchFactor; ++branch) + { + auto const child = root.getChildNodeID(branch); + EXPECT_EQ(child.getDepth(), 1u); + EXPECT_EQ(child.isPrefixOf(kTestKey), branch == correct) << "branch " << branch; + } +} + +TEST(SHAMapNodeIDTest, prefix_check_is_depth_sensitive) +{ + // kTestKey and kOther agree on the first two nibbles ("b9") and then diverge. + constexpr uint256 kOther("b99891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"); + + auto id = SHAMapNodeID{}.getChildNodeID(selectBranch(SHAMapNodeID{}, kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kOther)) << "shared first nibble"; + + id = id.getChildNodeID(selectBranch(id, kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kOther)) << "shared second nibble"; + + // Third nibble differs, so the deeper ID no longer covers kOther. + id = id.getChildNodeID(selectBranch(id, kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kTestKey)); + EXPECT_FALSE(id.isPrefixOf(kOther)); +} + +TEST(SHAMapNodeIDTest, leaf_id_from_key_is_prefix_of_that_key) +{ + SHAMapNodeID const leaf{SHAMap::kLeafDepth, kTestKey}; + EXPECT_TRUE(leaf.isPrefixOf(kTestKey)); + + // At full depth the prefix is the whole key, so nothing else matches. + constexpr uint256 kOther("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca9"); + EXPECT_FALSE(leaf.isPrefixOf(kOther)); +} + +TEST(SHAMapNodeIDTest, create_id_masks_key_to_depth) +{ + for (auto depth = 0u; depth <= SHAMap::kLeafDepth; ++depth) + { + auto const id = SHAMapNodeID::createID(depth, kTestKey); + EXPECT_EQ(id.getDepth(), depth); + EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << depth; + } +} + +// The guards below must hold with XRPL_ASSERT compiled out (NDEBUG), so each one +// has to be a real runtime check rather than an assert. + +TEST(SHAMapNodeIDTest, child_of_leaf_depth_id_throws) +{ + auto const leafDepthID = SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey); + ASSERT_EQ(leafDepthID.getDepth(), SHAMap::kLeafDepth); + EXPECT_THROW((void)leafDepthID.getChildNodeID(0), std::logic_error); +} + +TEST(SHAMapNodeIDTest, out_of_range_depth_is_clamped) +{ + // A depth past kLeafDepth has no mask in depthMask's 65-entry table, so both the constructor + // and createID clamp it. createID needs its own clamp: it picks the mask while evaluating the + // constructor's argument, so the constructor's clamp cannot cover that read. + // + // Both clamps are marked UNREACHABLE, which is an assert and therefore fatal wherever asserts + // are live. Only a build with them compiled out (or routed to Antithesis's non-fatal handler) + // reaches the clamp itself, so that is the only configuration that can assert on the result. +#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR) + for (auto const depth : {SHAMap::kLeafDepth + 1u, 100u, 255u, 256u, 320u}) + { + auto const id = SHAMapNodeID::createID(depth, kTestKey); + + // Clamped to a real depth, not the depth asked for, and not a byte-narrowed version of it: + // 256 would otherwise become 0 and name the root, 320 would become 64. + EXPECT_EQ(id.getDepth(), SHAMap::kLeafDepth) << "depth " << depth; + + // id_ and depth_ still agree, so the object is usable rather than merely non-crashing. + EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << depth; + EXPECT_EQ(id, SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey)) << "depth " << depth; + + // The clamp holds through the wire format too, which encodes the depth in one byte. + auto const roundTripped = deserializeSHAMapNodeID(id.getRawString()); + ASSERT_TRUE(roundTripped.has_value()) << "depth " << depth; + EXPECT_EQ(roundTripped->getDepth(), SHAMap::kLeafDepth) << "depth " << depth; + } + + // The constructor clamps on its own, for the paths that do not go through createID. + SHAMapNodeID const direct{SHAMap::kLeafDepth + 1u, uint256{}}; + EXPECT_EQ(direct.getDepth(), SHAMap::kLeafDepth); +#else + EXPECT_DEATH( + (void)SHAMapNodeID::createID(SHAMap::kLeafDepth + 1u, kTestKey), "depth within tree"); +#endif +} + +TEST(SHAMapNodeIDTest, select_branch_clamps_leaf_depth) +{ + // selectBranch's own precondition is depth < kLeafDepth: a depth-64 ID has no nibble left + // to select. That makes it unlike the guards above, which have a throw/return reachable + // even with XRPL_ASSERT compiled out; selectBranch has no such path, so the two build + // configurations have to be tested differently. + // + // Under ENABLE_VOIDSTAR, XRPL_ASSERT routes to Antithesis's assert_impl, which only records + // the hit and returns rather than aborting, even though NDEBUG is undefined there (voidstar + // requires a Debug build). So the assert is live in name but never fatal, the same as the + // NDEBUG case below. + auto const leafDepthID = SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey); + +#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR) + // With the assert compiled out or routed to a non-fatal handler, the clamp is what stands + // between this call and reading past the end of the 32-byte key. Clamping means it reads the + // same byte, and returns the same branch, as the deepest ID that still has one: depth 63. + auto const deepestWithBranchID = SHAMapNodeID::createID(SHAMap::kLeafDepth - 1u, kTestKey); + auto const branch = selectBranch(leafDepthID, kTestKey); + EXPECT_LT(branch, SHAMap::kBranchFactor); + EXPECT_EQ(branch, selectBranch(deepestWithBranchID, kTestKey)); +#else + // In a debug build the assert is live and must reject this call outright, in a forked + // process so a failure here cannot take down the rest of the suite. + EXPECT_DEATH((void)selectBranch(leafDepthID, kTestKey), "depth below leaf depth"); +#endif +} + +TEST(SHAMapNodeIDTest, deserialize_rejects_out_of_range_depth) +{ + // getRawString() only serializes a depth already accepted by the constructor's own + // assertion, so an out-of-range depth here is built by hand instead. + auto serializeWithRawDepth = [](unsigned int depth) { + Serializer s; + s.addBitString(uint256{}); + s.add8(static_cast(depth)); + return s.getString(); + }; + + for (auto const depth : {65u, 100u, 255u}) + EXPECT_FALSE(deserializeSHAMapNodeID(serializeWithRawDepth(depth)).has_value()) + << "depth " << depth; + + // A depth-64 ID is legal, since leaves live there, but it has no children. + auto const id = + deserializeSHAMapNodeID(SHAMapNodeID{SHAMap::kLeafDepth, uint256{}}.getRawString()); + ASSERT_TRUE(id.has_value()); + EXPECT_THROW((void)id->getChildNodeID(0), std::logic_error); +} + +} // namespace xrpl::tests