runes: ensure that uniqueid is a valid number.

It always is for runes we create, but in theory you can take our secret key
and make our own runes with your own tools.

(We correctly refuse runes without uniqueids if they're *not* ours
anyway: uniqueid is only used for our own runes).

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

lightningd/runes.c

@@ -83,6 +83,37 @@ static void flush_usage_table(struct runes *runes)
 	notleak(new_reltimer(runes->ld->timers, runes, time_from_sec(60), flush_usage_table, runes));
 }
 
+/* Convert unique_id string to u64.  We only expect this to fail when we're
+ * dealing with runes from elsewhere (i.e. param_rune) */
+static bool unique_id_num(const struct rune *rune, u64 *num)
+{
+	unsigned long long l;
+	char *end;
+
+	if (!rune->unique_id)
+		return false;
+	l = strtoull(rune->unique_id, &end, 0);
+	if (*end)
+		return false;
+
+	/* sqlite3 only does signed 64 bits, so don't exceed 63 bits. */
+	if (l > INT64_MAX)
+		return false;
+
+	*num = l;
+	return true;
+}
+
+u64 rune_unique_id(const struct rune *rune)
+{
+	u64 num;
+
+	/* Any of our runes must have valid unique ids! */
+	if (!unique_id_num(rune, &num))
+		abort();
+	return num;
+}
+
 static const char *rate_limit_check(const tal_t *ctx,
 				    const struct runes *runes,
 				    const struct rune *rune,
@@ -91,6 +122,8 @@ static const char *rate_limit_check(const tal_t *ctx,
 {
 	unsigned long r;
 	char *endp;
+	u64 unique_id = rune_unique_id(rune);
+
 	if (alt->condition != '=')
 		return "rate operator must be =";
 
@@ -100,10 +133,10 @@ static const char *rate_limit_check(const tal_t *ctx,
 
 	/* We cache this: we only add usage counter if whole rune succeeds! */
 	if (!cinfo->usage) {
-		cinfo->usage = usage_table_get(runes->usage_table, atol(rune->unique_id));
+		cinfo->usage = usage_table_get(runes->usage_table, unique_id);
 		if (!cinfo->usage) {
 			cinfo->usage = tal(runes->usage_table, struct usage);
-			cinfo->usage->id = atol(rune->unique_id);
+			cinfo->usage->id = unique_id;
 			cinfo->usage->counter = 0;
 			usage_table_add(runes->usage_table, cinfo->usage);
 		}
@@ -157,12 +190,18 @@ static struct command_result *param_rune(struct command *cmd, const char *name,
 					 const char * buffer, const jsmntok_t *tok,
 					 struct rune_and_string **rune_and_string)
 {
+	u64 uid;
+
 	*rune_and_string = tal(cmd, struct rune_and_string);
 	(*rune_and_string)->runestr = json_strdup(*rune_and_string, buffer, tok);
 	(*rune_and_string)->rune = rune_from_base64(cmd, (*rune_and_string)->runestr);
 	if (!(*rune_and_string)->rune)
 		return command_fail_badparam(cmd, name, buffer, tok,
 					     "should be base64 string");
+	/* We always create runes with integer unique ids: accept no less! */
+	if (!unique_id_num((*rune_and_string)->rune, &uid))
+		return command_fail_badparam(cmd, name, buffer, tok,
+					     "should have valid numeric unique_id");
 
 	return NULL;
 }
@@ -255,7 +294,7 @@ static bool is_rune_blacklisted(const struct runes *runes, const struct rune *ru
 	if (rune->unique_id == NULL) {
 		return false;
 	}
-	uid = atol(rune->unique_id);
+	uid = rune_unique_id(rune);
 	for (size_t i = 0; i < tal_count(runes->blacklist); i++) {
 		if (runes->blacklist[i].start <= uid && runes->blacklist[i].end >= uid) {
 			return true;
@@ -334,7 +373,7 @@ static struct command_result *json_showrunes(struct command *cmd,
 	response = json_stream_success(cmd);
 	json_array_start(response, "runes");
 	if (ras) {
-		long uid = atol(ras->rune->unique_id);
+		u64 uid = rune_unique_id(ras->rune);
 		const char *from_db = wallet_get_rune(tmpctx, cmd->ld->wallet, uid);
 
 		/* We consider it stored iff this is exactly stored */

lightningd/runes.h

@@ -15,4 +15,7 @@ void runes_finish_init(struct runes *runes);
  */
 const char *rune_is_ours(struct lightningd *ld, const struct rune *rune);
 
+/* Get unique id number of rune. */
+u64 rune_unique_id(const struct rune *rune);
+
 #endif /* LIGHTNING_LIGHTNINGD_RUNES_H */

tests/test_runes.py

@@ -522,6 +522,27 @@ def test_invalid_restrictions(node_factory):
             print(l1.rpc.createrune(restrictions=[[f"{cond}method"]]))
 
 
+def test_nonnumeric_uniqueid(node_factory):
+    """We always use numeric uniqueids, don't allow other forms"""
+    l1 = node_factory.get_node()
+
+    # "zero"
+    with pytest.raises(RpcError, match='should have valid numeric unique_id'):
+        l1.rpc.showrunes('pX1h8z05kg0WZqe0ogo_8MHOa7-8gVZNXTckyyLKuLk9emVybw==')
+
+    # "0andstr"
+    with pytest.raises(RpcError, match='should have valid numeric unique_id'):
+        l1.rpc.showrunes('1dYYAIxmjRDHbXUIpztpwDA0q7RrHKbXJd9Yhj50Dfc9MGFuZHN0cmluZw==')
+
+    # "9223372036854775809"
+    with pytest.raises(RpcError, match='should have valid numeric unique_id'):
+        l1.rpc.showrunes('cr7eQmdhPVqH3-M2g67JymP2zVKyZ_n5hhFk3IMSg5o9OTIyMzM3MjAzNjg1NDc3NTgwOQ==')
+
+    # No unique id!
+    with pytest.raises(RpcError, match='should have valid numeric unique_id'):
+        l1.rpc.showrunes('SaQlgbzu6SGAhANReucs7P8GLuVbSlPg30QG78UzNcY=')
+
+
 def test_showrune_id(node_factory):
     l1 = node_factory.get_node()
 

wallet/test/run-db.c

@@ -234,6 +234,9 @@ struct htlc_in *remove_htlc_in_by_dbid(struct htlc_in_map *remaining_htlcs_in UN
 /* Generated stub for rune_is_ours */
 const char *rune_is_ours(struct lightningd *ld UNNEEDED, const struct rune *rune UNNEEDED)
 { fprintf(stderr, "rune_is_ours called!\n"); abort(); }
+/* Generated stub for rune_unique_id */
+u64 rune_unique_id(const struct rune *rune UNNEEDED)
+{ fprintf(stderr, "rune_unique_id called!\n"); abort(); }
 /* Generated stub for to_canonical_invstr */
 const char *to_canonical_invstr(const tal_t *ctx UNNEEDED, const char *invstring UNNEEDED)
 { fprintf(stderr, "to_canonical_invstr called!\n"); abort(); }

wallet/test/run-wallet.c

@@ -687,6 +687,9 @@ void resolve_close_command(struct lightningd *ld UNNEEDED, struct channel *chann
 /* Generated stub for rune_is_ours */
 const char *rune_is_ours(struct lightningd *ld UNNEEDED, const struct rune *rune UNNEEDED)
 { fprintf(stderr, "rune_is_ours called!\n"); abort(); }
+/* Generated stub for rune_unique_id */
+u64 rune_unique_id(const struct rune *rune UNNEEDED)
+{ fprintf(stderr, "rune_unique_id called!\n"); abort(); }
 /* Generated stub for serialize_onionpacket */
 u8 *serialize_onionpacket(
 	const tal_t *ctx UNNEEDED,