Protocol Amendment: Always Require Fully-Canonical Signatures

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 @@ public:
     /** 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 @@ public:
         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);