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

Prevent second preimage attack #16

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

InoMurko
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d2970f1 on omisego:prevent_second_preimage_attack into e73c4b1 on yosriady:master.

@yosriady
Copy link
Owner

Looking good! Do you mind sharing briefly about:

  • The second preimage attack, and
  • How the introduction of the salt parameters help address it?

In order to eventually merge and release the new changes, we'd need to document it for users.

@pgebal
Copy link
Contributor

pgebal commented Jan 23, 2020

Hi @yosriady. First of all, thanks for the work you put into MerkleTree.

In the second preimage attack attacker can leverage the fact that we do not differentiate between nodes and leafs in the following way:
Let's have the following tree:

r
n1 n2
l1 l2 l3 l4, where l* are leaves before applying the hash function to them.

Knowing the hash function and images of leafs: [hash(l1), hash(l2)] attacker can shorten the proof and instead of providing a proof hash(l2)-n2, can provide (hash(l1) <> hash(l2)) as a leaf and a shortened proof n2. hash((hash(l1) <> hash(l2))) = n1, hash(n1, n2) = r
That potentailly could lead to claims that hash(l1) <> hash(l2)) is a leaf in the Merkle tree.

The proposed fix prevents that because now attacker would have to find x such that hash(0 <> x) = n1 and hash(1 <> n1 <> n2) = r, and we assume that's difficult.
If my explanation is not clear, please take a look here: https://en.wikipedia.org/wiki/Merkle_tree#Second_preimage_attack or I'll try to explain that in more detail.

@yosriady
Copy link
Owner

@pgebal Noted 👍

@InoMurko InoMurko marked this pull request as ready for review January 24, 2020 10:45
@InoMurko
Copy link
Contributor Author

InoMurko commented Jan 28, 2020

hey @yosriady what do you think about merging this? anything else we could do or provide?

thanks!

@yosriady
Copy link
Owner

@InoMurko Looks good. Are there are any breaking changes (e.g. hash_leaves) or are the changes backwards compatible?

@pgebal
Copy link
Contributor

pgebal commented Jan 29, 2020

@yosriady
there are 2 breaking changes:

  • deprecated function MerkleTree.Proof.proven?/3 is removed
  • optional argument :hash_leaves for MerkleTree.fast_root and MerkleTree.build is removed. If we would leave that option, attack prevention by mixing salts would not make sense.

So it's not backwards compatible.

@yosriady
Copy link
Owner

@pgebal Noted, thanks for confirming. I'll publish a new major version in light of these changes.

@yosriady yosriady merged commit 5dfaf8a into yosriady:master Jan 30, 2020
@yosriady
Copy link
Owner

The 2.0.0 release with your changes has been published to Hex: https://hex.pm/packages/merkle_tree

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.

None yet

4 participants