From ea76103d5f522ae3ce4b27155e194faab99e379e Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Mon, 12 Nov 2018 06:23:12 -0800 Subject: [PATCH] Detect malformed data earlier during deserialization (RIPD-1695): When deserializing specially crafted data, the code would ignore certain types of errors. Reserializing objects created from such data results in failures or generates a different serialization, which is not ideal. Also addresses: RIPD-1677, RIPD-1682, RIPD-1686 and RIPD-1689. Acknowledgements: Ripple thanks Guido Vranken for responsibly disclosing these issues. Bug Bounties and Responsible Disclosures: We welcome reviews of the rippled code and urge researchers to responsibly disclose any issues that they may find. For more on Ripple's Bug Bounty program, please visit: https://ripple.com/bug-bounty --- src/ripple/protocol/STObject.h | 10 --- src/ripple/protocol/impl/STObject.cpp | 119 ++++++++++++-------------- src/ripple/protocol/impl/STTx.cpp | 4 +- src/test/protocol/STObject_test.cpp | 71 +++++++++++++++ src/test/protocol/STTx_test.cpp | 50 +++++++++-- 5 files changed, 172 insertions(+), 82 deletions(-) diff --git a/src/ripple/protocol/STObject.h b/src/ripple/protocol/STObject.h index 7a7ff9f5a7..531437758d 100644 --- a/src/ripple/protocol/STObject.h +++ b/src/ripple/protocol/STObject.h @@ -538,16 +538,6 @@ private: getSortedFields ( STObject const& objToSort, WhichFields whichFields); - // Two different ways to compare STObjects. - // - // This one works only if the SOTemplates are the same. Presumably it - // runs faster since there's no sorting. - static bool equivalentSTObjectSameTemplate ( - STObject const& obj1, STObject const& obj2); - - // This way of comparing STObjects always works, but is slower. - static bool equivalentSTObject (STObject const& obj1, STObject const& obj2); - // Implementation for getting (most) fields that return by value. // // The remove_cv and remove_reference are necessitated by the STBitString diff --git a/src/ripple/protocol/impl/STObject.cpp b/src/ripple/protocol/impl/STObject.cpp index 6d6b8ace19..7b954eec41 100644 --- a/src/ripple/protocol/impl/STObject.cpp +++ b/src/ripple/protocol/impl/STObject.cpp @@ -169,49 +169,58 @@ bool STObject::set (SerialIter& sit, int depth) noexcept (false) v_.clear(); // Consume data in the pipe until we run out or reach the end - // - while (!reachedEndOfObject && !sit.empty ()) + while (!sit.empty ()) { int type; int field; // Get the metadata for the next field - // - sit.getFieldID (type, field); + sit.getFieldID(type, field); - reachedEndOfObject = (type == STI_OBJECT) && (field == 1); + // The object termination marker has been found and the termination + // marker has been consumed. Done deserializing. + if (type == STI_OBJECT && field == 1) + { + reachedEndOfObject = true; + break; + } - if ((type == STI_ARRAY) && (field == 1)) + if (type == STI_ARRAY && field == 1) { JLOG (debugLog().error()) - << "Encountered object with end of array marker"; - Throw ("Illegal terminator in object"); + << "Encountered object with embedded end-of-array marker"; + Throw("Illegal end-of-array marker in object"); } - if (!reachedEndOfObject) + auto const& fn = SField::getField(type, field); + + if (fn.isInvalid ()) { - // Figure out the field - // - auto const& fn = SField::getField (type, field); - - if (fn.isInvalid ()) - { - JLOG (debugLog().error()) - << "Unknown field: field_type=" << type - << ", field_name=" << field; - Throw ("Unknown field"); - } - - // Unflatten the field - v_.emplace_back(sit, fn, depth+1); - - // If the object type has a known SOTemplate then set it. - STObject* const obj = dynamic_cast (&(v_.back().get())); - if (obj) - obj->applyTemplateFromSField (fn); // May throw + JLOG (debugLog().error()) + << "Unknown field: field_type=" << type + << ", field_name=" << field; + Throw ("Unknown field"); } + + // Unflatten the field + v_.emplace_back(sit, fn, depth+1); + + // If the object type has a known SOTemplate then set it. + if (auto const obj = dynamic_cast(&(v_.back().get()))) + obj->applyTemplateFromSField (fn); // May throw } + // We want to ensure that the deserialized object does not contain any + // duplicate fields. This is a key invariant: + auto const sf = getSortedFields(*this, withAllFields); + + auto const dup = std::adjacent_find (sf.cbegin(), sf.cend(), + [] (STBase const* lhs, STBase const* rhs) + { return lhs->getFName() == rhs->getFName(); }); + + if (dup != sf.cend()) + Throw ("Duplicate field detected"); + return reachedEndOfObject; } @@ -279,10 +288,23 @@ bool STObject::isEquivalent (const STBase& t) const if (!v) return false; - if (mType != nullptr && (v->mType == mType)) - return equivalentSTObjectSameTemplate (*this, *v); + if (mType != nullptr && v->mType == mType) + { + return std::equal (begin(), end(), v->begin(), v->end(), + [](STBase const& st1, STBase const& st2) + { + return (st1.getSType() == st2.getSType()) && st1.isEquivalent(st2); + }); + } - return equivalentSTObject (*this, *v); + auto const sf1 = getSortedFields(*this, withAllFields); + auto const sf2 = getSortedFields(*v, withAllFields); + + return std::equal (sf1.begin (), sf1.end (), sf2.begin (), sf2.end (), + [] (STBase const* st1, STBase const* st2) + { + return (st1->getSType() == st2->getSType()) && st1->isEquivalent (*st2); + }); } uint256 STObject::getHash (std::uint32_t prefix) const @@ -741,42 +763,7 @@ STObject::getSortedFields ( return lhs->getFName().fieldCode < rhs->getFName().fieldCode; }); - // There should never be duplicate fields in an STObject. Verify that - // in debug mode. - assert (std::adjacent_find (sf.cbegin(), sf.cend(), - [] (STBase const* lhs, STBase const* rhs) - { - return lhs->getFName().fieldCode == rhs->getFName().fieldCode; - }) == sf.cend()); - return sf; } -bool STObject::equivalentSTObjectSameTemplate ( - STObject const& obj1, STObject const& obj2) -{ - assert (obj1.mType != nullptr); - assert (obj1.mType == obj2.mType); - - return std::equal (obj1.begin (), obj1.end (), obj2.begin (), obj2.end (), - [] (STBase const& st1, STBase const& st2) - { - return (st1.getSType() == st2.getSType()) && - st1.isEquivalent (st2); - }); -} - -bool STObject::equivalentSTObject (STObject const& obj1, STObject const& obj2) -{ - auto sf1 = getSortedFields (obj1, withAllFields); - auto sf2 = getSortedFields (obj2, withAllFields); - - return std::equal (sf1.begin (), sf1.end (), sf2.begin (), sf2.end (), - [] (STBase const* st1, STBase const* st2) - { - return (st1->getSType() == st2->getSType()) && - st1->isEquivalent (*st2); - }); -} - } // ripple diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp index 0207e1b7db..3b7361ee34 100644 --- a/src/ripple/protocol/impl/STTx.cpp +++ b/src/ripple/protocol/impl/STTx.cpp @@ -71,7 +71,9 @@ STTx::STTx (SerialIter& sit) noexcept (false) if ((length < txMinSizeBytes) || (length > txMaxSizeBytes)) Throw ("Transaction length invalid"); - set (sit); + if (set (sit)) + Throw ("Transaction contains an object terminator"); + tx_type_ = static_cast (getFieldU16 (sfTransactionType)); applyTemplate (getTxFormat (tx_type_)->elements); // May throw diff --git a/src/test/protocol/STObject_test.cpp b/src/test/protocol/STObject_test.cpp index d6b7f39906..932518aa61 100644 --- a/src/test/protocol/STObject_test.cpp +++ b/src/test/protocol/STObject_test.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include @@ -631,6 +632,75 @@ public: } } + void + testMalformed() + { + testcase ("Malformed serialized forms"); + + try + { + std::array const payload {{ 0xee, 0xee, 0xe1, 0xee, 0xee }}; + SerialIter sit{makeSlice(payload)}; + auto obj = std::make_shared(sit, sfMetadata); + BEAST_EXPECT(!obj); + } + catch (std::exception const& e) + { + BEAST_EXPECT(strcmp(e.what(), + "Duplicate field detected") == 0); + } + + try + { + std::array const payload {{ 0xe2, 0xe1, 0xe2 }}; + SerialIter sit{makeSlice(payload)}; + auto obj = std::make_shared(sit, sfMetadata); + BEAST_EXPECT(!obj); + } + catch (std::exception const& e) + { + BEAST_EXPECT(strcmp(e.what(), + "Duplicate field detected") == 0); + } + + try + { + std::array const payload + {{ + 0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x4f, + 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00, + 0x00, 0x00, 0x35, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x68, + 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x73, 0x00, 0x81, 0x14, + 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x65, 0x24, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe, 0xf3, 0xe7, 0xe5, 0x65, + 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x6f, 0x00, 0x00, 0x20, + 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00, 0x35, + 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x68, 0x40, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x02, 0x00, 0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, + 0x00, 0x20, 0x1e, 0x00, 0x4f, 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, + 0x00, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00, 0x35, 0x24, 0x59, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, + 0x00, 0x54, 0x72, 0x61, 0x6e, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, + 0xfe, 0xf3, 0xe7, 0xe5, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, + 0x00, 0x6f, 0x00, 0x00, 0x20, 0xf6, 0x00, 0x00, 0x03, 0x1f, 0x20, 0x20, + 0x00, 0x00, 0x00, 0x00, 0x35, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x73, 0x00, + 0x81, 0x14, 0x00, 0x10, 0x00, 0x73, 0x00, 0x81, 0x14, 0x00, 0x10, 0x00, + 0x00, 0x00, 0x00, 0x26, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe + }}; + + SerialIter sit{makeSlice(payload)}; + auto obj = std::make_shared(sit); + BEAST_EXPECT(!obj); + } + catch (std::exception const& e) + { + BEAST_EXPECT(strcmp(e.what(), + "Duplicate field detected") == 0); + } + } + void run() override { @@ -642,6 +712,7 @@ public: testParseJSONArray(); testParseJSONArrayWithInvalidChildrenObjects(); testParseJSONEdgeCases(); + testMalformed(); } }; diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 7414b99307..5edcf581a4 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -35,8 +35,7 @@ class STTx_test : public beast::unit_test::suite public: void run() override { - testcase ("overly nested transactions"); - testDeepNesting(); + testMalformedSerializedForm(); testcase ("secp256k1 signatures"); testSTTx (KeyType::secp256k1); @@ -48,8 +47,10 @@ public: testObjectCtorErrors(); } - void testDeepNesting() + void testMalformedSerializedForm() { + testcase ("Malformed serialized form"); + constexpr unsigned char payload1[] = { 0x0a, 0xff, 0xff, 0xff, 0xff, 0x63, 0x63, 0x63, 0x63, 0x63, 0x63, 0x63, @@ -1217,6 +1218,31 @@ public: 0x12, 0x12, 0x12, 0xff }; + constexpr unsigned char dupField[] = + { + 0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x4f, + 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00, + 0x00, 0x00, 0x35, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x68, + 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x73, 0x00, 0x81, 0x14, + 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x65, 0x24, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe, 0xf3, 0xe7, 0xe5, 0x65, + 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x6f, 0x00, 0x00, 0x20, + 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00, 0x35, + 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x68, 0x40, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x02, 0x00, 0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, + 0x00, 0x20, 0x1e, 0x00, 0x4f, 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, + 0x00, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00, 0x35, 0x24, 0x59, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, + 0x00, 0x54, 0x72, 0x61, 0x6e, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe5, + 0xfe, 0xf3, 0xe7, 0xe5, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, + 0x00, 0x6f, 0x00, 0x00, 0x20, 0xf6, 0x00, 0x00, 0x03, 0x1f, 0x20, 0x20, + 0x00, 0x00, 0x00, 0x00, 0x35, 0x00, 0x59, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x68, 0x40, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x73, 0x00, + 0x81, 0x14, 0x00, 0x10, 0x00, 0x73, 0x00, 0x81, 0x14, 0x00, 0x10, 0x00, + 0x00, 0x00, 0x00, 0x26, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe + }; + try { protocol::TMTransaction tx2; @@ -1229,7 +1255,8 @@ public: } catch (std::exception const& e) { - BEAST_EXPECT(strcmp(e.what(), "Maximum nesting depth of STVar exceeded") == 0); + BEAST_EXPECT(strcmp(e.what(), + "Maximum nesting depth of STVar exceeded") == 0); } try @@ -1240,7 +1267,20 @@ public: } catch (std::exception const& e) { - BEAST_EXPECT(strcmp(e.what(), "Maximum nesting depth of STVar exceeded") == 0); + BEAST_EXPECT(strcmp(e.what(), + "Maximum nesting depth of STVar exceeded") == 0); + } + + try + { + ripple::SerialIter sit (Slice{dupField, sizeof(dupField)}); + auto stx = std::make_shared(sit); + fail("An exception should have been thrown"); + } + catch (std::exception const& e) + { + BEAST_EXPECT(strcmp(e.what(), + "Duplicate field detected") == 0); } }