Protocol Amendment: Always Require Fully-Canonical Signatures

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

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);

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

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<bool, std::string>
-    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<bool, std::string> checkSingleSign () const;
-    std::pair<bool, std::string> checkMultiSign () const;
+    std::pair<bool, std::string>
+    checkSingleSign (RequireFullyCanonicalSig requireCanonicalSig) const;
+
+    std::pair<bool, std::string>
+    checkMultiSign (RequireFullyCanonicalSig requireCanonicalSig) const;
 
     uint256 tid_;
     TxType tx_type_;

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

src/ripple/protocol/impl/STTx.cpp

@@ -22,6 +22,7 @@
 #include <ripple/basics/Log.h>
 #include <ripple/basics/safe_cast.h>
 #include <ripple/basics/StringUtilities.h>
+#include <ripple/protocol/Feature.h>
 #include <ripple/protocol/HashPrefix.h>
 #include <ripple/protocol/jss.h>
 #include <ripple/protocol/Protocol.h>
@@ -177,7 +178,8 @@ void STTx::sign (
     tid_ = getHash(HashPrefix::transactionID);
 }
 
-std::pair<bool, std::string> STTx::checkSign() const
+std::pair<bool, std::string>
+STTx::checkSign(RequireFullyCanonicalSig requireCanonicalSig) const
 {
     std::pair<bool, std::string> ret {false, ""};
     try
@@ -186,7 +188,9 @@ std::pair<bool, std::string> 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<bool, std::string> STTx::checkSingleSign () const
+std::pair<bool, std::string>
+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<bool, std::string> 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<bool, std::string> STTx::checkSingleSign () const
     return {true, ""};
 }
 
-std::pair<bool, std::string> STTx::checkMultiSign () const
+std::pair<bool, std::string>
+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<bool, std::string> 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);

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 <ripple/app/tx/apply.h>
+#include <ripple/basics/StringUtilities.h>
+#include <ripple/protocol/Feature.h>
+#include <test/jtx/Env.h>
+
+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<STTx const> (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

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);