Merge pull request #38 from wolfmcnally/master

Improved argument validation and naming canonicalization.

README.md

@@ -63,7 +63,7 @@ This table below also establishes provenance (repository of origin, permalink, a
 
 These are other projects that work with or leverage `bc-shamir`:
 
-- [bc-slip39](https://github.com/BlockchainCommons/bc-slip39) — Blockchain Common's SLIP39 implementation.
+- [bc-slip39](https://github.com/BlockchainCommons/bc-slip39) — Blockchain Common's SLIP-39 implementation.
 - [lethekit](https://github.com/BlockchainCommons/bc-lethekit) — Open source DIY hardware box for offline cryptographic tools by Blockchain Commons
 
 ### Derived from…

src/interpolate.c

@@ -134,10 +134,6 @@ int16_t interpolate(
     uint8_t x,           // x coordinate to interpolate
     uint8_t* result      // space for yl bytes of results
 ) {
-    if(yl > SHAMIR_MAX_SECRET_SIZE) {
-        return SHAMIR_ERROR_SECRET_TOO_LONG;
-    }
-
     // The hazmat gf256 implementation needs the y-coordinate data
     // to be in 32-byte blocks
     uint8_t *y[n];

src/shamir-constants.h

@@ -12,7 +12,7 @@
 #define SHAMIR_MAX_SECRET_SIZE 32
 
 #define SHAMIR_ERROR_SECRET_TOO_LONG       (-101)
-#define SHAMIR_ERROR_TOO_MANY_SHARDS       (-102)
+#define SHAMIR_ERROR_TOO_MANY_SHARES       (-102)
 #define SHAMIR_ERROR_INTERPOLATION_FAILURE (-103)
 #define SHAMIR_ERROR_CHECKSUM_FAILURE      (-104)
 #define SHAMIR_ERROR_SECRET_TOO_SHORT      (-105)

src/shamir.c

@@ -37,39 +37,50 @@ uint8_t * create_digest(
     return result;
 }
 
+static int32_t validate_parameters(
+    uint8_t threshold,
+    uint8_t share_count,
+    uint32_t secret_length
+) {
+    if(share_count > SHAMIR_MAX_SHARE_COUNT) {
+        return SHAMIR_ERROR_TOO_MANY_SHARES;
+    } else if(threshold < 1 || threshold > share_count) {
+        return SHAMIR_ERROR_INVALID_THRESHOLD;
+    } else if(secret_length > SHAMIR_MAX_SECRET_SIZE) {
+        return SHAMIR_ERROR_SECRET_TOO_LONG;
+    } else if(secret_length < SHAMIR_MIN_SECRET_SIZE) {
+        return SHAMIR_ERROR_SECRET_TOO_SHORT;
+    } else if(secret_length & 1) {
+        return SHAMIR_ERROR_SECRET_NOT_EVEN_LEN;
+    }
+    return 0;
+}
 
 //////////////////////////////////////////////////
 // shamir sharing
 int32_t split_secret(
     uint8_t threshold,
-    uint8_t shard_count,
+    uint8_t share_count,
     const uint8_t *secret,
     uint32_t secret_length,
     uint8_t *result,
     void* ctx,
     void (*random_generator)(uint8_t *, size_t, void*)
 ) {
-    if(shard_count > SHAMIR_MAX_SHARD_COUNT) {
-        return SHAMIR_ERROR_TOO_MANY_SHARDS;
-    } else if(threshold < 1 || threshold > shard_count) {
-        return SHAMIR_ERROR_INVALID_THRESHOLD;
-    } else if(secret_length > SHAMIR_MAX_SECRET_SIZE) {
-        return SHAMIR_ERROR_SECRET_TOO_LONG;
-    } else if(secret_length < SHAMIR_MIN_SECRET_SIZE) {
-        return SHAMIR_ERROR_SECRET_TOO_SHORT;
-    } else if(secret_length & 1) {
-        return SHAMIR_ERROR_SECRET_NOT_EVEN_LEN;
+    int32_t err = validate_parameters(threshold, share_count, secret_length);
+    if(err) {
+        return err;
     }
 
     if(threshold == 1) {
-        // just return shard_count copies of the secret
+        // just return share_count copies of the secret
         uint8_t *share = result;
-        for(uint8_t i=0; i< shard_count; ++i, share += secret_length) {
+        for(uint8_t i=0; i< share_count; ++i, share += secret_length) {
             for(uint8_t j=0; j<secret_length; ++j) {
                 share[j] = secret[j];
             }
         }
-        return shard_count;
+        return share_count;
     } else {
         uint8_t digest[secret_length];
         uint8_t x[16];
@@ -96,7 +107,7 @@ int32_t split_secret(
         y[n] = secret;
         n+=1;
 
-        for(uint8_t i=threshold -2; i<shard_count; ++i, share += secret_length) {
+        for(uint8_t i=threshold -2; i<share_count; ++i, share += secret_length) {
             if(interpolate(n, x, secret_length, y, i, share) < 0) {
                 return SHAMIR_ERROR_INTERPOLATION_FAILURE;
             }
@@ -106,18 +117,23 @@ int32_t split_secret(
         memzero(x, sizeof(x));
         memzero(y, sizeof(y));
     }
-    return shard_count;
+    return share_count;
 }
 
 
-// returns the number of bytes written to the secret array, or -1 if there was an error
+// returns the number of bytes written to the secret array, or a negative value if there was an error
 int32_t recover_secret(
     uint8_t threshold,
     const uint8_t *x,
     const uint8_t **shares,
     uint32_t share_length,
     uint8_t *secret
 ) {
+    int32_t err = validate_parameters(threshold, threshold, share_length);
+    if(err) {
+        return err;
+    }
+
     uint8_t digest[share_length];
     uint8_t verify[4];
     uint8_t valid = 1;

src/shamir.h

@@ -11,12 +11,12 @@
 #include <stdint.h>
 #include "shamir-constants.h"
 
-#define SHAMIR_MAX_SHARD_COUNT 16
+#define SHAMIR_MAX_SHARE_COUNT 16
 #define SECRET_INDEX 255
 #define DIGEST_INDEX 254
 
 /**
- * creates a digest used to help valididate secret reconstruction (see slip-39 docs)
+ * creates a digest used to help valididate secret reconstruction (see SLIP-39 docs)
  *
  * returns: a pointer to the resulting 4-byte digest
  * inputs: random_data: array of data to create a digest for
@@ -34,17 +34,17 @@ uint8_t* create_digest(
 );
 
 //////////////////////////////////////////////////
-// slip39 shamir sharing
+// Shamir Secret Sharing (based on SLIP-39)
 
 /**
- * used slip39's version of shamir sharing to split a secret up into
- * shard_count shares such that threshold of them must be presented
+ * uses SLIP-39's strategy for shamir sharing to split a secret up into
+ * share_count shares such that threshold of them must be presented
  * to recover the secret.
  *
- * returns: the number of shards created, or a negative value if there was an error
+ * returns: the number of shares created, or a negative value if there was an error
  *
- * inputs: threshold: number of shards required to recover secret. Must be 1 <= threshold <= shard_count.
- *         shard_count: number of shards to generate
+ * inputs: threshold: number of shares required to recover secret. Must be 1 <= threshold <= share_count.
+ *         share_count: number of shares to generate
  *         secret: array of bytes representing the secret
  *         secret_length: length of the secret array. must be >= 16, <= 32 and even.
  *         result: place to store the resulting shares. Must be able to hold
@@ -53,7 +53,7 @@ uint8_t* create_digest(
  */
 int32_t split_secret(
     uint8_t threshold,
-    uint8_t shard_count,
+    uint8_t share_count,
     const uint8_t *secret,
     uint32_t secret_length,
     uint8_t *result,
@@ -62,21 +62,21 @@ int32_t split_secret(
 );
 
 /**
- * recover a secret from shards
+ * recover a secret from shares
  *
  * returns: the number of bytes written to the secret array, or a negative value if there was an error
  *
- * inputs: threshold: number of shards required
- *         x: array of x values  (length threshold)
- *         shards: array (length threshold) of pointers to y value arrays
- *         shard_length: number of bytes in each y value array
- *         secret: array for writing results (must be at least shard_length long)
+ * inputs: threshold: number of shares required and provided to this function
+ *         x: array of x values (length: threshold)
+ *         shares: array (length: threshold) of pointers to y value arrays
+ *         share_length: number of bytes in each y value array
+ *         secret: array for writing results (must be at least share_length long)
  */
 int32_t recover_secret(
     uint8_t threshold,
     const uint8_t *x,
-    const uint8_t **shards,
-    uint32_t shard_length,
+    const uint8_t **shares,
+    uint32_t share_length,
     uint8_t *secret
 );
 

test/test.c

@@ -12,15 +12,15 @@
 #include "../src/bc-shamir.h"
 #include "test-utils.h"
 
-static size_t _test_split_secret(const char* secret, uint8_t threshold, uint8_t shard_count, char** output_shares) {
+static size_t _test_split_secret(const char* secret, uint8_t threshold, uint8_t share_count, char** output_shares) {
   uint8_t* secret_data;
   size_t secret_len = hex_to_data(secret, &secret_data);
-  size_t result_len = shard_count * secret_len;
+  size_t result_len = share_count * secret_len;
   uint8_t result_data[result_len];
-  int32_t result = split_secret(threshold, shard_count, secret_data, secret_len, result_data, NULL, fake_random);
-  assert(result == shard_count);
+  int32_t result = split_secret(threshold, share_count, secret_data, secret_len, result_data, NULL, fake_random);
+  assert(result == share_count);
 
-  for(int i = 0; i < shard_count; i++) {
+  for(int i = 0; i < share_count; i++) {
     size_t offset = i * secret_len;
     output_shares[i] = data_to_hex(result_data + offset, secret_len);
   }
@@ -49,13 +49,13 @@ static char* _test_recover_secret(uint8_t threshold, const char** recovery_share
   return data_to_hex(secret_data, share_len);
 }
 
-static void _test_shamir(const char* secret, uint8_t threshold, uint8_t shard_count, const uint8_t* recovery_share_indexes) {
+static void _test_shamir(const char* secret, uint8_t threshold, uint8_t share_count, const uint8_t* recovery_share_indexes) {
   // printf("secret: %s\n", secret);
 
-  char* output_shares[shard_count];
-  size_t secret_len = _test_split_secret(secret, threshold, shard_count, output_shares);
+  char* output_shares[share_count];
+  size_t secret_len = _test_split_secret(secret, threshold, share_count, output_shares);
 
-  // for(int i = 0; i < shard_count; i++) {
+  // for(int i = 0; i < share_count; i++) {
   //   printf("%d: %s\n", i, output_shares[i]);
   // }
 
@@ -67,7 +67,7 @@ static void _test_shamir(const char* secret, uint8_t threshold, uint8_t shard_co
   char* out_secret = _test_recover_secret(threshold, (const char **)recovery_shares, recovery_share_indexes);
   // printf("out_secret: %s\n", out_secret);
 
-  for(int i = 0; i < shard_count; i++) {
+  for(int i = 0; i < share_count; i++) {
     free(output_shares[i]);
   }