From ec137044a014530263cd3309d81643a5a3c1fdab Mon Sep 17 00:00:00 2001 From: Mo Morsi Date: Tue, 11 Feb 2020 16:16:50 -0500 Subject: [PATCH] Protocol Amendment: Always Require Fully-Canonical Signatures --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/tx/impl/apply.cpp | 7 ++- src/ripple/protocol/Feature.h | 4 ++ src/ripple/protocol/STTx.h | 14 ++++- src/ripple/protocol/impl/Feature.cpp | 4 ++ src/ripple/protocol/impl/STTx.cpp | 23 ++++++-- src/test/app/tx/apply_test.cpp | 83 ++++++++++++++++++++++++++++ src/test/protocol/STTx_test.cpp | 3 +- 8 files changed, 128 insertions(+), 11 deletions(-) create mode 100644 src/test/app/tx/apply_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 6e7feb69af0..3bf641b38cb 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -687,6 +687,7 @@ target_sources (rippled PRIVATE src/test/app/ValidatorKeys_test.cpp src/test/app/ValidatorList_test.cpp src/test/app/ValidatorSite_test.cpp + src/test/app/tx/apply_test.cpp #[===============================[ test sources: subdir: basics diff --git a/src/ripple/app/tx/impl/apply.cpp b/src/ripple/app/tx/impl/apply.cpp index 169adc03f87..0c56f8fb17a 100644 --- a/src/ripple/app/tx/impl/apply.cpp +++ b/src/ripple/app/tx/impl/apply.cpp @@ -47,7 +47,12 @@ checkValidity(HashRouter& router, if (!(flags & SF_SIGGOOD)) { // Don't know signature state. Check it. - auto const sigVerify = tx.checkSign(); + auto const requireCanonicalSig = + rules.enabled(featureRequireFullyCanonicalSig) ? + STTx::RequireFullyCanonicalSig::yes : + STTx::RequireFullyCanonicalSig::no; + + auto const sigVerify = tx.checkSign(requireCanonicalSig); if (! sigVerify.first) { router.setFlags(id, SF_SIGBAD); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 5f00cebf1e1..ac41b76dbed 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -110,6 +110,8 @@ class FeatureCollections "DeletableAccounts", // fixQualityUpperBound should be activated before FlowCross "fixQualityUpperBound", + "fix1781", // XRPEndpointSteps should be included in the circular payment check + "RequireFullyCanonicalSig" }; std::vector features; @@ -397,6 +399,8 @@ extern uint256 const fixCheckThreading; extern uint256 const fixPayChanRecipientOwnerDir; extern uint256 const featureDeletableAccounts; extern uint256 const fixQualityUpperBound; +extern uint256 const fix1781; +extern uint256 const featureRequireFullyCanonicalSig; } // ripple diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h index 8c4e7ae0923..ed3bc5b4839 100644 --- a/src/ripple/protocol/STTx.h +++ b/src/ripple/protocol/STTx.h @@ -132,8 +132,13 @@ class STTx final /** Check the signature. @return `true` if valid signature. If invalid, the error message string. */ + enum class RequireFullyCanonicalSig : bool + { + no, + yes + }; std::pair - checkSign() const; + checkSign(RequireFullyCanonicalSig requireCanonicalSig) const; // SQL Functions with metadata. static @@ -150,8 +155,11 @@ class STTx final std::string const& escapedMetaData) const; private: - std::pair checkSingleSign () const; - std::pair checkMultiSign () const; + std::pair + checkSingleSign (RequireFullyCanonicalSig requireCanonicalSig) const; + + std::pair + checkMultiSign (RequireFullyCanonicalSig requireCanonicalSig) const; uint256 tid_; TxType tx_type_; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 1e5d30f9784..7b2191bbb40 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -129,6 +129,8 @@ detail::supportedAmendments () "fixPayChanRecipientOwnerDir", "DeletableAccounts", "fixQualityUpperBound", + "fix1781", + "RequireFullyCanonicalSig" }; return supported; } @@ -187,5 +189,7 @@ uint256 const fixCheckThreading = *getRegisteredFeature("fixCheckThreading"); uint256 const fixPayChanRecipientOwnerDir = *getRegisteredFeature("fixPayChanRecipientOwnerDir"); uint256 const featureDeletableAccounts = *getRegisteredFeature("DeletableAccounts"); uint256 const fixQualityUpperBound = *getRegisteredFeature("fixQualityUpperBound"); +uint256 const fix1781 = *getRegisteredFeature("fix1781"); +uint256 const featureRequireFullyCanonicalSig = *getRegisteredFeature("RequireFullyCanonicalSig"); } // ripple diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp index ffdfed55554..fac651cfe94 100644 --- a/src/ripple/protocol/impl/STTx.cpp +++ b/src/ripple/protocol/impl/STTx.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -177,7 +178,8 @@ void STTx::sign ( tid_ = getHash(HashPrefix::transactionID); } -std::pair STTx::checkSign() const +std::pair +STTx::checkSign(RequireFullyCanonicalSig requireCanonicalSig) const { std::pair ret {false, ""}; try @@ -186,7 +188,9 @@ std::pair STTx::checkSign() const // at the SigningPubKey. It it's empty we must be // multi-signing. Otherwise we're single-signing. Blob const& signingPubKey = getFieldVL (sfSigningPubKey); - ret = signingPubKey.empty () ? checkMultiSign () : checkSingleSign (); + ret = signingPubKey.empty () ? + checkMultiSign (requireCanonicalSig) : + checkSingleSign (requireCanonicalSig); } catch (std::exception const&) { @@ -250,7 +254,8 @@ STTx::getMetaSQL (Serializer rawTxn, % getSequence () % inLedger % status % rTxn % escapedMetaData); } -std::pair STTx::checkSingleSign () const +std::pair +STTx::checkSingleSign (RequireFullyCanonicalSig requireCanonicalSig) const { // We don't allow both a non-empty sfSigningPubKey and an sfSigners. // That would allow the transaction to be signed two ways. So if both @@ -261,7 +266,10 @@ std::pair STTx::checkSingleSign () const bool validSig = false; try { - bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig); + bool const fullyCanonical = + (getFlags() & tfFullyCanonicalSig) || + (requireCanonicalSig == RequireFullyCanonicalSig::yes); + auto const spk = getFieldVL (sfSigningPubKey); if (publicKeyType (makeSlice(spk))) @@ -287,7 +295,8 @@ std::pair STTx::checkSingleSign () const return {true, ""}; } -std::pair STTx::checkMultiSign () const +std::pair +STTx::checkMultiSign (RequireFullyCanonicalSig requireCanonicalSig) const { // Make sure the MultiSigners are present. Otherwise they are not // attempting multi-signing and we just have a bad SigningPubKey. @@ -314,7 +323,9 @@ std::pair STTx::checkMultiSign () const auto const txnAccountID = getAccountID (sfAccount); // Determine whether signatures must be full canonical. - bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig); + bool const fullyCanonical = + (getFlags() & tfFullyCanonicalSig) || + (requireCanonicalSig == RequireFullyCanonicalSig::yes); // Signers must be in sorted order by AccountID. AccountID lastAccountID (beast::zero); diff --git a/src/test/app/tx/apply_test.cpp b/src/test/app/tx/apply_test.cpp new file mode 100644 index 00000000000..2113f333ab4 --- /dev/null +++ b/src/test/app/tx/apply_test.cpp @@ -0,0 +1,83 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2020 Dev Null Productions + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include +#include + +namespace ripple { + +class Apply_test : public beast::unit_test::suite +{ +public: + void run() override + { + testcase ("Require Fully Canonicial Signature"); + testFullyCanonicalSigs(); + } + + void testFullyCanonicalSigs() + { + // Construct a payments w/out a fully-canonical tx + const std::string non_fully_canonical_tx = + "12000022000000002400000001201B00497D9C6140000000000F6950684000000" + "00000000C732103767C7B2C13AD90050A4263745E4BAB2B975417FA22E87780E1" + "506DDAF21139BE74483046022100E95670988A34C4DB0FA73A8BFD6383872AF43" + "8C147A62BC8387406298C3EADC1022100A7DC80508ED5A4750705C702A81CBF9D" + "2C2DC3AFEDBED37BBCCD97BC8C40E08F8114E25A26437D923EEF4D6D815DF9336" + "8B62E6440848314BB85996936E4F595287774684DC2AC6266024BEF"; + + auto ret = strUnHex (non_fully_canonical_tx); + SerialIter sitTrans (makeSlice(*ret)); + STTx const tx = *std::make_shared (std::ref (sitTrans)); + + { + test::jtx::Env no_fully_canonical (*this, + test::jtx::supported_amendments() - + featureRequireFullyCanonicalSig); + + Validity valid = checkValidity(no_fully_canonical.app().getHashRouter(), + tx, + no_fully_canonical.current()->rules(), + no_fully_canonical.app().config()).first; + + if(valid != Validity::Valid) + fail("Non-Fully canoncial signature was not permitted"); + } + + { + test::jtx::Env fully_canonical (*this, + test::jtx::supported_amendments()); + + Validity valid = checkValidity(fully_canonical.app().getHashRouter(), + tx, + fully_canonical.current()->rules(), + fully_canonical.app().config()).first; + if(valid == Validity::Valid) + fail("Non-Fully canoncial signature was permitted"); + } + + pass(); + } +}; + +BEAST_DEFINE_TESTSUITE(Apply,app,ripple); + +} // ripple diff --git a/src/test/protocol/STTx_test.cpp b/src/test/protocol/STTx_test.cpp index a37064b15bb..65e9a0ef2c6 100644 --- a/src/test/protocol/STTx_test.cpp +++ b/src/test/protocol/STTx_test.cpp @@ -1489,7 +1489,8 @@ class STTx_test : public beast::unit_test::suite }); j.sign (keypair.first, keypair.second); - unexpected (!j.checkSign().first, "Transaction fails signature test"); + unexpected (!j.checkSign(STTx::RequireFullyCanonicalSig::yes).first, + "Transaction fails signature test"); Serializer rawTxn; j.add (rawTxn);