hsmtool: tighten checks for validity on getcodexsecret

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

common/codex32.c

@@ -396,20 +396,38 @@ struct codex32 *codex32_decode(const tal_t *ctx,
 }
 
 /* Returns Codex32 encoded secret of the seed provided. */
-char *codex32_secret_encode(const tal_t *ctx,
+const char *codex32_secret_encode(const tal_t *ctx,
 				  const char *id,
 				  const u32 threshold,
 				  const u8 *seed,
-			    size_t seedlen)
+				  size_t seedlen,
+				  char **bip93)
 {
 	const struct checksum_engine *csum_engine;
 	const char *hrp = "ms";
-	assert(threshold <= 9 && threshold >= 0 &&
-		threshold != 1 && strlen(id) == 4);
+
+	if (threshold > 9 || threshold < 0 || threshold == 1)
+		return tal_fmt(ctx, "Invalid threshold %u", threshold);
+
+	if (strlen(id) != 4)
+		return tal_fmt(ctx, "Invalid id: must be 4 characters");
+
+	for (size_t i = 0; id[i]; i++) {
+		s8 rev;
+
+		if (id[i] & 0x80)
+			return tal_fmt(ctx, "Invalid id: must be ASCII");
+
+		rev = bech32_charset_rev[(int)id[i]];
+		if (rev == -1)
+			return tal_fmt(ctx, "Invalid id: must be valid bech32 string");
+		if (bech32_charset[rev] != id[i])
+			return tal_fmt(ctx, "Invalid id: must be lower-case");
+	}
 
 	/* Every codex32 has hrp `ms` and since we are generating a
 	 * secret it's share index would be `s` and threshold given by user. */
-	char *ret = tal_fmt(ctx, "%s1%d%ss", hrp, threshold, id);
+	*bip93 = tal_fmt(ctx, "%s1%d%ss", hrp, threshold, id);
 
 	uint8_t next_u5 = 0, rem = 0;
 
@@ -417,24 +435,24 @@ char *codex32_secret_encode(const tal_t *ctx,
             /* Each byte provides at least one u5. Push that. */
             uint8_t u5 = (next_u5 << (5 - rem)) | seed[i] >> (3 + rem);
 
-	    tal_append_fmt(&ret, "%c", bech32_charset[u5]);
+	    tal_append_fmt(bip93, "%c", bech32_charset[u5]);
             next_u5 = seed[i] & ((1 << (3 + rem)) - 1);
 
             /* If there were 2 or more bits from the last iteration, then
              * this iteration will push *two* u5s. */
             if(rem >= 2) {
-	        tal_append_fmt(&ret, "%c", bech32_charset[next_u5 >> (rem - 2)]);
+	        tal_append_fmt(bip93, "%c", bech32_charset[next_u5 >> (rem - 2)]);
                 next_u5 &= (1 << (rem - 2)) - 1;
             }
             rem = (rem + 8) % 5;
         }
         if(rem > 0) {
-	    tal_append_fmt(&ret, "%c", bech32_charset[next_u5 << (5 - rem)]);
+	    tal_append_fmt(bip93, "%c", bech32_charset[next_u5 << (5 - rem)]);
         }
 
 	csum_engine = &initial_engine_csum[seedlen >= 51];
 	char csum[csum_engine->len];
-	calculate_checksum(hrp, csum, ret + 3, csum_engine);
-	tal_append_fmt(&ret, "%.*s", (int)csum_engine->len, csum);
-	return ret;
+	calculate_checksum(hrp, csum, *bip93 + 3, csum_engine);
+	tal_append_fmt(bip93, "%.*s", (int)csum_engine->len, csum);
+	return NULL;
 }

common/codex32.h

@@ -42,18 +42,19 @@ struct codex32 *codex32_decode(const tal_t *ctx,
 
 /** Encode a seed into codex32 secret format.
  *
- *  Out: char *:      String containing the codex32 (BIP93) format secret.
- *       fail:        Asserting error if invalid threshold is used.
  *  In: input:        id: Valid 4 char string identifying the secret
  * 		      threshold: Threshold according to the bip93
  * 		      seed: The secret in u8*
  * 		      seedlen: Length of the seed provided.
  *      Returns a string which contains the seed provided in bip93 format.
+ *
+ * Returns an error string, or returns NULL and sets @bip93.
  */
-char *codex32_secret_encode(const tal_t *ctx,
+const char *codex32_secret_encode(const tal_t *ctx,
 				  const char *id,
 				  const u32 threshold,
 				  const u8 *seed,
-			    size_t seedlen);
+				  size_t seedlen,
+				  char **bip93);
 
 #endif /* LIGHTNING_COMMON_CODEX32_H */

common/test/run-codex32.c

@@ -122,7 +122,7 @@ void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNE
 int main(int argc, char *argv[])
 {
 	common_setup(argv[0]);
-	char *fail;
+	char *fail, *c;
 	struct codex32 *parts;
 
 	/* Test vector for codex32_secret_encode*/
@@ -133,7 +133,7 @@ int main(int argc, char *argv[])
             0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11, 0x00,
 	};
 
-	char *c = codex32_secret_encode(tmpctx, "leet", 0, seed_b, ARRAY_SIZE(seed_b));
+	assert(codex32_secret_encode(tmpctx, "leet", 0, seed_b, ARRAY_SIZE(seed_b), &c) == NULL);
 	assert(streq(c,
 		     "ms10leetsllhdmn9m42vcsamx24zrxgs3qrl7ahwvhw4fnzrhve25gvezzyqqtum9pgv99ycma"));
 

tests/test_misc.py

@@ -1355,6 +1355,36 @@ def test_recover(node_factory, bitcoind):
     os.unlink(os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "hsm_secret"))
     l1.daemon.start()
 
+    cmd_line = ["tools/hsmtool", "getcodexsecret", os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "hsm_secret")]
+    out = subprocess.check_output(cmd_line + ["leet", "0"]).decode('utf-8')
+    assert out == "ms10leetsllhdmn9m42vcsamx24zrxgs3qrl7ahwvhw4fnzrhve25gvezzyqqtum9pgv99ycma\n"
+
+    # Check bad ids, threshold.
+    out = subprocess.run(cmd_line + ["lee", "0"], stderr=subprocess.PIPE, timeout=TIMEOUT)
+    assert 'Invalid id: must be 4 characters' in out.stderr.decode('utf-8')
+    assert out.returncode == 2
+
+    out = subprocess.run(cmd_line + ["Leet", "0"], stderr=subprocess.PIPE, timeout=TIMEOUT)
+    assert 'Invalid id: must be lower-case' in out.stderr.decode('utf-8')
+    assert out.returncode == 2
+
+    out = subprocess.run(cmd_line + ["💔", "0"], stderr=subprocess.PIPE, timeout=TIMEOUT)
+    assert 'Invalid id: must be ASCII' in out.stderr.decode('utf-8')
+    assert out.returncode == 2
+
+    for bad_bech32 in ['b', 'o', 'i', '1']:
+        out = subprocess.run(cmd_line + [bad_bech32 + "eet", "0"], stderr=subprocess.PIPE, timeout=TIMEOUT)
+        assert 'Invalid id: must be valid bech32 string' in out.stderr.decode('utf-8')
+        assert out.returncode == 2
+
+    out = subprocess.run(cmd_line + ["leet", "1"], stderr=subprocess.PIPE, timeout=TIMEOUT)
+    assert 'Invalid threshold 1' in out.stderr.decode('utf-8')
+    assert out.returncode == 2
+
+    out = subprocess.run(cmd_line + ["leet", "99"], stderr=subprocess.PIPE, timeout=TIMEOUT)
+    assert 'Invalid threshold 99' in out.stderr.decode('utf-8')
+    assert out.returncode == 2
+
     basedir = l1.daemon.opts.get("lightning-dir")
     with open(os.path.join(basedir, TEST_NETWORK, 'hsm_secret'), 'rb') as f:
         buff = f.read()
@@ -1369,11 +1399,6 @@ def test_recover(node_factory, bitcoind):
     l1.daemon.opts['recover'] = "ms10leetsllhdmn9m42vcsamx24zrxgs3qrl7ahwvhw4fnzrhve25gvezzyqqtum9pgv99ycma"
     l1.daemon.start(wait_for_initialized=False, stderr_redir=True)
 
-    cmd_line = ["tools/hsmtool", "getcodexsecret", os.path.join(l1.daemon.lightning_dir, TEST_NETWORK, "hsm_secret"), "leet", "0"]
-    lines = subprocess.check_output(cmd_line).decode('utf-8').splitlines()
-    expected_output = "Codex32 Secret of your hsm_secret is: ms10leetsllhdmn9m42vcsamx24zrxgs3qrl7ahwvhw4fnzrhve25gvezzyqqtum9pgv99ycma"
-    assert [expected_output] == lines
-
     # Will exit with failure code.
     assert l1.daemon.wait() == 1
     assert l1.daemon.is_in_stderr(r"hsm_secret already exists!")

tools/hsmtool.c

@@ -251,9 +251,15 @@ static int make_codexsecret(const char *hsm_secret_path,
 			    const u32 threshold)
 {
 	struct secret hsm_secret;
+	char *bip93;
+	const char *err;
 	get_hsm_secret(&hsm_secret, hsm_secret_path);
-	printf("Codex32 Secret of your hsm_secret is: %s\n",
-		codex32_secret_encode(tmpctx, id, threshold, hsm_secret.data, 32));
+
+	err = codex32_secret_encode(tmpctx, id, threshold, hsm_secret.data, 32, &bip93);
+	if (err)
+		errx(ERROR_USAGE, "%s", err);
+
+	printf("%s\n", bip93);
 	return 0;
 }