Share hex length is (I think) always invalid (odd)

[jgeewax]

The issue looks to be with the prefixing, which always appends an "8" (https://github.com/amper5and/secrets.js/blob/master/secrets.js#L246)

From what I can see, the following lines aren't quite right:

for(var i=0; i<numShares; i++){
    x[i] = config.bits.toString(36).toUpperCase() + padLeft(x[i],padding) + bin2hex(y[i]);
}

1. Why are we using base 36? For a larger number than 8, (ie, 200), there's no way that this will generate a valid hexadecimal value... ((200).toString(36) == '5k')

2. Assuming we meant this to be 16 instead of 36 (which makes much more sense), it still can result in an odd-length string, which is 👎 ((8).toString(16) == '8')

   • To illustrate why this is a bad time, here's a traceback:

   > new Buffer((8).toString(16), 'hex');
   TypeError: Invalid hex string
   at TypeError (native)
   at Buffer.write (buffer.js:568:21)
   at fromString (buffer.js:115:26)
   at new Buffer (buffer.js:54:12)
   at repl:1:1
   at REPLServer.defaultEval (repl.js:248:27)
   at bound (domain.js:287:14)
   at REPLServer.runBound [as eval] (domain.js:300:12)
   at REPLServer.<anonymous> (repl.js:412:12)
   at emitOne (events.js:82:20)

I think we should do what you did a bit later in the line with padLeft:

for(var i = 0; i < numShares; i++) {
    // x[i] is (even-length number of bits, any padding requested, and then the secret share)
    var prefix = padLeft(config.bits.toString(16).toUpperCase(), 2),
        padding = padLeft(x[i], padding);
    x[i] = prefix + padding + bin2hex(y[i]);
}

The question then becomes, what corresponding changes do we need for the combining portion of code...?

I think we would need to update the processShare method to make this work (https://github.com/amper5and/secrets.js/blob/master/secrets.js#L301)

(the lines affected are var bits = ... and var id = ...)

function processShare(share){
    var bits = parseInt(share.substring(0, 2), 16);
    if(bits && (typeof bits !== 'number' || bits%1 !== 0 || bits<defaults.minBits || bits>defaults.maxBits)){
        throw new Error('Number of bits must be an integer between ' + defaults.minBits + ' and ' + defaults.maxBits + ', inclusive.')
    }

    var max = Math.pow(2, bits) - 1;
    var idLength = max.toString(config.radix).length;

    var id = parseInt(share.substr(2, idLength), config.radix);
    if(typeof id !== 'number' || id%1 !== 0 || id<1 || id>max){
        throw new Error('Share id must be an integer between 1 and ' + config.max + ', inclusive.');
    }
    share = share.substr(idLength + 1);
    if(!share.length){
        throw new Error('Invalid share: zero-length share.')
    }
    return {
        'bits': bits,
        'id': id,
        'value': share
    };
};

[grempe]

@jgeewax I wonder if you would be interested in taking a look at my fork of this project and seeing if you have similar issues still there. I don't think any changes have been accepted on this project for a couple of years. I did a pretty major revamp on the fork and perhaps it resolved your issues. I would certainly take a look at pull requests if there is an issue you are still seeing.

https://github.com/grempe/secrets.js

Glenn

[amper5and]

So, I know I haven't worked on this project in years, but just glancing through your comments, I wanted to point a few things out. Most of this is obvious from the readme btw:

Why are we using base 36? For a larger number than 8, (ie, 200), there's no way that this will generate a valid hexadecimal value... ((200).toString(36) == '5k')

secrets.js only uses Galois fields between 2^3 and 2^20, so the bits accepted must be between 3 and 20. In the interest of using ONE character for the bit-field in the final share, base-36 was selected, to encode all possible values using the characters [3-9, a-k].

The issue looks to be with the prefixing, which always appends an "8" (https://github.com/amper5and/secrets.js/blob/master/secrets.js#L246)

The 8 is appended because an 8-bit field is used by default. You can change config.bits to something other than 8, and you'll have that value appended