Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DIP: aj-tx-vuln for enforcing transaction hygiene / rejecting vulnerable txes #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

coolaj86
Copy link

@coolaj86 coolaj86 commented Jun 27, 2023

Formalize enforcement of security vulnerability fix as per "Lexicographical Indexing of Transaction Inputs and Outputs".

Preview

https://github.com/dashhive/DIPs/blob/dip-aj-tx-vuln/dip-aj-tx-vuln.md

TL;DR

A naive implementation of construction a transaction leaks information about the senders and receivers.

This is a known vulnerability which is fixed by "Lexicographical Indexing of Transaction Inputs and Outputs", however it is NOT ENFORCED by the nodes that are verifying transactions.

This summarizes the technical details of "Lexicographical Indexing of Transaction Inputs and Outputs", and provides a path forward to enforcing that vulnerable messages from poorly implemented clients are rejected.

@coolaj86 coolaj86 changed the title DIP: aj-tx-vuln for enforcing transaction higiene / rejecting vulnerable txes DIP: aj-tx-vuln for enforcing transaction hygiene / rejecting vulnerable txes Jun 27, 2023
@kxcd
Copy link

kxcd commented Jun 27, 2023

Regarding 598fa5c477ebef8ab74979c8a51d159d8c028b332af973020ec58ec807f009cc this is an exchange hot wallet paying out two withdrawals and sending the change back to its originating hot wallet, this is not a bug and the ability to create such a TX is a feature. We also create such TXes on the MNOwatch wallets to ensure full transparency to the community, so they can see exactly how we are spending our money and where it is going.

@coolaj86
Copy link
Author

coolaj86 commented Jun 28, 2023

That's not what this is about.

This is about enforcing the Lexicographical Sort security update that was introduced back in 2015 to not reveal excess information about senders and receivers through the improper positioning in list indexes.

outputs are sorted, completely deterministically, as specified in
"Lexicographical Indexing of Transaction Inputs and Outputs".

Transactions with version 3 or lower SHOULD print a warning and MUST return an
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who / what is printing this warning?

(NOTE: this is compared in the _SAME_ byte order as written in the transaction)
- The length SHOULD be checked before comparing

As a side benefit, this also makes it possible to index transactions for
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
As a side benefit, this also makes it possible to index transactions for
As a side benefit, this also makes it possible to index transactions more

much information is made available without any breaking changes to existing
Master Nodes:

Before a transaction is signed:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not clear what this means


For example, consider
[598fa5c477ebef8ab74979c8a51d159d8c028b332af973020ec58ec807f009cc](https://insight.dash.org/insight/tx/598fa5c477ebef8ab74979c8a51d159d8c028b332af973020ec58ec807f009cc),
which was selected while sampling transactions at random just minutes ago, and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
which was selected while sampling transactions at random just minutes ago, and
which was selected while sampling transactions at random, and

Comment on lines +93 to +94
- there are two recipients
- the sender's change address is `Xf7JKrFdbQaWTZszRsQthm2s4xXCX2uZRb`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain how you know this in the dip?

Comment on lines +64 to +65
1.0 recipient 1 or xpub 1
0.7 recipient 2 or xpub 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate "xpub 1"

Comment on lines +91 to +94
We can know, with very high confidence that:

- there are two recipients
- the sender's change address is `Xf7JKrFdbQaWTZszRsQthm2s4xXCX2uZRb`
Copy link
Member

@PastaPastaPasta PastaPastaPasta Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is VIRTUALLY impossible to be the case as one input and output both have the same address XmZQkfLtk3xLtbBMenTdaZMxsUBYAsRz1o that seems significantly more likely to be the change don't we think?

Copy link
Author

@coolaj86 coolaj86 Jul 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned above, this was a typo. I had several txes open in the inspector and I copied from the wrong example.

I can go back and find the example that's typical of what I'm talking about. Picking 4 non-CoinJoin TXes at random, one of them will use the vulnerable naive implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants