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

@@ -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

src/ripple/protocol/impl/STObject.cpp

@@ -169,30 +169,30 @@ 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);
-
-        if ((type == STI_ARRAY) && (field == 1))
+        // The object termination marker has been found and the termination
+        // marker has been consumed. Done deserializing.
+        if (type == STI_OBJECT && field == 1)
         {
-            JLOG (debugLog().error())
-                << "Encountered object with end of array marker";
-            Throw<std::runtime_error> ("Illegal terminator in object");
+            reachedEndOfObject = true;
+            break;
         }
 
-        if (!reachedEndOfObject)
+        if (type == STI_ARRAY && field == 1)
         {
-            // Figure out the field
-            //
-            auto const& fn = SField::getField (type, field);
+            JLOG (debugLog().error())
+                << "Encountered object with embedded end-of-array marker";
+            Throw<std::runtime_error>("Illegal end-of-array marker in object");
+        }
+
+        auto const& fn = SField::getField(type, field);
 
         if (fn.isInvalid ())
         {
@@ -206,11 +206,20 @@ bool STObject::set (SerialIter& sit, int depth) noexcept (false)
         v_.emplace_back(sit, fn, depth+1);
 
         // If the object type has a known SOTemplate then set it.
-            STObject* const obj = dynamic_cast <STObject*> (&(v_.back().get()));
-            if (obj)
+        if (auto const obj = dynamic_cast<STObject*>(&(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<std::runtime_error> ("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

src/ripple/protocol/impl/STTx.cpp

@@ -71,7 +71,9 @@ STTx::STTx (SerialIter& sit) noexcept (false)
     if ((length < txMinSizeBytes) || (length > txMaxSizeBytes))
         Throw<std::runtime_error> ("Transaction length invalid");
 
-    set (sit);
+    if (set (sit))
+        Throw<std::runtime_error> ("Transaction contains an object terminator");
+
     tx_type_ = static_cast<TxType> (getFieldU16 (sfTransactionType));
 
     applyTemplate (getTxFormat (tx_type_)->elements);  // May throw

src/test/protocol/STObject_test.cpp

@@ -26,6 +26,7 @@
 #include <ripple/beast/unit_test.h>
 #include <test/jtx.h>
 
+#include <array>
 #include <memory>
 #include <type_traits>
 
@@ -631,6 +632,75 @@ public:
         }
     }
 
+    void
+    testMalformed()
+    {
+        testcase ("Malformed serialized forms");
+
+        try
+        {
+            std::array<std::uint8_t, 5> const payload {{ 0xee, 0xee, 0xe1, 0xee, 0xee }};
+            SerialIter sit{makeSlice(payload)};
+            auto obj = std::make_shared<STArray>(sit, sfMetadata);
+            BEAST_EXPECT(!obj);
+        }
+        catch (std::exception const& e)
+        {
+            BEAST_EXPECT(strcmp(e.what(),
+                "Duplicate field detected") == 0);
+        }
+
+        try
+        {
+            std::array<std::uint8_t, 3> const payload {{ 0xe2, 0xe1, 0xe2 }};
+            SerialIter sit{makeSlice(payload)};
+            auto obj = std::make_shared<STObject>(sit, sfMetadata);
+               BEAST_EXPECT(!obj);
+        }
+        catch (std::exception const& e)
+        {
+            BEAST_EXPECT(strcmp(e.what(),
+                "Duplicate field detected") == 0);
+        }
+
+        try
+        {
+            std::array<std::uint8_t, 250> 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<STTx>(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();
     }
 };
 

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<ripple::STTx const>(sit);
+            fail("An exception should have been thrown");
+        }
+        catch (std::exception const& e)
+        {
+            BEAST_EXPECT(strcmp(e.what(),
+                "Duplicate field detected") == 0);
         }
     }