ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-06-03 08:46:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Fix correct MPT sequence byte order in STIssue serialization

Browse Source
This commit is contained in:
Gregory Tsipenyuk
2026-05-07 13:42:36 -04:00
parent 50244a8637
commit 4b2d7871fb
4 changed files with 302 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
src/libxrpl/ledger/helpers/RippleStateHelpers.cpp
View File

@@ -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<Issue>().native())
{
return tecINTERNAL; // LCOV_EXCL_LINE
}
}
auto const sleRippleState = std::make_shared<SLE>(ltRIPPLE_STATE, uIndex);
view.insert(sleRippleState);

79
src/libxrpl/protocol/STIssue.cpp
View File

@@ -4,8 +4,10 @@
#include <xrpl/json/json_value.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/MPTIssue.h>
#include <xrpl/protocol/Rules.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STBase.h>
#include <xrpl/protocol/Serializer.h>
@@ -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<AccountID>(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);
}
});
}

16
src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp
View File

@@ -8,6 +8,7 @@
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/DirectoryHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Protocol.h>
@@ -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;
}

200
src/test/protocol/STIssue_test.cpp
View File

@@ -3,13 +3,23 @@
#include <test/jtx/amount.h> // IWYU pragma: keep
#include <xrpl/basics/base_uint.h>
#include <xrpl/beast/hash/uhash.h>
#include <xrpl/beast/unit_test/suite.h>
#include <xrpl/json/json_value.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/MPTIssue.h>
#include <xrpl/protocol/Rules.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STIssue.h>
#include <xrpl/protocol/Serializer.h>
#include <xrpl/protocol/UintTypes.h>
#include <xrpl/protocol/jss.h>
#include <cstdint>
#include <unordered_set>
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<uint256, beast::Uhash<>> 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<uint256, beast::Uhash<>> 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<uint256, beast::Uhash<>> 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();
}
};