From 57fe197d3ee63793cbc078bdb444a0a857f7ab04 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 13 Feb 2019 14:08:55 -0800 Subject: [PATCH] Remove runtime inference of unrecognized SFields --- src/ripple/protocol/SField.h | 27 +--- src/ripple/protocol/SOTemplate.h | 3 + src/ripple/protocol/impl/SField.cpp | 86 +---------- src/ripple/protocol/impl/SOTemplate.cpp | 8 +- src/test/protocol/STObject_test.cpp | 98 ++++++------ src/test/protocol/STTx_test.cpp | 188 ++++++++++++++++++++++-- 6 files changed, 241 insertions(+), 169 deletions(-) diff --git a/src/ripple/protocol/SField.h b/src/ripple/protocol/SField.h index 4834d402de..cb784b2581 100644 --- a/src/ripple/protocol/SField.h +++ b/src/ripple/protocol/SField.h @@ -24,8 +24,6 @@ #include #include #include -#include -#include #include namespace ripple { @@ -104,17 +102,8 @@ field_code(int id, int index) /** Identifies fields. Fields are necessary to tag data in signed transactions so that - the binary format of the transaction can be canonicalized. - - There are two categories of these fields: - - 1. Those that are created at compile time. - 2. Those that are created at run time. - - Both are always const. Category 1 can only be created in FieldNames.cpp. - This is enforced at compile time. Category 2 can only be created by - calling getField with an as yet unused fieldType and fieldValue (or the - equivalent fieldCode). + the binary format of the transaction can be canonicalized. All + SFields are created at compile time. Each SField, once constructed, lives until program termination, and there is only one instance per fieldType/fieldValue pair which serves the entire @@ -156,21 +145,14 @@ public: SField& operator=(SField&&) = delete; public: - struct private_access_tag_t; // public, but still an implementation detail + struct private_access_tag_t; // public, but still an implementation detail // These constructors can only be called from SField.cpp SField (private_access_tag_t, SerializedTypeID tid, int fv, const char* fn, int meta = sMD_Default, IsSigning signing = IsSigning::yes); explicit SField (private_access_tag_t, int fc); - SField (private_access_tag_t, SerializedTypeID tid, int fv); -protected: - // These constructors can only be called from SField.cpp - explicit SField (SerializedTypeID tid, int fv); - -public: - // getField will dynamically construct a new SField if necessary static const SField& getField (int fieldCode); static const SField& getField (std::string const& fieldName); static const SField& getField (int type, int value) @@ -270,10 +252,7 @@ public: private: static int num; - - static std::mutex SField_mutex; static std::map knownCodeToField; - static std::map> unknownCodeToField; }; /** A field with a type known at compile time. */ diff --git a/src/ripple/protocol/SOTemplate.h b/src/ripple/protocol/SOTemplate.h index 690bb34e48..8544f206d0 100644 --- a/src/ripple/protocol/SOTemplate.h +++ b/src/ripple/protocol/SOTemplate.h @@ -20,6 +20,7 @@ #ifndef RIPPLE_PROTOCOL_SOTEMPLATE_H_INCLUDED #define RIPPLE_PROTOCOL_SOTEMPLATE_H_INCLUDED +#include #include #include #include @@ -49,6 +50,8 @@ public: : e_field (fieldName) , flags (flags) { + if (! e_field.isUseful()) + Throw ("SField in SOElement must be useful."); } }; diff --git a/src/ripple/protocol/impl/SField.cpp b/src/ripple/protocol/impl/SField.cpp index 58f931b40e..22c98e30b7 100644 --- a/src/ripple/protocol/impl/SField.cpp +++ b/src/ripple/protocol/impl/SField.cpp @@ -17,7 +17,6 @@ */ //============================================================================== -#include #include #include #include @@ -28,11 +27,7 @@ namespace ripple { // Storage for static const members. SField::IsSigning const SField::notSigning; int SField::num = 0; -std::mutex SField::SField_mutex; std::map SField::knownCodeToField; -std::map> SField::unknownCodeToField; - -using StaticScopedLockType = std::lock_guard ; // Give only this translation unit permission to construct SFields struct SField::private_access_tag_t @@ -40,7 +35,7 @@ struct SField::private_access_tag_t explicit private_access_tag_t() = default; }; -SField::private_access_tag_t access; +static SField::private_access_tag_t access; // Construct all compile-time SFields, and register them in the knownCodeToField // database: @@ -270,29 +265,6 @@ SField::SField(private_access_tag_t, int fc) knownCodeToField[fieldCode] = this; } -// call with the map mutex to protect num. -// This is naturally done with no extra expense -// from getField(int code). -SField::SField(SerializedTypeID tid, int fv) - : fieldCode (field_code (tid, fv)) - , fieldType (tid) - , fieldValue (fv) - , fieldName (std::to_string (tid) + '/' + std::to_string (fv)) - , fieldMeta (sMD_Default) - , fieldNum (++num) - , signingField (IsSigning::yes) - , jsonName (fieldName.c_str()) -{ - assert ((fv != 1) || ((tid != STI_ARRAY) && (tid != STI_OBJECT))); -} - -SField::SField(private_access_tag_t, - SerializedTypeID tid, int fv) - : SField(tid, fv) -{ - knownCodeToField[fieldCode] = this; -} - SField const& SField::getField (int code) { @@ -300,54 +272,9 @@ SField::getField (int code) if (it != knownCodeToField.end ()) { - // 99+% of the time, it will be a valid, known field return * (it->second); } - - int type = code >> 16; - int field = code & 0xffff; - - // Don't dynamically extend types that have no binary encoding. - if ((field > 255) || (code < 0)) - return sfInvalid; - - switch (type) - { - // Types we are willing to dynamically extend - // types (common) - case STI_UINT16: - case STI_UINT32: - case STI_UINT64: - case STI_HASH128: - case STI_HASH256: - case STI_AMOUNT: - case STI_VL: - case STI_ACCOUNT: - case STI_OBJECT: - case STI_ARRAY: - // types (uncommon) - case STI_UINT8: - case STI_HASH160: - case STI_PATHSET: - case STI_VECTOR256: - break; - - default: - return sfInvalid; - } - - { - // Lookup in the run-time data base, and create if it does not - // yet exist. - StaticScopedLockType sl (SField_mutex); - - auto it = unknownCodeToField.find (code); - - if (it != unknownCodeToField.end ()) - return * (it->second); - return *(unknownCodeToField[code] = std::unique_ptr( - new SField(safe_cast(type), field))); - } + return sfInvalid; } int SField::compare (SField const& f1, SField const& f2) @@ -373,15 +300,6 @@ SField::getField (std::string const& fieldName) if (fieldPair.second->fieldName == fieldName) return * (fieldPair.second); } - { - StaticScopedLockType sl (SField_mutex); - - for (auto const & fieldPair : unknownCodeToField) - { - if (fieldPair.second->fieldName == fieldName) - return * (fieldPair.second); - } - } return sfInvalid; } diff --git a/src/ripple/protocol/impl/SOTemplate.cpp b/src/ripple/protocol/impl/SOTemplate.cpp index ded77c68d2..5abac18dee 100644 --- a/src/ripple/protocol/impl/SOTemplate.cpp +++ b/src/ripple/protocol/impl/SOTemplate.cpp @@ -28,18 +28,20 @@ void SOTemplate::push_back (SOElement const& r) // if (mIndex.empty ()) { - // Unmapped indices will be set to -1 + // Unmapped indices are initialized to -1 // mIndex.resize (SField::getNumFields () + 1, -1); } // Make sure the field's index is in range // - assert (r.e_field.getNum () < mIndex.size ()); + if (r.e_field.getNum() <= 0 || r.e_field.getNum() >= mIndex.size()) + Throw ("Invalid field index for SOTemplate."); // Make sure that this field hasn't already been assigned // - assert (getIndex (r.e_field) == -1); + if (getIndex (r.e_field) != -1) + Throw ("Duplicate field index for SOTemplate."); // Add the field to the index mapping table // diff --git a/src/test/protocol/STObject_test.cpp b/src/test/protocol/STObject_test.cpp index 932518aa61..9861d1a473 100644 --- a/src/test/protocol/STObject_test.cpp +++ b/src/test/protocol/STObject_test.cpp @@ -235,12 +235,54 @@ public: testcase ("serialization"); unexpected (sfGeneric.isUseful (), "sfGeneric must not be useful"); + { + // Try to put sfGeneric in an SOTemplate. + SOTemplate elements; + except( [&]() + { + elements.push_back (SOElement (sfGeneric, SOE_REQUIRED)); + }); + } + + unexpected (sfInvalid.isUseful (), "sfInvalid must not be useful"); + { + // Test return of sfInvalid. + auto testInvalid = [this] (SerializedTypeID tid, int fv) + { + SField const& shouldBeInvalid {SField::getField (tid, fv)}; + BEAST_EXPECT (shouldBeInvalid == sfInvalid); + }; + testInvalid (STI_VL, 255); + testInvalid (STI_HASH256, 255); + testInvalid (STI_UINT32, 255); + testInvalid (STI_VECTOR256, 255); + testInvalid (STI_OBJECT, 255); + } + { + // Try to put sfInvalid in an SOTemplate. + SOTemplate elements; + except( [&]() + { + elements.push_back (SOElement (sfInvalid, SOE_REQUIRED)); + }); + } + { + // Try to put the same SField into an SOTemplate twice. + SOTemplate elements; + elements.push_back (SOElement (sfAccount, SOE_REQUIRED)); + except( [&]() + { + elements.push_back (SOElement (sfAccount, SOE_REQUIRED)); + }); + } + + // Put a variety of SFields of different types in an SOTemplate. + SField const& sfTestVL = sfMasterSignature; + SField const& sfTestH256 = sfCheckID; + SField const& sfTestU32 = sfSettleDelay; + SField const& sfTestV256 = sfAmendments; + SField const& sfTestObject = sfMajority; - SField const& sfTestVL = SField::getField (STI_VL, 255); - SField const& sfTestH256 = SField::getField (STI_HASH256, 255); - SField const& sfTestU32 = SField::getField (STI_UINT32, 255); - SField const& sfTestV256 = SField::getField(STI_VECTOR256, 255); - SField const& sfTestObject = SField::getField (STI_OBJECT, 255); SOTemplate elements; elements.push_back (SOElement (sfFlags, SOE_REQUIRED)); @@ -639,15 +681,15 @@ public: try { - std::array const payload {{ 0xee, 0xee, 0xe1, 0xee, 0xee }}; + std::array const payload { + { 0xe9, 0x12, 0xab, 0xcd, 0x12, 0xfe, 0xdc }}; 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); + BEAST_EXPECT(strcmp(e.what(), "Duplicate field detected") == 0); } try @@ -659,45 +701,7 @@ public: } 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); + BEAST_EXPECT(strcmp(e.what(), "Duplicate field detected") == 0); } } diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index 5edcf581a4..3ca07704a4 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -28,6 +28,8 @@ #include #include +#include + namespace ripple { class STTx_test : public beast::unit_test::suite @@ -1218,7 +1220,7 @@ public: 0x12, 0x12, 0x12, 0xff }; - constexpr unsigned char dupField[] = + constexpr unsigned char payload3[] = { 0x12, 0x00, 0x65, 0x24, 0x00, 0x00, 0x00, 0x00, 0x20, 0x1e, 0x00, 0x4f, 0x00, 0x00, 0x20, 0x1f, 0x03, 0xf6, 0x00, 0x00, 0x20, 0x20, 0x00, 0x00, @@ -1243,6 +1245,173 @@ public: 0x00, 0x00, 0x00, 0x26, 0x00, 0x00, 0x00, 0x00, 0xe5, 0xfe }; + // Construct an STObject with 11 levels of object nesting so the + // maximum nesting level exception is thrown. + { + // Construct an SOTemplate to get the ball rolling on building + // an STObject that can contain another STObject. + SOTemplate recurse; + recurse.push_back (SOElement {sfTransactionMetaData, SOE_OPTIONAL}); + recurse.push_back (SOElement {sfTransactionHash, SOE_OPTIONAL}); + + // Make an STObject that nests objects ten levels deep. There's + // a minimum transaction size we must meet, so include a hash256. + uint256 const hash {42u}; + auto inner = + std::make_unique (recurse, sfTransactionMetaData); + inner->setFieldH256 (sfTransactionHash, hash); + + for (int i = 1; i < 10; ++i) + { + auto outer = + std::make_unique (recurse, sfTransactionMetaData); + outer->set (std::move (inner)); + outer->setFieldH256 (sfTransactionHash, hash); + inner = std::move (outer); + } + { + // A ten-deep nested STObject should throw an exception that + // there is no sfTransactionType field. + Serializer const tenDeep {inner->getSerializer()}; + SerialIter tenSit {tenDeep.slice()}; + try + { + auto stx = std::make_shared(tenSit); + fail ("STTx construction should have thrown."); + } + catch (std::runtime_error const& ex) + { + BEAST_EXPECT ( + std::strcmp (ex.what(), "Field not found") == 0); + } + } + + // Add one more level of nesting and we should get an error + // about excessive nesting. + STObject outer {recurse, sfTransactionMetaData}; + outer.set (std::move (inner)); + outer.setFieldH256 (sfTransactionHash, hash); + + Serializer const tooDeep {outer.getSerializer()}; + SerialIter tooDeepSit {tooDeep.slice()}; + try + { + auto stx = std::make_shared(tooDeepSit); + fail ("STTx construction should have thrown."); + } + catch (std::runtime_error const& ex) + { + BEAST_EXPECT (std::strcmp (ex.what(), + "Maximum nesting depth of STVar exceeded") == 0); + } + } + { + // Construct an STObject with 11 levels of nesting so the + // maximum nesting level exception is thrown. This time + // we're nesting arrays in addition to objects. + + // Construct an SOTemplate to get the ball rolling on building + // an STObject that can contain an STArray. + SOTemplate recurse; + recurse.push_back (SOElement {sfTransactionMetaData, SOE_OPTIONAL}); + recurse.push_back (SOElement {sfTransactionHash, SOE_OPTIONAL}); + recurse.push_back (SOElement {sfTemplate, SOE_OPTIONAL}); + + // Make an STObject that nests ten levels deep alternating objects + // and arrays. Include a hash256 to meet the minimum transaction + // size. + uint256 const hash {42u}; + STObject inner = {recurse, sfTransactionMetaData}; + inner.setFieldH256 (sfTransactionHash, hash); + + for (int i = 1; i < 5; ++i) + { + STObject outer {recurse, sfTransactionMetaData}; + outer.setFieldH256 (sfTransactionHash, hash); + STArray& array {outer.peekFieldArray (sfTemplate)}; + array.push_back (std::move (inner)); + inner = std::move (outer); + } + { + // A nine-deep nested STObject/STArray should throw an + // exception that there is no sfTransactionType field. + Serializer const nineDeep {inner.getSerializer()}; + SerialIter nineSit {nineDeep.slice()}; + try + { + auto stx = std::make_shared(nineSit); + fail ("STTx construction should have thrown."); + } + catch (std::runtime_error const& ex) + { + BEAST_EXPECT ( + std::strcmp (ex.what(), "Field not found") == 0); + } + } + + // Add one more level of nesting and we should get an error + // about excessive nesting. + STObject outer {recurse, sfTransactionMetaData}; + outer.setFieldH256 (sfTransactionHash, hash); + STArray& array {outer.peekFieldArray (sfTemplate)}; + array.push_back (std::move (inner)); + + Serializer const tooDeep {outer.getSerializer()}; + SerialIter tooDeepSit {tooDeep.slice()}; + try + { + auto stx = std::make_shared(tooDeepSit); + fail ("STTx construction should have thrown."); + } + catch (std::runtime_error const& ex) + { + BEAST_EXPECT (std::strcmp (ex.what(), + "Maximum nesting depth of STVar exceeded") == 0); + } + } + + { + // Make an otherwise legit STTx with a duplicate field. Should + // generate an exception when we deserialize. + auto const keypair = randomKeyPair (KeyType::secp256k1); + STTx acctSet (ttACCOUNT_SET, + [&keypair](auto& obj) + { + obj.setAccountID (sfAccount, calcAccountID(keypair.first)); + obj.setFieldU32 (sfSequence, 7); + obj.setFieldAmount (sfFee, STAmount (2557891634ull)); + obj.setFieldVL (sfSigningPubKey, keypair.first.slice()); + obj.setFieldU32 (sfSetFlag, 0x0DDBA11); + obj.setFieldU32 (sfClearFlag, 0xB01DFACE); + }); + + Serializer serialized {acctSet.getSerializer()}; + { + // Verify we have a valid transaction. + SerialIter sit {serialized.slice()}; + auto stx = std::make_shared(sit); + } + + // Tweak the serialized data to change the ClearFlag to + // a SetFlag. This will leave us with two SetFlag fields + // which we should trap as a duplicate field. + BEAST_EXPECT (serialized.modData()[15] == sfClearFlag.fieldValue); + serialized.modData()[15] = sfSetFlag.fieldValue; + + SerialIter sit {serialized.slice()}; + try + { + auto stx = std::make_shared(sit); + fail("An exception should have been thrown"); + } + catch (std::exception const& ex) + { + BEAST_EXPECT( + strcmp(ex.what(), "Duplicate field detected") == 0); + } + } + + // Exercise the three raw transactions. try { protocol::TMTransaction tx2; @@ -1253,10 +1422,9 @@ public: auto stx = std::make_shared(sit); fail("An exception should have been thrown"); } - catch (std::exception const& e) + catch (std::exception const& ex) { - BEAST_EXPECT(strcmp(e.what(), - "Maximum nesting depth of STVar exceeded") == 0); + BEAST_EXPECT(strcmp(ex.what(), "Unknown field") == 0); } try @@ -1265,22 +1433,20 @@ public: auto stx = std::make_shared(sit); fail("An exception should have been thrown"); } - catch (std::exception const& e) + catch (std::exception const& ex) { - BEAST_EXPECT(strcmp(e.what(), - "Maximum nesting depth of STVar exceeded") == 0); + BEAST_EXPECT(strcmp(ex.what(), "Unknown field") == 0); } try { - ripple::SerialIter sit (Slice{dupField, sizeof(dupField)}); + ripple::SerialIter sit {payload3}; auto stx = std::make_shared(sit); fail("An exception should have been thrown"); } - catch (std::exception const& e) + catch (std::exception const& ex) { - BEAST_EXPECT(strcmp(e.what(), - "Duplicate field detected") == 0); + BEAST_EXPECT(strcmp(ex.what(), "Unknown field") == 0); } }