From 00eeb0a005ef2f37490963f2ea485b3c1330acdc Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:52:04 +0100 Subject: [PATCH] fix: Reject variable-length prefixes the encoder cannot write --- include/xrpl/protocol/STValidation.h | 7 + include/xrpl/protocol/Serializer.h | 192 ++++++++++++++++++++++++-- src/libxrpl/protocol/STValidation.cpp | 43 +++++- src/libxrpl/protocol/Serializer.cpp | 143 +++++++++++-------- 4 files changed, 314 insertions(+), 71 deletions(-) diff --git a/include/xrpl/protocol/STValidation.h b/include/xrpl/protocol/STValidation.h index 8101b27341..f70e971f87 100644 --- a/include/xrpl/protocol/STValidation.h +++ b/include/xrpl/protocol/STValidation.h @@ -124,6 +124,13 @@ public: [[nodiscard]] NodeID const& getNodeID() const noexcept; + /** + * Whether this validation carries a good signature. + * + * Reports false if the signature cannot be checked at all, so a caller + * cannot tell that apart from a bad signature. Either way the validation is + * unusable, and the reason is logged. Only a computed answer is remembered. + */ [[nodiscard]] bool isValid() const noexcept; diff --git a/include/xrpl/protocol/Serializer.h b/include/xrpl/protocol/Serializer.h index c1ea5c16ba..997199629a 100644 --- a/include/xrpl/protocol/Serializer.h +++ b/include/xrpl/protocol/Serializer.h @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -25,6 +26,101 @@ private: Blob data_; public: + /** + * A header is never longer than this. The encoder fills a buffer of this + * size and writes only the bytes it used. + */ + static constexpr int kMaxNumberOfBytesInHeader = 3; + + // A field whose size varies is stored as a header holding its length, then + // the field data. The header is 1, 2 or 3 bytes long. Nothing outside it says + // which, so the decoder reads the first byte and its value says how long the + // header is: + // + // 0 ... 192 kMin/kMaxValueOfFirstByteFor1ByteHeader + // 193 ... 240 kMin/kMaxValueOfFirstByteFor2ByteHeader + // 241 ... 254 kMin/kMaxValueOfFirstByteFor3ByteHeader + // 255 belongs to no header + // + // Each range starts one past the end of the range before it. + + static constexpr int kMinValueOfFirstByteFor1ByteHeader = 0; + static constexpr int kMaxValueOfFirstByteFor1ByteHeader = 192; + + static constexpr int kMinValueOfFirstByteFor2ByteHeader = + kMaxValueOfFirstByteFor1ByteHeader + 1; + static constexpr int kMaxValueOfFirstByteFor2ByteHeader = 240; + + static constexpr int kMinValueOfFirstByteFor3ByteHeader = + kMaxValueOfFirstByteFor2ByteHeader + 1; + + static constexpr int kMaxValueOfFirstByteFor3ByteHeader = 254; + + // A length x too big for one byte is split across the header. For 2 bytes: + // + // first byte = 193 + (x - 193) / 256 + // second byte = (x - 193) % 256 + // + // so 300 is stored as 193, 107. For 3 bytes it is the same, from 241, with + // the remainder split across two bytes: 20,000 is stored as 241, 29, 95. + + static constexpr int kNumberOfValuesInOneByte = 256; + static constexpr int kNumberOfValuesInTwoBytes = + kNumberOfValuesInOneByte * kNumberOfValuesInOneByte; + + // Each header length therefore covers a range of field lengths: + // + // 0 ... 192 kMin/kMaxValueOfLengthFor1ByteHeader + // 193 ... 12,480 kMin/kMaxValueOfLengthFor2ByteHeader + // 12,481 ... 918,744 kMin/kMaxValueOfLengthFor3ByteHeader + // + // The encoder always uses the shortest header that fits. + + /** + * A 1 byte header holds the length in the byte itself, so both ends of + * this range are the same numbers as the first byte's own range. + */ + static constexpr int kMinValueOfLengthFor1ByteHeader = kMinValueOfFirstByteFor1ByteHeader; + static constexpr int kMaxValueOfLengthFor1ByteHeader = kMaxValueOfFirstByteFor1ByteHeader; + + static constexpr int kMinValueOfLengthFor2ByteHeader = kMaxValueOfLengthFor1ByteHeader + 1; + + /** + * 48 values of the first byte mean a 2 byte header, and each of them covers + * 256 lengths. The 48 is worked out from the two range ends above, so it + * stays right if either of them changes. + */ + static constexpr int kMaxValueOfLengthFor2ByteHeader = kMinValueOfLengthFor2ByteHeader + + ((kMaxValueOfFirstByteFor2ByteHeader - kMaxValueOfFirstByteFor1ByteHeader) * + kNumberOfValuesInOneByte) - + 1; + + static constexpr int kMinValueOfLengthFor3ByteHeader = kMaxValueOfLengthFor2ByteHeader + 1; + + /** + * 14 values of the first byte mean a 3 byte header, and each of them covers + * 65,536 lengths. Counted the same way, that gives the largest length any + * header can state. + * + * Nothing is accepted or rejected against this. The assertion below uses it + * to check that every length the encoder writes is one a header can state. + */ + static constexpr int kMaxRepresentableLength = kMinValueOfLengthFor3ByteHeader + + ((kMaxValueOfFirstByteFor3ByteHeader - kMaxValueOfFirstByteFor2ByteHeader) * + kNumberOfValuesInTwoBytes) - + 1; + + /** + * The largest length the encoder will write. This is the one number here + * that is picked rather than worked out. The decoder accepts nothing above + * it, so both sides agree on the same set of lengths. + */ + static constexpr int kMaxValueOfLengthFor3ByteHeader = 918744; + + static_assert( + kMaxValueOfLengthFor3ByteHeader <= kMaxRepresentableLength, + "a length the encoder writes must be one a header can state"); + explicit Serializer(int n = 256) { data_.reserve(n); @@ -61,7 +157,7 @@ public: // assemble functions int - add8(unsigned char i); + add8(unsigned char byteValue); int add16(std::uint16_t i); @@ -270,18 +366,90 @@ public: return v.data_ == data_; } + /** + * Works out how long a header is, from its first byte. + * + * Each overload of decodeVLLength below reads one header length, so call + * this first to learn which of them to call. + * + * @param firstByte First byte of the header, as read from the stream. + * @return How many bytes the whole header takes, counting firstByte: 1, 2 + * or 3. + * @throws std::overflow_error if firstByte is the one value that starts no + * header. + */ static int - decodeLengthLength(int b1); + decodeLengthLength(std::byte firstByte); + + /** + * Reads the field length out of a 1 byte header. + * + * @param firstByte The single header byte, which is the length itself. + * @return Field length in bytes, from kMinValueOfLengthFor1ByteHeader to + * kMaxValueOfLengthFor1ByteHeader. + * @throws std::overflow_error if firstByte is big enough to mean a longer + * header, in which case it is not a length by itself. + */ static int - decodeVLLength(int b1); + decodeVLLength(std::byte firstByte); + + /** + * Reads the field length out of a 2 byte header. + * + * @param firstByte First header byte. Its value means a 2 byte header, and + * how far it sits into that range gives the top part of the length. + * @param secondByte Second header byte, holding the rest of the length. + * @return Field length in bytes, from kMinValueOfLengthFor2ByteHeader to + * kMaxValueOfLengthFor2ByteHeader. + * @throws std::overflow_error if firstByte is outside the range that means + * a 2 byte header. + */ static int - decodeVLLength(int b1, int b2); + decodeVLLength(std::byte firstByte, std::byte secondByte); + + /** + * Reads the field length out of a 3 byte header. + * + * @param firstByte First header byte. Its value means a 3 byte header, and + * how far it sits into that range gives the top part of the length. + * @param secondByte Second header byte, holding the middle part of the + * length. + * @param thirdByte Third header byte, holding the low part. + * @return Field length in bytes, from kMinValueOfLengthFor3ByteHeader to + * kMaxValueOfLengthFor3ByteHeader. + * @throws std::overflow_error if firstByte is outside the range that means + * a 3 byte header, or if the three bytes together state a length above + * kMaxValueOfLengthFor3ByteHeader, which the encoder would not write back. + */ static int - decodeVLLength(int b1, int b2, int b3); + decodeVLLength(std::byte firstByte, std::byte secondByte, std::byte thirdByte); private: + /** + * Works out how many bytes the header needs for the given length. + * + * This deliberately repeats the width choice addEncoded makes, so that + * addVL's assertion can compare the two. It has no other caller; do not + * reach for it as a utility. + * + * @param length Field length in bytes. + * @return How many header bytes it needs: 1, 2 or 3. + * @throws std::overflow_error if length is negative, or above + * kMaxValueOfLengthFor3ByteHeader. + */ static int - encodeLengthLength(int length); // length to encode length + encodeLengthLength(int length); + + /** + * Appends the length header for a field of the given length. + * + * The field's own data is not written; the caller appends it next. + * + * @param length Field length in bytes. + * @return Offset within this Serializer at which the header was written. + * @throws std::overflow_error if length is negative, or above + * kMaxValueOfLengthFor3ByteHeader. + */ int addEncoded(int length); }; @@ -390,9 +558,15 @@ public: void getFieldID(int& type, int& name); - // Returns the size of the VL if the - // next object is a VL. Advances the iterator - // to the beginning of the VL. + /** + * Reads the length header at the read position and steps past it. + * + * @return Field length in bytes. The iterator is left on the first byte of + * the field data. + * @throws std::overflow_error if the header states a length the encoder could + * not have written. + * @throws std::runtime_error if the data runs out before the header does. + */ int getVLDataLength(); diff --git a/src/libxrpl/protocol/STValidation.cpp b/src/libxrpl/protocol/STValidation.cpp index 1656aad3a2..cf02fdacf7 100644 --- a/src/libxrpl/protocol/STValidation.cpp +++ b/src/libxrpl/protocol/STValidation.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include +#include #include namespace xrpl { @@ -104,11 +106,42 @@ STValidation::isValid() const noexcept publicKeyType(getSignerPublic()) == KeyType::Secp256k1, "xrpl::STValidation::isValid : valid key type"); - valid_ = verifyDigest( - getSignerPublic(), - getSigningHash(), - makeSlice(getFieldVL(sfSignature)), - (getFlags() & kVfFullyCanonicalSig) != 0u); + // Log that the signature was never checked, so an operator does not + // read this as a bad key. The log is guarded because it can throw too. + auto reportUncheckable = [this](char const* reason) noexcept { + try + { + JLOG(debugLog().error()) + << "Cannot check the signature of the validation for ledger " << getLedgerHash() + << ": " << reason; + } + catch (...) // NOLINT(bugprone-empty-catch) + { + // Nothing can be reported when reporting is what failed. + } + }; + + // The signing hash re-serializes the fields, which can fail. This + // function is noexcept, so report the validation as invalid instead of + // throwing. valid_ stays unset, so a later call checks again. + try + { + valid_ = verifyDigest( + getSignerPublic(), + getSigningHash(), + makeSlice(getFieldVL(sfSignature)), + (getFlags() & kVfFullyCanonicalSig) != 0u); + } + catch (std::exception const& e) + { + reportUncheckable(e.what()); + return false; + } + catch (...) + { + reportUncheckable("unknown exception"); + return false; + } } return valid_.value(); diff --git a/src/libxrpl/protocol/Serializer.cpp b/src/libxrpl/protocol/Serializer.cpp index 80ecdee6c8..7f6fe625c2 100644 --- a/src/libxrpl/protocol/Serializer.cpp +++ b/src/libxrpl/protocol/Serializer.cpp @@ -143,10 +143,10 @@ Serializer::addFieldID(int type, int name) } int -Serializer::add8(unsigned char byte) +Serializer::add8(unsigned char byteValue) { int const ret = data_.size(); - data_.push_back(byte); + data_.push_back(byteValue); return ret; } @@ -210,109 +210,138 @@ Serializer::addVL(void const* ptr, int len) int Serializer::addEncoded(int length) { - std::array bytes{}; + // Without this, a negative length would fall into the 1 byte case below and + // be cast to a first byte no header uses. A size too big for int arrives + // here negative as well, since callers pass sizes through this parameter. + if (length < kMinValueOfLengthFor1ByteHeader) + Throw("addEncoded: length is negative or did not fit in an int"); + + std::array bytes{}; int numBytes = 0; - if (length <= 192) + if (length <= kMaxValueOfLengthFor1ByteHeader) { - bytes[0] = static_cast(length); + bytes[0] = static_cast(length); numBytes = 1; } - else if (length <= 12480) + else if (length <= kMaxValueOfLengthFor2ByteHeader) { - length -= 193; - bytes[0] = 193 + static_cast(length >> 8); - bytes[1] = static_cast(length & 0xff); + // Count from the smallest length a 2 byte header covers. + int const offset = length - kMinValueOfLengthFor2ByteHeader; + bytes[0] = static_cast( + kMinValueOfFirstByteFor2ByteHeader + (offset / kNumberOfValuesInOneByte)); + bytes[1] = static_cast(offset % kNumberOfValuesInOneByte); numBytes = 2; } - else if (length <= 918744) + else if (length <= kMaxValueOfLengthFor3ByteHeader) { - length -= 12481; - bytes[0] = 241 + static_cast(length >> 16); - bytes[1] = static_cast((length >> 8) & 0xff); - bytes[2] = static_cast(length & 0xff); + int const offset = length - kMinValueOfLengthFor3ByteHeader; + bytes[0] = static_cast( + kMinValueOfFirstByteFor3ByteHeader + (offset / kNumberOfValuesInTwoBytes)); + bytes[1] = + static_cast((offset / kNumberOfValuesInOneByte) % kNumberOfValuesInOneByte); + bytes[2] = static_cast(offset % kNumberOfValuesInOneByte); numBytes = 3; } else { - Throw("lenlen"); + Throw("addEncoded: length is too large to encode"); } - return addRaw(&bytes[0], numBytes); + return addRaw(bytes.data(), numBytes); } int Serializer::encodeLengthLength(int length) { - if (length < 0) - Throw("len<0"); + if (length < kMinValueOfLengthFor1ByteHeader) + { + Throw( + "encodeLengthLength: length is negative or did not fit in an int"); + } - if (length <= 192) + if (length <= kMaxValueOfLengthFor1ByteHeader) return 1; - if (length <= 12480) + if (length <= kMaxValueOfLengthFor2ByteHeader) return 2; - if (length <= 918744) + if (length <= kMaxValueOfLengthFor3ByteHeader) return 3; - Throw("len>918744"); - return 0; // Silence compiler warning. + Throw("encodeLengthLength: length is too large to encode"); } int -Serializer::decodeLengthLength(int b1) +Serializer::decodeLengthLength(std::byte firstByte) { - if (b1 < 0) - Throw("b1<0"); + int const firstByteValue = std::to_integer(firstByte); - if (b1 <= 192) + if (firstByteValue <= kMaxValueOfFirstByteFor1ByteHeader) return 1; - if (b1 <= 240) + if (firstByteValue <= kMaxValueOfFirstByteFor2ByteHeader) return 2; - if (b1 <= 254) + if (firstByteValue <= kMaxValueOfFirstByteFor3ByteHeader) return 3; - Throw("b1>254"); - return 0; // Silence compiler warning. + Throw("decodeLengthLength: first byte does not start any header"); } int -Serializer::decodeVLLength(int b1) +Serializer::decodeVLLength(std::byte firstByte) { - if (b1 < 0) - Throw("b1<0"); + int const length = std::to_integer(firstByte); - if (b1 > 254) - Throw("b1>254"); + // A bigger value means a longer header, so it is not a length by itself. + if (length > kMaxValueOfLengthFor1ByteHeader) + Throw("decodeVLLength 1 byte: first byte is not a length"); - return b1; + return length; } int -Serializer::decodeVLLength(int b1, int b2) +Serializer::decodeVLLength(std::byte firstByte, std::byte secondByte) { - if (b1 < 193) - Throw("b1<193"); + int const firstByteValue = std::to_integer(firstByte); - if (b1 > 240) - Throw("b1>240"); + if (firstByteValue < kMinValueOfFirstByteFor2ByteHeader) + Throw("decodeVLLength 2 byte: first byte is below the range"); - return 193 + ((b1 - 193) * 256) + b2; + if (firstByteValue > kMaxValueOfFirstByteFor2ByteHeader) + Throw("decodeVLLength 2 byte: first byte is above the range"); + + // Both bytes are bounded by their own type, and the first one is bounded to + // the 2 byte range above, so this cannot leave the range the header covers. + return kMinValueOfLengthFor2ByteHeader + + ((firstByteValue - kMinValueOfFirstByteFor2ByteHeader) * kNumberOfValuesInOneByte) + + std::to_integer(secondByte); } int -Serializer::decodeVLLength(int b1, int b2, int b3) +Serializer::decodeVLLength(std::byte firstByte, std::byte secondByte, std::byte thirdByte) { - if (b1 < 241) - Throw("b1<241"); + int const firstByteValue = std::to_integer(firstByte); - if (b1 > 254) - Throw("b1>254"); + if (firstByteValue < kMinValueOfFirstByteFor3ByteHeader) + Throw("decodeVLLength 3 byte: first byte is below the range"); - return 12481 + ((b1 - 241) * 65536) + (b2 * 256) + b3; + if (firstByteValue > kMaxValueOfFirstByteFor3ByteHeader) + Throw("decodeVLLength 3 byte: first byte is above the range"); + + int const length = kMinValueOfLengthFor3ByteHeader + + ((firstByteValue - kMinValueOfFirstByteFor3ByteHeader) * kNumberOfValuesInTwoBytes) + + (std::to_integer(secondByte) * kNumberOfValuesInOneByte) + + std::to_integer(thirdByte); + + // A 3 byte header reaches further than kMaxValueOfLengthFor3ByteHeader, which + // is as far as the encoder goes. Refuse the rest, so every length accepted + // here is one that can be written back. + if (length > kMaxValueOfLengthFor3ByteHeader) + Throw("decodeVLLength 3 byte: length is too large to re-encode"); + + return length; } //------------------------------------------------------------------------------ @@ -471,24 +500,24 @@ SerialIter::getRaw(int size) int SerialIter::getVLDataLength() { - int const b1 = get8(); + std::byte const firstByte{get8()}; int datLen = 0; - int const lenLen = Serializer::decodeLengthLength(b1); + int const lenLen = Serializer::decodeLengthLength(firstByte); if (lenLen == 1) { - datLen = Serializer::decodeVLLength(b1); + datLen = Serializer::decodeVLLength(firstByte); } else if (lenLen == 2) { - int const b2 = get8(); - datLen = Serializer::decodeVLLength(b1, b2); + std::byte const secondByte{get8()}; + datLen = Serializer::decodeVLLength(firstByte, secondByte); } else { XRPL_ASSERT(lenLen == 3, "xrpl::SerialIter::getVLDataLength : lenLen is 3"); - int const b2 = get8(); - int const b3 = get8(); - datLen = Serializer::decodeVLLength(b1, b2, b3); + std::byte const secondByte{get8()}; + std::byte const thirdByte{get8()}; + datLen = Serializer::decodeVLLength(firstByte, secondByte, thirdByte); } return datLen; }