diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index ad87d106c4..c7fc4fa796 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -90,7 +90,11 @@ public: operator=(STObject&& other); STObject(SOTemplate const& type, SField const& name); - STObject(SOTemplate const& type, SerialIter& sit, SField const& name); + STObject( + SOTemplate const& type, + SerialIter& sit, + SField const& name, + bool requireCanonicalOrder = false); STObject(SerialIter& sit, SField const& name, int depth = 0); STObject(SerialIter&& sit, SField const& name); explicit STObject(SField const& name); @@ -123,7 +127,7 @@ public: set(SOTemplate const&); bool - set(SerialIter& u, int depth = 0); + set(SerialIter& u, int depth = 0, bool requireCanonicalOrder = false); [[nodiscard]] SerializedTypeID getSType() const override; diff --git a/include/xrpl/protocol/STValidation.h b/include/xrpl/protocol/STValidation.h index 444fdfa600..8101b27341 100644 --- a/include/xrpl/protocol/STValidation.h +++ b/include/xrpl/protocol/STValidation.h @@ -54,6 +54,22 @@ class STValidation final : public STObject, public CountedObject NetClock::time_point seenTime_; public: + /** + * @struct DeserializeOptions + * @brief Options controlling deserialization of a STValidation. + + * @var DeserializeOptions::checkSignature + * Whether to verify the data was signed properly + * + * @var DeserializeOptions::requireCanonicalOrder + * Whether to require the fields to be in canonical order + */ + struct DeserializeOptions + { + bool checkSignature; + bool requireCanonicalOrder; + }; + /** * Construct a STValidation from a peer from serialized data. * @@ -64,12 +80,12 @@ public: * that signed the validation. For manifest based * validators, this should be the NodeID of the master * public key. - * @param checkSignature Whether to verify the data was signed properly + * @param options Options controlling deserialization * * @note Throws if the object is not valid */ template - STValidation(SerialIter& sit, LookupNodeID&& lookupNodeID, bool checkSignature); + STValidation(SerialIter& sit, LookupNodeID&& lookupNodeID, DeserializeOptions options); /** * Construct, sign and trust a new STValidation issued by this node. @@ -163,8 +179,8 @@ private: }; template -STValidation::STValidation(SerialIter& sit, LookupNodeID&& lookupNodeID, bool checkSignature) - : STObject(validationFormat(), sit, sfValidation) +STValidation::STValidation(SerialIter& sit, LookupNodeID&& lookupNodeID, DeserializeOptions options) + : STObject(validationFormat(), sit, sfValidation, options.requireCanonicalOrder) , signingPubKey_([this]() { auto const spk = getFieldVL(sfSigningPubKey); @@ -175,7 +191,7 @@ STValidation::STValidation(SerialIter& sit, LookupNodeID&& lookupNodeID, bool ch }()) , nodeID_(lookupNodeID(signingPubKey_)) { - if (checkSignature && !isValid()) + if (options.checkSignature && !isValid()) { JLOG(debugLog().error()) << "Invalid signature in validation: " << getJson(JsonOptions::Values::None); diff --git a/src/libxrpl/protocol/STObject.cpp b/src/libxrpl/protocol/STObject.cpp index 4b3ace2be3..e8a6df8c0a 100644 --- a/src/libxrpl/protocol/STObject.cpp +++ b/src/libxrpl/protocol/STObject.cpp @@ -56,10 +56,15 @@ STObject::STObject(SOTemplate const& type, SField const& name) : STBase(name) set(type); } -STObject::STObject(SOTemplate const& type, SerialIter& sit, SField const& name) : STBase(name) +STObject::STObject( + SOTemplate const& type, + SerialIter& sit, + SField const& name, + bool requireCanonicalOrder) + : STBase(name) { v_.reserve(type.size()); - set(sit); + set(sit, 0, requireCanonicalOrder); applyTemplate(type); // May throw } @@ -208,12 +213,13 @@ STObject::applyTemplateFromSField(SField const& sField) // return true = terminated with end-of-object bool -STObject::set(SerialIter& sit, int depth) +STObject::set(SerialIter& sit, int depth, bool requireCanonicalOrder) { bool reachedEndOfObject = false; v_.clear(); + std::optional prevFieldCode; // Consume data in the pipe until we run out or reach the end while (!sit.empty()) { @@ -238,7 +244,6 @@ STObject::set(SerialIter& sit, int depth) } auto const& fn = SField::getField(type, field); - if (fn.isInvalid()) { JLOG(debugLog().error()) @@ -246,6 +251,13 @@ STObject::set(SerialIter& sit, int depth) Throw("Unknown field"); } + if (requireCanonicalOrder && prevFieldCode.has_value() && fn.fieldCodeMem <= *prevFieldCode) + { + JLOG(debugLog().error()) << "Fields in object are not in canonical order"; + Throw("Fields in object are not in canonical order"); + } + prevFieldCode = fn.fieldCodeMem; + // Unflatten the field v_.emplace_back(sit, fn, depth + 1); diff --git a/src/test/protocol/STValidation_test.cpp b/src/test/protocol/STValidation_test.cpp index e42411bd3f..eb9aefd0ed 100644 --- a/src/test/protocol/STValidation_test.cpp +++ b/src/test/protocol/STValidation_test.cpp @@ -153,7 +153,10 @@ public: SerialIter sit{kPayload8}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, true); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = true, .requireCanonicalOrder = false}); BEAST_EXPECT(val); BEAST_EXPECT(val->isFieldPresent(sfLedgerSequence)); @@ -174,7 +177,10 @@ public: { SerialIter sit{kPayload1}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("An exception should have been thrown"); } catch (std::exception const& ex) @@ -186,7 +192,10 @@ public: { SerialIter sit{kPayload2}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("An exception should have been thrown"); } catch (std::exception const& ex) @@ -198,7 +207,10 @@ public: { SerialIter sit{kPayload3}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("An exception should have been thrown"); } catch (std::exception const& ex) @@ -210,7 +222,10 @@ public: { SerialIter sit{kPayload4}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("An exception should have been thrown"); } catch (std::exception const& ex) @@ -224,7 +239,10 @@ public: { SerialIter sit{kPayload5}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("Expected exception not thrown from validation"); } catch (std::exception const& ex) @@ -236,7 +254,10 @@ public: { SerialIter sit{kPayload6}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("Expected exception not thrown from validation"); } catch (std::exception const& ex) @@ -249,7 +270,10 @@ public: SerialIter sit{kPayload7}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, false); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = false}); fail("Expected exception not thrown from validation"); } @@ -279,7 +303,10 @@ public: SerialIter sit{makeSlice(v2)}; auto val = std::make_shared( - sit, [](PublicKey const& pk) { return calcNodeID(pk); }, true); + sit, + [](PublicKey const& pk) { return calcNodeID(pk); }, + STValidation::DeserializeOptions{ + .checkSignature = true, .requireCanonicalOrder = false}); fail("Mutated validation signature checked out: offset=" + std::to_string(i)); } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 5662b6d33d..962ab0f408 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -2346,12 +2346,22 @@ PeerImp::onMessage(std::shared_ptr const& m) std::shared_ptr val; { SerialIter sit(makeSlice(m->validation())); - val = std::make_shared( - std::ref(sit), - [this](PublicKey const& pk) { - return calcNodeID(app_.getValidatorManifests().getMasterKey(pk)); - }, - false); + try + { + val = std::make_shared( + std::ref(sit), + [this](PublicKey const& pk) { + return calcNodeID(app_.getValidatorManifests().getMasterKey(pk)); + }, + STValidation::DeserializeOptions{ + .checkSignature = false, .requireCanonicalOrder = true}); + } + catch (std::exception const& e) + { + JLOG(pJournal_.warn()) << "Validation: Exception, " << e.what(); + fee_.update(Resource::kFeeInvalidData, e.what()); + return; + } val->setSeen(closeTime); }