search

aido / app-seed-tool

A Ledger application that provides some useful seed management utilities
Apache License 2.0
42 stars 2 forks source link

Device freezing if "1" was selected as shares threshold #36

Open DmKoshelek opened 3 months ago

DmKoshelek commented 3 months ago

Today during testing #32 issue I find out that I can select "1" as a number of thresholds. If you think about that it is the same as sharing whole seed phrase, but I decided to try to generate shares anyway. And device have just been frozen.

Steps to reproduce the behavior:

  1. Open Seed App
  2. Select "Check BIP39 phase"
  3. Input the seed phrase and complete check
  4. Select number of shares - 2
  5. Select number of threshold - 1

Actual result: Device is frozen Expected result: You can't select "1" as a threshold

Device: Ledger Nano S+ Firmware version: 1.1.1 App Version v1.7.4-rc.2

photo_2024-06-16_19-06-23

aido commented 3 months ago

Thanks again for this issue report.

This does not happen when testing with Speculos on a Ledger Nano S+ but I shall investigate further to see what the difference is between Speculos and a physical device. I will also test on a Ledger Nano S to see if there are differences between devices.

aido commented 3 months ago

Further investigation reveals that this issue does in fact happen when testing with Speculos. It also seems to occur on all devices. This will make testing and troubleshooting much much easier and quicker. The problem is most likely to be some edge case that needs to be caught. But we'll see.

aido commented 3 months ago

Problem found.

The cause of the issue is this error check: https://github.com/aido/app-seed-tool/blob/6a841cfc3812f23cf655a19cb582c4b6651c99e6/src/sskr/sskr.c#L193-L194

The SSKR_ERROR_INVALID_SINGLETON_MEMBER error code means:

SSKR_ERROR_INVALID_SINGLETON_MEMBER: if any group with threshold 1 has a count greater than 1.

I now need to check if the SSKR standard actually allows a threshold of 1 when share count is greater than 1. If not then the option should be disabled in the app. Disabled as opposed to throwing an error as it is currently doing.

A quick look at the SSKR standard documentation does not mention this condition but for security reasons it makes sense that such a condition may exist. I have queried the Blockchain Commons developer community on this issue: BlockchainCommons/Gordian-Developer-Community#129

aido commented 3 months ago

seedtool-cli confirms that 1-of-2 is not allowed:

aido@dev$ seedtool --in hex 59f2293a5bce7d4de59e71b4207ac5d2 --out sskr --group=2-of-3
tuna acid epic gyro free able able acid able race iris play yoga good lamb solo apex cook inch wave quad tent hang wall yurt many nail deli chef
tuna acid epic gyro free able able acid acid gear wall body apex meow miss gift wand jowl bulb dark void keys keno tiny math item gear fizz away
tuna acid epic gyro free able able acid also grim jugs sets buzz scar quiz twin song zest list idle claw monk buzz menu down list buzz cost soap
aido@dev$
aido@dev$ seedtool --in hex 59f2293a5bce7d4de59e71b4207ac5d2 --out sskr --group=1-of-1
tuna acid epic gyro soap mild able able able hawk whiz diet fact help taco kiwi gift view noon jugs quiz crux kiln silk tied brew cusp noon wasp
aido@dev$
aido@dev$ seedtool --in hex 59f2293a5bce7d4de59e71b4207ac5d2 --out sskr --group=1-of-2
seedtool: Invalid group specifier. 1-of-M groups where M > 1 are not supported.
Try `seedtool --help' or `seedtool --usage' for more information.
aido commented 3 months ago

Hi @DmKoshelek,

The latest commit fixes this issue and it will be included in v1.7.4.

Instead of ungracefully freezing the app now gracefully provides a warning if a user chooses 1-of-m shares when m > 1.