Sign method - verify accurate encoding #1026

intelliot · 2019-08-08T20:50:43Z

When signing a transaction, we should decode the signedTransaction blob and check the integrity (accuracy) of the data.

mDuo13

I'm not sure how to test that this works as intended but I do wonder how many edge cases like with Flags might break it.

mDuo13 · 2019-08-08T21:03:15Z

src/transaction/sign.ts

+  const decoded = binary.decode(serialized)
+
+  // ...And ensure it is equal to the original tx, except:
+  // - It must have a TxnSignature or txSigners (multisign).


What about flags? The "standard" way of serializing a transaction usually adds tfFullyCanonicalSig even if the user didn't specify it. I could see that feature accidentally tripping this check.

That's a great question. I think that if the user didn't specify tfFullyCanonicalSig then ripple-lib's sign method does not add it. However, the prepare* methods -- including prepareTransaction -- do: https://github.com/ripple/ripple-lib/blob/develop/src/transaction/utils.ts#L67

I thought if you left Flags off entirely it did provide it.

I've verified that signing a transaction with ripple-lib's sign method (which also serializes it) does not add tfFullyCanonicalSig, regardless of whether the user specified Flags or not. But note that the prepare* methods do add it.

I've added a test that ensures that no Flags are added: c5714de

I think that despite the prepare* methods, the sign method should also add Flags if none are specified, mainly because our documentation says we do: https://xrpl.org/transaction-malleability.html

To further protect users, Ripple has configured its code to enable the tfFullyCanonicalSig flag by default where possible. Ripple strongly encourages third-party implementations of XRP Ledger software to generate only fully-canonical signatures, and enable tfFullyCanonicalSig on transactions by default.

@ximinez I think that's beyond the scope of this PR, but I've created a new PR that does that: #1028

intelliot · 2019-08-09T00:36:19Z

As far as I'm currently aware, the only case that this will catch occurs when an amount of XRP drops contains a decimal point; and this will be caught ahead of time with ripple-binary-codec 0.2.2. (This PR also updates ripple-lib to use ripple-binary-codec 0.2.2.) This PR incorporates the secondary check as "defense in depth". Here's how I tested it:

Created unit tests.
I verified that the unit tests pass with the secondary check alone (using the older ripple-binary-codec).
I removed the secondary check and verified that the tests fail.
I updated ripple-binary-codec and verified that the tests pass (with a small change to the tests: the error message that the test expects).
I added the secondary check back and verified that the tests still pass.

I also verified that the decoded transaction's Fee is checked (to ensure it's not higher than maxFeeXRP) by temporarily commenting out the first Fee check (which checks the original txJSON prior to signing/encoding).

keefertaylor · 2019-08-09T14:44:14Z

src/transaction/sign.ts

@@ -1,3 +1,4 @@
+import * as isEqual from '../common/js/lodash.isequal'
 import * as utils from './utils'
 import keypairs = require('ripple-keypairs')
 import binary = require('ripple-binary-codec')


optional: Consider rebasing this import to binaryCodec because it makes the code below easier to read IMO:

const decoded = binary.decode(serialized)

vs

const decoded = binaryCodec.decode(serialized)

keefertaylor · 2019-08-09T14:46:25Z

src/transaction/sign.ts

+  // - It must have a TxnSignature or txSigners (multisign).
+  if (!decoded.TxnSignature && !tx.Signers) {
+    throw new utils.common.errors.ValidationError(
+      'Serialized signed transaction is missing "TxnSignature" property'


Consider rephrasing to line up with the check you're actually performing:
"Serialized transaction must have a TxnSignature or Signers property."

keefertaylor · 2019-08-09T14:47:54Z

src/transaction/sign.ts

+  const decoded = binary.decode(serialized)
+
+  // ...And ensure it is equal to the original tx, except:
+  // - It must have a TxnSignature or txSigners (multisign).


nit: Refer to this property the same way you refer to it in code: 'Signers' vs txSigners, or at the very least 'tx.Signers'.

Or, if you think it should be called txSigners, you could refactor the code to this property name.

keefertaylor · 2019-08-09T14:48:42Z

src/transaction/sign.ts

+  }
+
+  // - We know that the original tx did not have Signers, so if it exists, we should delete it:
+  delete decoded.Signers


You've already deleted this on line 66.

src/transaction/sign.ts

test/api-test.js

keefertaylor · 2019-08-09T14:54:51Z

Some initial (mostly stylistic) comments.

keefertaylor

LGTM - Please wait for another reviewer to stamp as well since I'm still relatively new to these flows.

test/api-test.js

intelliot · 2019-08-16T23:21:11Z

Thanks to @mDuo13 for taking another look! I've also done some additional manual testing myself, and everything looks good.

jnr101 · 2019-09-23T08:17:09Z

Hi,

I am using values with standard amount of digits after the '.' like e.g. "3.141000". It turns out that this fails the verification. It was easy for me to fix, but if you feel like values with zeros should be accepted, then you might want to change it.

The following code will show the issue:

const {RippleAPI} = require('ripple-lib')

api = new RippleAPI({ server: 'wss://s1.ripple.com' });

const address = "rrrrrrrrrrrrrrrrrNAMEtxvNvQ"
const secret = "shNs3GgHxoNPRWL6chhwH5s5A97T5"

const order = {
  "direction": "sell",
  "quantity": {
    "currency": "USD",
    "counterparty": 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B',
    "value": "3.140000"
  },
  "totalPrice": {
    "currency": "XRP",
    "value": "31415"
  }
};

(async () => {
  await api.connect()
  const accountInfo = await api.getAccountInfo(address)
  const prepared = await api.prepareOrder(address, order, { "sequence": accountInfo.sequence } )
  const {signedTransaction} = api.sign(prepared.txJSON, secret);
  api.disconnect()
})().catch(err => {
  console.error(err.message, '\n', JSON.stringify(err.data, null, 2))
})

intelliot · 2019-09-23T19:12:36Z

@jnr101 Thanks for pointing that out! I put some thought into it and decided to keep the current behavior because it's risky (and potentially unexpected) for the encoded transaction to contain different values than were originally provided to the sign method. In the case of trailing zeros, you are probably doing the right thing by trimming those off prior to asking ripple-lib to sign.

To make this behavior more discoverable, I improved the error message and added a diff so that users can quickly see where the discrepancy lies: #1037

jnr101 · 2019-09-23T21:13:26Z

Hi @intelliot, sounds good to me. I have run the tests and looks good 👍

yxxyun · 2019-10-03T16:51:53Z

after this update, is this right behavior ?

intelliot · 2019-10-03T18:19:22Z

@yxxyun Correct. The underlying transaction format is in drops and thus does not allow more than 6 decimal places for XRP amounts. It would be risky for the library to silently/automatically do rounding or truncation, so apps/clients must do that prior to passing the amount to ripple-lib. Depending on the context/use case, different rounding policies are possible. Hope this helps!

yxxyun · 2019-10-04T00:47:47Z

edited,
fixed the bug in utoken.

intelliot · 2019-10-04T06:17:23Z

@yxxyun Could you create a new issue describing the problem?

Note that you can specify TakerGets and TakerPays by using OfferCreate JSON with ripple-lib's prepareTransaction method.

intelliot added 4 commits August 7, 2019 12:24

Decode signed transactions and check field integrity

8be83f0

Add tests

abd91ef

Update tests to output more descriptive errors on failure

60cd535

Update ripple-binary-codec to 0.2.2

b97bdd8

intelliot requested review from mDuo13, AyAggarwal and jwbusch August 8, 2019 20:50

mDuo13 reviewed Aug 8, 2019

View reviewed changes

Test signing a tx without Flags

c5714de

keefertaylor reviewed Aug 9, 2019

View reviewed changes

src/transaction/sign.ts Show resolved Hide resolved

keefertaylor reviewed Aug 9, 2019

View reviewed changes

test/api-test.js Show resolved Hide resolved

intelliot added 2 commits August 9, 2019 10:13

Refactoring and updates per code review

f5968b1

Merge branch 'develop' into verify-encoding-for-sign-method

61a7281

intelliot requested review from ximinez, keefertaylor and mDuo13 August 9, 2019 17:18

keefertaylor approved these changes Aug 9, 2019

View reviewed changes

test/api-test.js Show resolved Hide resolved

intelliot merged commit 5e138b9 into develop Aug 16, 2019

intelliot deleted the verify-encoding-for-sign-method branch August 19, 2019 18:14

intelliot mentioned this pull request Sep 23, 2019

Improve error message when signing fails #1037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign method - verify accurate encoding #1026

Sign method - verify accurate encoding #1026

intelliot commented Aug 8, 2019

mDuo13 left a comment

mDuo13 Aug 8, 2019

intelliot Aug 8, 2019

mDuo13 Aug 8, 2019

intelliot Aug 9, 2019

ximinez Aug 9, 2019

intelliot Aug 9, 2019

intelliot commented Aug 9, 2019

keefertaylor Aug 9, 2019

keefertaylor Aug 9, 2019

keefertaylor Aug 9, 2019

keefertaylor Aug 9, 2019

keefertaylor commented Aug 9, 2019

keefertaylor left a comment

intelliot commented Aug 16, 2019 •

edited

jnr101 commented Sep 23, 2019 •

edited

intelliot commented Sep 23, 2019 •

edited

jnr101 commented Sep 23, 2019

yxxyun commented Oct 3, 2019

intelliot commented Oct 3, 2019

yxxyun commented Oct 4, 2019 •

edited

intelliot commented Oct 4, 2019

Sign method - verify accurate encoding #1026

Sign method - verify accurate encoding #1026

Conversation

intelliot commented Aug 8, 2019

mDuo13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intelliot commented Aug 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keefertaylor commented Aug 9, 2019

keefertaylor left a comment

Choose a reason for hiding this comment

intelliot commented Aug 16, 2019 • edited

jnr101 commented Sep 23, 2019 • edited

intelliot commented Sep 23, 2019 • edited

jnr101 commented Sep 23, 2019

yxxyun commented Oct 3, 2019

intelliot commented Oct 3, 2019

yxxyun commented Oct 4, 2019 • edited

intelliot commented Oct 4, 2019

intelliot commented Aug 16, 2019 •

edited

jnr101 commented Sep 23, 2019 •

edited

intelliot commented Sep 23, 2019 •

edited

yxxyun commented Oct 4, 2019 •

edited