Protocol Amendment: Always Require Fully-Canonical Signatures #3256

movitto · 2020-02-11T21:50:50Z

Addresses #3042 by adding an amendment which if enabled would require full-canonical signatures regardless of whether or not the tfFullyCanonicalSig flag is set. Existing functionality will be preserved if amendment is not enabled.

movitto · 2020-02-11T21:51:37Z

Incidentally, first #XRPCommunity amendment?

codecov-io · 2020-02-11T22:41:51Z

Codecov Report

Merging #3256 into develop will decrease coverage by 0.2%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #3256      +/-   ##
===========================================
- Coverage    70.33%   70.12%   -0.21%     
===========================================
  Files          675      674       -1     
  Lines        53178    53612     +434     
===========================================
+ Hits         37402    37598     +196     
- Misses       15776    16014     +238

Impacted Files	Coverage Δ
src/ripple/protocol/Feature.h	`100% <ø> (ø)`	⬆️
src/ripple/protocol/STTx.h	`64.7% <ø> (ø)`	⬆️
src/ripple/protocol/impl/Feature.cpp	`91.66% <ø> (ø)`	⬆️
src/ripple/app/tx/impl/apply.cpp	`81.66% <100%> (+0.63%)`	⬆️
src/ripple/protocol/impl/STTx.cpp	`83.91% <100%> (+0.21%)`	⬆️
src/ripple/app/misc/impl/ValidatorList.cpp	`78.05% <0%> (-11.14%)`	⬇️
src/ripple/basics/impl/FileUtilities.cpp	`69.23% <0%> (-7.24%)`	⬇️
src/ripple/basics/impl/StringUtilities.cpp	`89.18% <0%> (-3.92%)`	⬇️
src/ripple/peerfinder/impl/PeerfinderManager.cpp	`53.93% <0%> (-1.63%)`	⬇️
src/ripple/peerfinder/impl/Counts.h	`52.74% <0%> (-1.19%)`	⬇️
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc4cefa...aedf91a. Read the comment docs.

nbougalis

Lean and mean. I like this. But it needs more thorough unit tests.

nbougalis · 2020-02-12T03:47:15Z

src/ripple/protocol/impl/STTx.cpp

@@ -186,7 +187,7 @@ 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 (rules) : checkSingleSign (rules);


I think that for these two private member functions, it's a better idea to pass an actual bool instead of the actual Rules object.

MarkusTeufelberger · 2020-02-12T12:15:17Z

Incidentally, first #XRPCommunity amendment?

Only if you consider Ripple not as a part of the community. ;-)

scottschurr · 2020-02-13T09:39:36Z

The non-unity CI builds are failing. It looks like, at a minimum, STTx.cpp is missing an include of Feature.h. Here the the output from the one of the non-unity builds:

../../src/ripple/protocol/impl/STTx.cpp:265:89: error: 'featureRequireFullyCanonicalSig' was not declared in this scope
         bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || rules.enabled(featureRequireFullyCanonicalSig);
                                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/ripple/protocol/impl/STTx.cpp:265:89: note: suggested alternative: 'tfFullyCanonicalSig'
         bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || rules.enabled(featureRequireFullyCanonicalSig);
                                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                         tfFullyCanonicalSig
../../src/ripple/protocol/impl/STTx.cpp: In member function 'std::pair<bool, std::__cxx11::basic_string<char> > ripple::STTx::checkMultiSign(const ripple::Rules&) const':
../../src/ripple/protocol/impl/STTx.cpp:318:85: error: 'featureRequireFullyCanonicalSig' was not declared in this scope
     bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || rules.enabled(featureRequireFullyCanonicalSig);
                                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/ripple/protocol/impl/STTx.cpp:318:85: note: suggested alternative: 'tfFullyCanonicalSig'
     bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) || rules.enabled(featureRequireFullyCanonicalSig);
                                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                     tfFullyCanonicalSig

movitto · 2020-02-13T16:48:54Z

Lean and mean. I like this. But it needs more thorough unit tests.

@nbougalis could you advise as to how to generate non-canonical signatures for testing purposes? Can ripple-lib be modified to do so?

movitto · 2020-02-13T16:51:27Z

Incidentally, first #XRPCommunity amendment?

Only if you consider Ripple not as a part of the community. ;-)

D'oh! Should've said first non-ripple #XRPCommunity ammendment! 🤦‍♂️

movitto · 2020-02-13T17:21:05Z

The non-unity CI builds are failing. It looks like, at a minimum, STTx.cpp is missing an include of Feature.h. Here the the output from the one of the non-unity builds:

Pushed a followon patch fixing the unity build

intelliot · 2020-02-13T20:47:31Z

I don't think ripple-lib can generate non-canonical signatures. It may be possible to modify it to do so, but I'm not knowledgeable about how to do that. Searches turned up the following resources, in case they are helpful. I imagine @nbougalis knows more.

scottschurr · 2020-02-21T00:29:42Z

First and foremost, thanks for doing this! I think you've made a great change. This is yet another case where you're doing something that we should have taken care of years ago. I appreciate it.

That said, there are a couple of changes that I think need to be made. They are:

Introducing Rules into STTx breaks levelization. I wouldn't have noticed, but fortunately it caused the validator-keys CI build to break. I think a good way to fix this would be to check the Rules in apply.cpp before the call into STTx. Then pass a flag into STTx, rather than passing all of the Rules. You can find an example of how I'd approach that problem here: scottschurr@ff6aa77. You're welcome to cherry-pick that, but you can also implement it yourself without much trouble.
The harder problem is that we need a unit test to verify the functionality that you're putting in. This is tricky, since you need a non-fully-canonical transaction signature and none of our tools currently produce that (as far as I know).

I don't think all is lost, however. I talked with @nbougalis and possibly, with some multi precision math, you can turn a fully-canonical signature into a valid non-fully-canonical signature. If you're interested in making a run at that, here are a couple of resources to checkout:
- This documentation page gives an overview of what the difference is between a canonical signature and a fully-canonical signature: https://xrpl.org/transaction-malleability.html
- Here's the code that checks how canonical a signature is: https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/impl/PublicKey.cpp#L115-L152. It may give you a sense for how to organize the math.

I haven't actually tried turning a fully-canonical signature into a non-fully-canonical signature using this technique. So I can't guarantee that it will work. Also, if you feel like figuring this out is above and beyond the call of duty then let me know and I'll try to put together a couple of unit tests.

I think you have a good change here. It's very much worth my time to help you get this change into the code base.

Let me know if you want any help. And thanks again.

movitto · 2020-02-21T20:04:23Z

@nbougalis @scottschurr OK I was able to generate a TX w/ a non-fully-canonical signature (also w/out the tfFullyCanonicalSig flag). I pushed a follow on patch adding a test case w/ that, verifying the transaction fails to be verified when the amendment is enabled, and succeeds when that amendment is disabled.

Also cherry picked @scottschurr's patch introducing the new "RequireFullyCanonicalSig" enum and added a followon patch updating the test case to use that.

Let me know if there is anything else!

scottschurr

Hi @movitto. I really appreciate you adding the uint test. And it's so close. But it doesn't actually test the change that you're adding. What you're adding is an amendment that, before it's passed, will allow a non-fully-canonically-signed transaction to slip through the system. Once the amendment is passed the non-fully-canonically-signed transaction should no longer be accepted.

Your unit test doesn't actually exercise the amendment itself. That's what I think we really want. Does that distinction make sense? If not let me know and I'll see if I can explain it better.

Thanks again for your work on this change. Once the unit test is in order I think this pull request will be good to go.

scottschurr · 2020-02-21T22:53:59Z

src/test/protocol/STTx_test.cpp

+    {
+        // Construct a payments w/out a fully-canonical tx
+        const std::string non_fully_canonical_tx =
+          "12000022000000002400000001201B00497D9C6140000000000F695068400000000000000C732103767C7B2C13AD90050A4263745E4BAB2B975417FA22E87780E1506DDAF21139BE74483046022100E95670988A34C4DB0FA73A8BFD6383872AF438C147A62BC8387406298C3EADC1022100A7DC80508ED5A4750705C702A81CBF9D2C2DC3AFEDBED37BBCCD97BC8C40E08F8114E25A26437D923EEF4D6D815DF93368B62E6440848314BB85996936E4F595287774684DC2AC6266024BEF";


Nit: This is a really long line. You can preserve the string and still limit the line(s) to 80 characters. The compiler concatenates consecutive string literals that are not separated by commas. So you can build the string literal like this:

// Construct a payments w/out a fully-canonical tx const std::string non_fully_canonical_tx = "12000022000000002400000001201B00497D9C6140000000000F6950684000000" "00000000C732103767C7B2C13AD90050A4263745E4BAB2B975417FA22E87780E1" "506DDAF21139BE74483046022100E95670988A34C4DB0FA73A8BFD6383872AF43" "8C147A62BC8387406298C3EADC1022100A7DC80508ED5A4750705C702A81CBF9D" "2C2DC3AFEDBED37BBCCD97BC8C40E08F8114E25A26437D923EEF4D6D815DF9336" "8B62E6440848314BB85996936E4F595287774684DC2AC6266024BEF";

Note that the example I'm giving indents the string literal by four spaces, not two, since this is an indent-by-four-spaces code base.

scottschurr · 2020-02-21T22:55:07Z

src/test/protocol/STTx_test.cpp

+        {
+            bool valid = tx.checkSign(STTx::RequireFullyCanonicalSig::no).first;
+            if(!valid)
+              fail("Non-Fully canoncial signature was not permitted");


Nit: this and the following fail should be indented by four, not two, spaces.

vs testing it via internal helpers

movitto · 2020-02-22T04:43:23Z

@scottschurr pushed a patch testing the acceptability of non-canonical txs via the enablement of the amendment vs the state of the boolean in the internal helpers. Also fixes the style issues you pointed out. I believe this addresses the items you pointed out, let me know if otherwise

scottschurr

👍 Looks great. Thanks for your persistence with the unit test.

@nbougalis, are you good with the unit test? If so, then I think this is good to go.

nbougalis

Looks good, thanks. Left two micro-nits that I'd like to see addressed.

nbougalis · 2020-02-26T21:35:29Z

src/ripple/app/tx/impl/apply.cpp

+        STTx::RequireFullyCanonicalSig const requireCanonicalSig =
+            rules.enabled(featureRequireFullyCanonicalSig) ?
+            STTx::RequireFullyCanonicalSig::yes :
+            STTx::RequireFullyCanonicalSig::no;
+
+        auto const sigVerify = tx.checkSign(requireCanonicalSig);


I'd prefer: auto const requireCanonicalSig = ... whatever ... instead of STTx::RequireFullyCanonicalSig const requireCanonicalSig = ... whatever ....

Rationale: the precise type of the variable here is irrelevant and doesn't add much. This is, ultimately, a bool with some type-safety and user-friendliness sprinked on it, and a programmer can deduce the actual type by examining the ternary.

nbougalis · 2020-02-26T21:46:15Z

src/ripple/protocol/impl/STTx.cpp

-        bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig);
+        bool const fullyCanonical =
+            (getFlags() & tfFullyCanonicalSig) ||
+            (requireCanonicalSig != RequireFullyCanonicalSig::no);


I'd prefer requireCanonicalSig == RequireFullyCanonicalSig::yes here (and below in checkMultiSign). It makes it easier to read and inverting the check just makes it harder (at least for me) to reason about this.

I hear the argument. Personally, I picked != no because I know that it's possible for a bool to have values other than 0 and 1. So in case there's something unexpected in the bool it will set fullyCanonical == true. Using == yes doesn't give that guarantee.

But I don't feel strongly about it. The case is pretty unlikely.

movitto · 2020-02-27T19:39:50Z

@nbougalis pushed a followup patch incorporating your feedback

scottschurr

👍

ghost · 2020-02-29T00:03:26Z

Linux build OK after rebasing on 1.5.0-b5, unit tests passed.

Protocol Amendment: Always Require Fully-Canonical Signatures

42cbeb3

nbougalis requested review from scottschurr, HowardHinnant and a user February 12, 2020 03:25

nbougalis suggested changes Feb 12, 2020

View reviewed changes

[fold] Pass boolean flags to private methods

5625454

[fold] Fix unity build - add missing includes

197a3cb

HowardHinnant previously approved these changes Feb 19, 2020

View reviewed changes

ghost previously approved these changes Feb 20, 2020

View reviewed changes

movitto and others added 3 commits February 21, 2020 14:50

Add test case with non-fully-canonical tx, verify new amendment

8e4030f

[FOLD] Fix levelization problem in STTx

118cffa

Use RequireFullyCanonicalSig enum in canonical amendment test

61633f4

movitto dismissed stale reviews from ghost and HowardHinnant via 61633f4 February 21, 2020 20:01

scottschurr reviewed Feb 21, 2020

View reviewed changes

[FOLD] test non-canonical txs edge cases via new amendent

b796e56

vs testing it via internal helpers

scottschurr assigned HowardHinnant, scottschurr and ghost Feb 24, 2020

scottschurr previously approved these changes Feb 24, 2020

View reviewed changes

HowardHinnant previously approved these changes Feb 24, 2020

View reviewed changes

nbougalis previously approved these changes Feb 26, 2020

View reviewed changes

[FOLD] incorporate code style feedback

aedf91a

movitto dismissed stale reviews from nbougalis, HowardHinnant, and scottschurr via aedf91a February 27, 2020 19:39

nbougalis approved these changes Feb 28, 2020

View reviewed changes

scottschurr approved these changes Feb 28, 2020

View reviewed changes

ghost approved these changes Feb 29, 2020

View reviewed changes

carlhua mentioned this pull request Mar 5, 2020

Proposed: 1.5.0-b7 #3283

Merged

carlhua closed this in #3283 Mar 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protocol Amendment: Always Require Fully-Canonical Signatures #3256

Protocol Amendment: Always Require Fully-Canonical Signatures #3256

movitto commented Feb 11, 2020

movitto commented Feb 11, 2020

codecov-io commented Feb 11, 2020 •

edited

Loading

nbougalis left a comment

nbougalis Feb 12, 2020

movitto Feb 13, 2020

MarkusTeufelberger commented Feb 12, 2020

scottschurr commented Feb 13, 2020

movitto commented Feb 13, 2020

movitto commented Feb 13, 2020 •

edited

Loading

movitto commented Feb 13, 2020

intelliot commented Feb 13, 2020

scottschurr commented Feb 21, 2020

movitto commented Feb 21, 2020

scottschurr left a comment

scottschurr Feb 21, 2020

scottschurr Feb 21, 2020

movitto commented Feb 22, 2020

scottschurr left a comment

nbougalis left a comment

nbougalis Feb 26, 2020

nbougalis Feb 26, 2020

scottschurr Feb 26, 2020

movitto commented Feb 27, 2020

scottschurr left a comment

ghost commented Feb 29, 2020

Protocol Amendment: Always Require Fully-Canonical Signatures #3256

Protocol Amendment: Always Require Fully-Canonical Signatures #3256

Conversation

movitto commented Feb 11, 2020

movitto commented Feb 11, 2020

codecov-io commented Feb 11, 2020 • edited Loading

Codecov Report

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis Feb 12, 2020

Choose a reason for hiding this comment

movitto Feb 13, 2020

Choose a reason for hiding this comment

MarkusTeufelberger commented Feb 12, 2020

scottschurr commented Feb 13, 2020

movitto commented Feb 13, 2020

movitto commented Feb 13, 2020 • edited Loading

movitto commented Feb 13, 2020

intelliot commented Feb 13, 2020

scottschurr commented Feb 21, 2020

movitto commented Feb 21, 2020

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr Feb 21, 2020

Choose a reason for hiding this comment

scottschurr Feb 21, 2020

Choose a reason for hiding this comment

movitto commented Feb 22, 2020

scottschurr left a comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

nbougalis Feb 26, 2020

Choose a reason for hiding this comment

nbougalis Feb 26, 2020

Choose a reason for hiding this comment

scottschurr Feb 26, 2020

Choose a reason for hiding this comment

movitto commented Feb 27, 2020

scottschurr left a comment

Choose a reason for hiding this comment

ghost commented Feb 29, 2020

codecov-io commented Feb 11, 2020 •

edited

Loading

movitto commented Feb 13, 2020 •

edited

Loading