From 4b2d7871fb8333cd92db23301d791a6c455d3278 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Thu, 7 May 2026 13:42:36 -0400 Subject: [PATCH] fix: Fix correct MPT sequence byte order in STIssue serialization --- .../ledger/helpers/RippleStateHelpers.cpp | 24 +++ src/libxrpl/protocol/STIssue.cpp | 79 +++++-- .../token/MPTokenIssuanceCreate.cpp | 16 ++ src/test/protocol/STIssue_test.cpp | 200 ++++++++++++++++++ 4 files changed, 302 insertions(+), 17 deletions(-) diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index 58f44534cf..2e9f9da068 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp @@ -211,6 +211,30 @@ trustCreate( // LCOV_EXCL_STOP } + if (view.rules().enabled(fixCleanup3_2_0)) + { + // fixCleanup3_2_0 introduces V2 MPT wire format, which uses + // xrpAccount() (AccountID{0}) as the serialization sentinel to + // distinguish MPT from IOU. A trust line whose account equals + // xrpAccount() would make that sentinel ambiguous during STIssue + // deserialization, so we reject it here. + // + // noAccount() (AccountID{1}) is the legacy V1 sentinel and is + // likewise rejected for defense-in-depth, even though creating a + // valid key pair for either value would require breaking SHA-256. + // + // The native() check guards against a malformed saBalance that would + // slip through as an XRP issue inside a trust-line creation. + auto invalidAccount = [](AccountID const& account) { + return account == xrpAccount() || account == noAccount(); + }; + if (invalidAccount(uLowAccountID) || invalidAccount(uHighAccountID) || + saBalance.get().native()) + { + return tecINTERNAL; // LCOV_EXCL_LINE + } + } + auto const sleRippleState = std::make_shared(ltRIPPLE_STATE, uIndex); view.insert(sleRippleState); diff --git a/src/libxrpl/protocol/STIssue.cpp b/src/libxrpl/protocol/STIssue.cpp index c0019c334f..60f70aacf9 100644 --- a/src/libxrpl/protocol/STIssue.cpp +++ b/src/libxrpl/protocol/STIssue.cpp @@ -4,8 +4,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -32,25 +34,50 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name} { asset_ = xrpIssue(); } - // Check if MPT + // Check if MPT or IOU. The second 160-bit field is the format discriminator: + // + // V1 (noAccount, pre-fixCleanup3_2_0): + // [issuer 160][noAccount 160][sequence 32 via get32/add32] + // get32() converts BE wire bytes to host order; the matching add32() + // converts back, so the round-trip is self-consistent on any single + // architecture. However, the wire bytes are host-order (non-canonical on + // LE), so external clients must byte-reverse the sequence field. + // + // V2 (xrpAccount, fixCleanup3_2_0 enabled): + // [issuer 160][xrpAccount 160][sequence 32 raw BE bytes] + // The sequence is written and read as raw bytes, preserving the + // canonical big-endian encoding produced by makeMptID(). Clients may + // embed the mpt_issuance_id bytes from RPC directly without reversal. + // + // Neither sentinel (non-zero, non-one): + // Regular IOU trust line. else { - // MPT is serialized as: - // - 160 bits MPT issuer account - // - 160 bits black hole account - // - 32 bits sequence AccountID const account = static_cast(sit.get160()); - // MPT - if (noAccount() == account) + if (account == noAccount() || account == xrpAccount()) { - MPTID mptID; - std::uint32_t sequence = sit.get32(); - static_assert(MPTID::size() == sizeof(sequence) + sizeof(currencyOrAccount)); - memcpy(mptID.data(), &sequence, sizeof(sequence)); - memcpy( - mptID.data() + sizeof(sequence), - currencyOrAccount.data(), - sizeof(currencyOrAccount)); + MPTID mptID{}; + auto const seqSize = sizeof(std::uint32_t); + if (account == noAccount()) + { + // get32() swaps BE wire bytes to host order; memcpy then stores + // host-order bytes into the MPTID. On LE this produces a + // byte-swapped MPTID relative to the canonical value from + // makeMptID(), but add() has the same inversion so the + // round-trip is consistent within a single-arch deployment. + std::uint32_t sequence = sit.get32(); + memcpy(mptID.data(), &sequence, sizeof(sequence)); + } + else + { + // V2: read raw wire bytes directly into the MPTID. No byte + // swapping; the canonical BE layout from makeMptID() is + // preserved end-to-end. + auto const rawBytes = sit.getRaw(seqSize); + memcpy(mptID.data(), rawBytes.data(), rawBytes.size()); + } + static_assert(MPTID::size() == seqSize + sizeof(currencyOrAccount)); + memcpy(mptID.data() + seqSize, currencyOrAccount.data(), sizeof(currencyOrAccount)); MPTIssue const issue{mptID}; asset_ = issue; } @@ -96,11 +123,29 @@ STIssue::add(Serializer& s) const s.addBitString(issue.account); }, [&](MPTIssue const& issue) { + auto const rules = getCurrentTransactionRules(); + auto const fixSerializationEnabled = rules && rules->enabled(fixCleanup3_2_0); s.addBitString(issue.getIssuer()); - s.addBitString(noAccount()); + // The sentinel distinguishes V2 (xrpAccount) from V1 (noAccount) + // during deserialization; see the constructor for the full format + // description. + s.addBitString(fixSerializationEnabled ? xrpAccount() : noAccount()); + // Copy the first 4 bytes of the MPTID (the canonical BE sequence) + // into a uint32_t so we can pass either to add32() or addRaw(). + // memcpy preserves the byte pattern exactly, so for V2 addRaw() + // emits the same canonical bytes that were in the MPTID. + // For V1, add32() applies a native-to-BE swap on top of what is + // already a BE-in-memory value, producing LE wire bytes on LE hosts. std::uint32_t sequence = 0; memcpy(&sequence, issue.getMptID().data(), sizeof(sequence)); - s.add32(sequence); + if (fixSerializationEnabled) + { + s.addRaw(&sequence, sizeof(sequence)); + } + else + { + s.add32(sequence); + } }); } diff --git a/src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp b/src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp index 64652a80b0..5dff00ad3b 100644 --- a/src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp +++ b/src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -91,6 +92,21 @@ MPTokenIssuanceCreate::preflight(PreflightContext const& ctx) if (maxAmt > kMAX_MP_TOKEN_AMOUNT) return temMALFORMED; } + + // V2 MPT wire format (fixCleanup3_2_0) uses xrpAccount() as the + // serialization sentinel and noAccount() was the V1 sentinel. An MPT + // whose issuer equals either sentinel would be indistinguishable from the + // format marker during STIssue deserialization. Reject such issuers here + // as defense-in-depth; deriving a key pair for AccountID{0} or AccountID{1} + // would require breaking SHA-256, but the check is cheap. + if (ctx.rules.enabled(fixCleanup3_2_0)) + { + auto const account = ctx.tx[sfAccount]; + if (account == xrpAccount() || account == noAccount()) + { + return temMALFORMED; // LCOV_EXCL_LINE + } + } return tesSUCCESS; } diff --git a/src/test/protocol/STIssue_test.cpp b/src/test/protocol/STIssue_test.cpp index 04364510f0..0b8adb078c 100644 --- a/src/test/protocol/STIssue_test.cpp +++ b/src/test/protocol/STIssue_test.cpp @@ -3,13 +3,23 @@ #include // IWYU pragma: keep #include +#include #include +#include #include +#include +#include #include +#include +#include #include #include #include #include +#include + +#include +#include namespace xrpl::test { @@ -137,12 +147,202 @@ public: "000000000000000000000000000000000000000000000002"); } + // Hard-coded wire-format fixture. + // + // Verifies what STIssue::add() actually puts on the wire for the 4-byte + // sequence field of an MPT issue. + // + // Before fix (V1, no amendment): add32() applies a native-to-BE swap on + // top of MPTID bytes that are already canonical BE. On LE hosts the two + // swaps cancel and the wire bytes end up in LE order — the opposite of + // what a conforming client expects. + // + // After fix (V2, amendment enabled): addRaw() writes the MPTID bytes + // verbatim. The wire bytes match the canonical BE encoding from makeMptID(). + void + testMPTWireFormat() + { + testcase("MPT serialization - serialized sequence bytes are canonical big-endian"); + using namespace jtx; + Account const alice{"alice"}; + + // Sequence 240 = 0x000000F0. + // Canonical BE bytes a client would expect: {0x00, 0x00, 0x00, 0xF0}. + // Serialized layout: issuer(20) + marker(20) + sequence(4). + MPTID const mptID240 = makeMptID(240, alice.id()); + + // Before fix: wire sequence bytes are LE-swapped, not canonical. + { + STIssue const st(sfAsset, Asset{MPTIssue{mptID240}}); + Serializer s; + st.add(s); + Slice const sl = s.slice(); + BEAST_EXPECT(sl.size() == 44); + BEAST_EXPECT(sl[40] == 0xF0); // ← wrong: LSB of 0xF0000000 on LE + BEAST_EXPECT(sl[41] == 0x00); + BEAST_EXPECT(sl[42] == 0x00); + BEAST_EXPECT(sl[43] == 0x00); + } + + // After fix: wire sequence bytes are canonical BE. + { + std::unordered_set> const presets{fixCleanup3_2_0}; + CurrentTransactionRulesGuard const guard(Rules{presets}); + + STIssue const st(sfAsset, Asset{MPTIssue{mptID240}}); + Serializer s; + st.add(s); + Slice const sl = s.slice(); + BEAST_EXPECT(sl.size() == 44); + BEAST_EXPECT(sl[40] == 0x00); // ← correct: MSB of 0x000000F0 + BEAST_EXPECT(sl[41] == 0x00); + BEAST_EXPECT(sl[42] == 0x00); + BEAST_EXPECT(sl[43] == 0xF0); + } + + // 0xDEADBEEF: non-palindromic value makes the byte-order contrast + // unambiguous regardless of host endianness. + MPTID const mptIDBEEF = makeMptID(0xDEADBEEF, alice.id()); + + // Before fix: {0xEF, 0xBE, 0xAD, 0xDE} — LE-swapped. + { + STIssue const st(sfAsset, Asset{MPTIssue{mptIDBEEF}}); + Serializer s; + st.add(s); + Slice const sl = s.slice(); + BEAST_EXPECT(sl[40] == 0xEF); + BEAST_EXPECT(sl[41] == 0xBE); + BEAST_EXPECT(sl[42] == 0xAD); + BEAST_EXPECT(sl[43] == 0xDE); + } + + // After fix: {0xDE, 0xAD, 0xBE, 0xEF} — canonical BE. + { + std::unordered_set> const presets{fixCleanup3_2_0}; + CurrentTransactionRulesGuard const guard(Rules{presets}); + + STIssue const st(sfAsset, Asset{MPTIssue{mptIDBEEF}}); + Serializer s; + st.add(s); + Slice const sl = s.slice(); + BEAST_EXPECT(sl[40] == 0xDE); + BEAST_EXPECT(sl[41] == 0xAD); + BEAST_EXPECT(sl[42] == 0xBE); + BEAST_EXPECT(sl[43] == 0xEF); + } + } + + // Cross-path test. + // + // Verifies that the STIssue codec (add/deserialize) agrees with the JSON + // path (mptIssueFromJson) on the meaning of a raw MPTID. + // + // The sentinels (noAccount for V1, xrpAccount for V2) are internal codec + // details. Clients only ever hold the raw 24-byte MPTID returned by RPC; + // they never construct sentinel bytes themselves. + // + // Before fix (V1): the deserializer calls get32(), which byte-swaps the + // canonical BE sequence bytes on LE hosts. The reconstructed MPTID does + // not match the original — codec output diverges from JSON output. + // + // After fix (V2): the deserializer reads the sequence bytes raw. The + // reconstructed MPTID matches exactly — codec and JSON paths agree. + void + testMPTCrossPath() + { + testcase( + "MPT serialization - decoded MPTID matches canonical value: broken in V1, fixed in V2"); + using namespace jtx; + Account const alice{"alice"}; + + // Use a non-palindromic sequence so byte-swapping produces a visibly + // different MPTID. seq=1 (0x00000001) would give 0x01000000 when + // swapped; 0xDEADBEEF is unambiguous on any host. + for (auto const seq : {240u, 0xDEADBEEFu, 1u}) + { + MPTID const canonical = makeMptID(seq, alice.id()); + + // The JSON path parses the hex string directly into an MPTID — + // always canonical. This is the reference value that the codec + // (add/deserialize round-trip) must agree with. + json::Value jv; + jv[jss::mpt_issuance_id] = to_string(canonical); + MPTIssue const fromJson = mptIssueFromJson(jv); + BEAST_EXPECT(fromJson.getMptID() == canonical); + + // Before fix: V1 codec writes [issuer][noAccount][add32(seq)]. + // Simulate the deserialization path with canonical (BE) sequence + // bytes and V1 marker: get32() byte-swaps on LE, so the + // reconstructed MPTID ≠ canonical and ≠ what mptIssueFromJson + // produced. + { + Serializer s; + s.addBitString(alice.id()); + s.addBitString(noAccount()); + s.addRaw(canonical.data(), sizeof(std::uint32_t)); + SerialIter sit(s.slice()); + STIssue const parsed(sit, sfAsset); + BEAST_EXPECT(parsed != Asset{MPTIssue{canonical}}); // ← bug + BEAST_EXPECT(parsed != Asset{fromJson}); // ← JSON/binary divergence + } + + // After fix: V2 codec writes [issuer][xrpAccount][addRaw(seq)]. + // Same canonical (BE) sequence bytes, V2 marker: getRaw() + // preserves bytes, so the reconstructed MPTID == canonical and + // == what mptIssueFromJson produced. + { + Serializer s; + s.addBitString(alice.id()); + s.addBitString(xrpAccount()); + s.addRaw(canonical.data(), sizeof(std::uint32_t)); + SerialIter sit(s.slice()); + STIssue const parsed(sit, sfAsset); + BEAST_EXPECT(parsed == Asset{MPTIssue{canonical}}); // ← fixed + BEAST_EXPECT(parsed == Asset{fromJson}); // ← JSON/binary agree + } + } + + // V1 round-trip (no amendment): xrpld's own add() and the + // deserializer are symmetrically wrong, so they cancel and the MPTID + // survives intact — the bug is invisible in internal round-trips. + { + for (auto const seq : {240u, 0xDEADBEEFu, 1u}) + { + MPTID const expected = makeMptID(seq, alice.id()); + STIssue const original(sfAsset, Asset{MPTIssue{expected}}); + Serializer s; + original.add(s); + SerialIter sit(s.slice()); + BEAST_EXPECT(STIssue(sit, sfAsset) == Asset{MPTIssue{expected}}); + } + } + + // V2 full round-trip (amendment enabled): add() and the deserializer + // both use canonical bytes — round-trip is correct and canonical. + { + std::unordered_set> const presets{fixCleanup3_2_0}; + CurrentTransactionRulesGuard const guard(Rules{presets}); + + for (auto const seq : {240u, 0xDEADBEEFu, 1u}) + { + MPTID const expected = makeMptID(seq, alice.id()); + STIssue const original(sfAsset, Asset{MPTIssue{expected}}); + Serializer s; + original.add(s); + SerialIter sit(s.slice()); + BEAST_EXPECT(STIssue(sit, sfAsset) == Asset{MPTIssue{expected}}); + } + } + } + void run() override { // compliments other unit tests to ensure complete coverage testConstructor(); testCompare(); + testMPTWireFormat(); + testMPTCrossPath(); } };