ilap / slip39-js

The javascript implementation of the SLIP39 for Shamir's Secret-Sharing for Mnemonic Codes.
MIT License
69 stars 27 forks source link

Feature: Mnemonic Validation #3

Closed joernroeder closed 5 years ago

joernroeder commented 5 years ago

This pull request is a follow up of issue #2 and adds a static validateMnemonic(mnemonic): Boolean method to the Slip39 class so that (slip39) mnemonics can be validated individually before trying to start seed recovery. It allows developers to test whether or not a mnemonic in itself is valid – not a group of mnemonics.

I've used relevant test vectors from vectors.json for tests but two vectors are still failing:

The error gets thrown in both cases from https://github.com/ilap/slip39-js/blob/master/src/slip39_helper.js#L259. Both tests fail due to the fact that the xor() function (and padding check) gets only called during secret recovery not during mnemonic decoding.

@ilap is this an issue and can it be detected on an individual, per mnemonic (share) basis? From my understanding Checksum (C) also includes the padded share value.

ilap commented 5 years ago

Hi Jorn,

Thx for the contribution. I think it's a bug in the encodeBigInt(number, length) function, as it does not check the expected length and the result's length. So, I think you can solve it by either of modifying the encodeBigInt() or add an expection to the decodeMnemonic(mnemonic)'s try catch block, e.g.: encodeBigInt(number, length = 0)

  if (length !== 0 && result.length > length) {
    throw new Error(`Error in ecoding BigInt value, expected less than ${length} length value, got ${result.length}`);
  }

or decodeMnemonic(mnemonic)

  if (share.length > valueByteCount) {
    throw new Error(`Encoding error: expected ${valueByteCount} length BigInt got ${share.length}`);
  }
ilap commented 5 years ago

Also, I would rename the length parameter of the encodeBigInt(number, length=0) to paddedLength, maxLength or similar. As the length is misleading.

joernroeder commented 5 years ago

Hi @ilap, yes that makes sense and after adding the check to encodeBigInt tests are now passing.

joernroeder commented 5 years ago

@ilap fixed it.