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

Include leaf index in LeafNodeTBS for better parent-hash guarantees #731

Merged
merged 1 commit into from Oct 3, 2022

Conversation

TWal
Copy link
Contributor

@TWal TWal commented Aug 3, 2022

A problem in parent-hash proofs

The goal of #527 and #713 was to have a property similar to:

If there is a parent-hash chain from the leaf L to a node N, then the signature of L binds the tree Canonicalize(N)

In #713, I proved that the pull-request allowed to prove the following property:

If Canonicalize(U1) = Canonicalize(U2),
and there is a parent-hash link from U1 to P1 and from U2 to P2,
then Canonicalize(P1) = Canonicalize(P2)

With this property, the hope is to inductively show that the leaf signature binds the canonicalization of subtrees it is parent-hash linked from.
The induction step works fine thanks to the previous theorem, however the base case is wrong.

The hypothesis "Canonicalize(U1) = Canonicalize(U2)" says that the content of the trees are equal, and also says they have the same location (i.e. they have the same node index). This is important for the induction step.
However, since the signature of a leaf doesn't bind its position in the tree, we can't initialize the induction.

This PR resolves this problem.

A concrete counter-example

Assume we have the following tree:

      Y
    __|__
   /     \
  _       Z
 / \     / \
A   _   C   D

If there is a parent-hash link from A to Y, then the following tree is also valid:

      Y
    __|__
   /     \
  _       Z
 / \     / \
_   A   C   D

The solution

We only need to add the leaf index inside the signature when the source is a commit.
I also added it in the update case because it doesn't hurt, but that's not necessary.

Copy link
Member

@beurdouche beurdouche left a comment

Choose a reason for hiding this comment

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

Nice work Théophile !

Copy link
Member

@raphaelrobert raphaelrobert left a comment

Choose a reason for hiding this comment

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

Thanks, nice work!

I think this is particularly relevant for the update case when new joiners examine the public tree. For the commit case, the position of the committer in the tree might be implied by the parent hash (unless I understood it wrong). Either way, it can't hurt to be explicit here.

@TWal
Copy link
Contributor Author

TWal commented Aug 3, 2022

In this message I will be using the convention used in https://messaginglayersecurity.rocks/mls-protocol/draft-ietf-mls-protocol.html#name-verifying-parent-hashes

The position of the committer is indeed implied by the parent hash when there is no filtered node:

  P
 / \
D   S

In this case, the parent-hash link is from D to P, and we have C = D (no blank nodes between D and P).
The position of S is included inside its tree hash, which is therefore included in the parent-hash link D ~> P, and from that we can deduce the position of D (it is the child of P that is not S).

However, if there are filtered nodes between D and P, we have the following situation:

      P
     / \
C = _   S
   /   / \
  …   …   …
 /
D

In this case, the position of P is still included in the parent-hash link D ~> P for the same reason. However, we have no information on the position of D, because it can be anywhere inside C's subtree

Copy link
Collaborator

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

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

Thanks @TWal, this makes sense to me. The only thought that came to mind was whether there was any other similar information we should bind in here at the same time, but nothing else occurred to me within the constraint that it has to be known to new joiners.

TWal added a commit to Inria-Prosecco/mls-star that referenced this pull request Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants