-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Group member check seems too restrictive when combining mnemonics #44
Comments
I think you are right. Workaround: If you have more shares than threshold, just pass the threshold shares to the library. @andrewkozlik what do you think? Should we update the code in the library? It's reference implementation, so I think it makes sense. |
That's what I initially implemented. I recall that there was then a team discussion where it was decided to be strict about the number of shares provided. I can't remember the rationale for this change, since as I recall I didn't really agree with it. I also previously had code which would automatically discard groups that are smaller than the group threshold, which I also removed in the same commit. The best that I can recall about the rationale is that the team came to a stance that the code shouldn't be too smart about processing the inputs, i.e. removing redundant information. The commit message reads:
06536e0#diff-bf3a7a9b0eedc233dfb28dd6bc7a662ae215fdf226b45f007628a7cdb6bead2fL678 Fun fact. On this pair of lines I had as a unit test exactly the code that @jfburdet is proposing: |
Functionally, changing the threshold check to a "less than" check is perfectly OK if all entered shares are correct. The only issue that I can think of when the user enters shares above the threshold is if one or more of the shares is invalid. Say the threshold is 3 and the user enters 4 shares, 3 of them are correct and 1 is invalid. The code will try to interpolate all 4 and fail with Of course this process of trying to pick subsets of size @matejcik any thoughts? |
I'm pretty sure that this is it. OTOH this seems to be before the 0.2 API rework, in particular, before
(and then later, if desired, we can call one more thought: is the validity a property of a single share, or of the whole group? (or both?) |
I'd say both. The shares are protected against accidental errors by a very strong per-share checksum. However, a malicious shareholder or attacker could supply the user with a malformed share that satisfies the per-share checksum but breaks the group checksum. There is no easy way to detect which share is the bad one. We can only detect that a bad share is present in the subset. So if we get |
Reviewing the code, This makes me think it's safe to implement the original suggestion of changing |
If I understand things well the "not equal" check here
python-shamir-mnemonic/shamir_mnemonic/shamir.py
Line 425 in c919df7
This would allow the following code to work :
If I understand spec well and sss, the threshold is the mimimum share count needed to decode the secret, so it should be allowed to pass more shares.
The text was updated successfully, but these errors were encountered: