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

Remove unnecessary index shuffling #10

Closed

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Aug 8, 2023

The implementation currently produces share indexes by shuffling an ordered index list. This shuffling is unnecessary, since the security of the design does not require such a distribution of share indexes. Further, the shuffling method used is biased, which was presumably not intended.

This PR simplifies the design by removing the shuffling entirely and issuing shares in numerical order. The change also makes it much easier to assert that a zero-indexed share (which would reveal the secret) is never produced.

@asta-li
Copy link
Contributor

asta-li commented Aug 10, 2023

I agree that the index shuffling is not required by the security of the design. However, it is not a vulnerability. As noted, this implementation correctly avoids unintentional use of a zero evaluation.

I would prefer to leave this implementation as-is for two primary reasons.

  1. It matches closely the index shuffling performed in the original reference implementation in https://github.com/hashicorp/vault/blob/main/shamir/shamir.go#L184. Preserving parallelism with an existing, trusted, and audited solution increases trust in this implementation.
  2. The existing library including this code has undergone at least two full security audits. Any new code changes may introduce unintended and unreviewed issues. The threshold for modifying previously audited code should be high.

@asta-li asta-li closed this Aug 10, 2023
@AaronFeickert
Copy link
Contributor Author

Understood. It may at least be worth a comment in the code that the shuffling is biased, but that security doesn't depend on this. Otherwise, a reader might use the biased shuffle elsewhere where security could be affected.

Might make a corresponding issue/PR on the linked repository if the same technique is used.

@AaronFeickert
Copy link
Contributor Author

Interestingly, upstream implemented the shuffle for a reason, in particular to avoid leaking information about the number of shares. This wasn't for a particular threat model, and it's unclear if such a leak would be of concern in this implementation.

The shuffle in the Go implementation uses a library function that internally uses a Fisher-Yates-type shuffle. They elect to use an insecure seed for the shuffle for unclear reasons.

@AaronFeickert AaronFeickert deleted the remove-index-shuffle branch August 10, 2023 19:22
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.

2 participants