From 018a3f1d58e38e464e32cbee26826a0bd63aab59 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 21 Jan 2019 11:27:43 +1030 Subject: [PATCH] short_channel_id: make mk_short_channel_id return a failure. We had a bug 0ba547ee101efe7c5b14a7fbcf41b5088efc2504 caused by short_channel_id overflow. If we'd caught this, we'd have terminated the peer instead of crashing, so add appropriate checks. Signed-off-by: Rusty Russell --- bitcoin/short_channel_id.c | 13 +++++++++---- bitcoin/short_channel_id.h | 5 +++-- gossipd/gossipd.c | 29 ++++++++++++++++++----------- lightningd/peer_control.c | 12 +++++++++--- wallet/wallet.c | 7 +++++-- 5 files changed, 44 insertions(+), 22 deletions(-) diff --git a/bitcoin/short_channel_id.c b/bitcoin/short_channel_id.c index be56dbdde..90432afa1 100644 --- a/bitcoin/short_channel_id.c +++ b/bitcoin/short_channel_id.c @@ -22,12 +22,17 @@ */ -void mk_short_channel_id(struct short_channel_id *scid, - u32 blocknum, u32 txnum, u16 outnum) +bool mk_short_channel_id(struct short_channel_id *scid, + u64 blocknum, u64 txnum, u64 outnum) { + if ((blocknum & 0xFFFFFF) != blocknum + || (txnum & 0xFFFFFF) != txnum + || (outnum & 0xFFFF) != outnum) + return false; scid->u64 = (((u64)blocknum & 0xFFFFFF) << 40 | ((u64)txnum & 0xFFFFFF) << 16 | (outnum & 0xFFFF)); + return true; } bool short_channel_id_from_str(const char *str, size_t strlen, @@ -50,8 +55,8 @@ bool short_channel_id_from_str(const char *str, size_t strlen, #else matches = sscanf(buf, "%ux%ux%hu", &blocknum, &txnum, &outnum); #endif - mk_short_channel_id(dst, blocknum, txnum, outnum); - return matches == 3; + return matches == 3 + && mk_short_channel_id(dst, blocknum, txnum, outnum); } char *short_channel_id_to_str(const tal_t *ctx, const struct short_channel_id *scid) diff --git a/bitcoin/short_channel_id.h b/bitcoin/short_channel_id.h index a4ec16e12..bcf2ef38d 100644 --- a/bitcoin/short_channel_id.h +++ b/bitcoin/short_channel_id.h @@ -47,8 +47,9 @@ static inline u16 short_channel_id_outnum(const struct short_channel_id *scid) return scid->u64 & 0xFFFF; } -void mk_short_channel_id(struct short_channel_id *scid, - u32 blocknum, u32 txnum, u16 outnum); +/* Returns false if blocknum, txnum or outnum require too many bits */ +bool WARN_UNUSED_RESULT mk_short_channel_id(struct short_channel_id *scid, + u64 blocknum, u64 txnum, u64 outnum); bool WARN_UNUSED_RESULT short_channel_id_from_str(const char *str, size_t strlen, struct short_channel_id *dst); diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c index a4c9ace9b..f9b906c46 100644 --- a/gossipd/gossipd.c +++ b/gossipd/gossipd.c @@ -664,13 +664,14 @@ static void reply_channel_range(struct peer *peer, * tail_blocks is the empty blocks at the end, in case they asked for all * blocks to 4 billion. */ -static void queue_channel_ranges(struct peer *peer, +static bool queue_channel_ranges(struct peer *peer, u32 first_blocknum, u32 number_of_blocks, u32 tail_blocks) { struct routing_state *rstate = peer->daemon->rstate; u8 *encoded = encode_short_channel_ids_start(tmpctx); struct short_channel_id scid; + bool scid_ok; /* BOLT #7: * @@ -688,10 +689,12 @@ static void queue_channel_ranges(struct peer *peer, /* Avoid underflow: we don't use block 0 anyway */ if (first_blocknum == 0) - mk_short_channel_id(&scid, 1, 0, 0); + scid_ok = mk_short_channel_id(&scid, 1, 0, 0); else - mk_short_channel_id(&scid, first_blocknum, 0, 0); + scid_ok = mk_short_channel_id(&scid, first_blocknum, 0, 0); scid.u64--; + if (!scid_ok) + return false; /* We keep a `uintmap` of `short_channel_id` to `struct chan *`. * Unlike a htable, it's efficient to iterate through, but it only @@ -712,7 +715,7 @@ static void queue_channel_ranges(struct peer *peer, reply_channel_range(peer, first_blocknum, number_of_blocks + tail_blocks, encoded); - return; + return true; } /* It wouldn't all fit: divide in half */ @@ -721,7 +724,7 @@ static void queue_channel_ranges(struct peer *peer, /* We always assume we can send 1 blocks worth */ status_broken("Could not fit scids for single block %u", first_blocknum); - return; + return false; } status_debug("queue_channel_ranges full: splitting %u+%u and %u+%u(+%u)", first_blocknum, @@ -729,10 +732,10 @@ static void queue_channel_ranges(struct peer *peer, first_blocknum + number_of_blocks / 2, number_of_blocks - number_of_blocks / 2, tail_blocks); - queue_channel_ranges(peer, first_blocknum, number_of_blocks / 2, 0); - queue_channel_ranges(peer, first_blocknum + number_of_blocks / 2, - number_of_blocks - number_of_blocks / 2, - tail_blocks); + return queue_channel_ranges(peer, first_blocknum, number_of_blocks / 2, 0) + && queue_channel_ranges(peer, first_blocknum + number_of_blocks / 2, + number_of_blocks - number_of_blocks / 2, + tail_blocks); } /*~ The peer can ask for all channels is a series of blocks. We reply with one @@ -778,8 +781,12 @@ static u8 *handle_query_channel_range(struct peer *peer, const u8 *msg) } else tail_blocks = 0; - queue_channel_ranges(peer, first_blocknum, number_of_blocks, - tail_blocks); + if (!queue_channel_ranges(peer, first_blocknum, number_of_blocks, + tail_blocks)) + return towire_errorfmt(peer, NULL, + "Invalid query_channel_range %u+%u", + first_blocknum, number_of_blocks + tail_blocks); + return NULL; } diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index 4e8c97631..18970e06b 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -537,9 +537,15 @@ static enum watch_result funding_lockin_cb(struct lightningd *ld, loc = wallet_transaction_locate(tmpctx, ld->wallet, txid); channel->scid = tal(channel, struct short_channel_id); - mk_short_channel_id(channel->scid, - loc->blkheight, loc->index, - channel->funding_outnum); + if (!mk_short_channel_id(channel->scid, + loc->blkheight, loc->index, + channel->funding_outnum)) { + channel_fail_permanent(channel, "Invalid funding scid %u:%u:%u", + loc->blkheight, loc->index, + channel->funding_outnum); + return DELETE_WATCH; + } + /* We've added scid, update */ wallet_channel_save(ld->wallet, channel); } diff --git a/wallet/wallet.c b/wallet/wallet.c index 068fbb253..a9772e102 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -2166,8 +2166,11 @@ wallet_outpoint_spend(struct wallet *w, const tal_t *ctx, const u32 blockheight, assert(res == SQLITE_ROW); scid = tal(ctx, struct short_channel_id); - mk_short_channel_id(scid, sqlite3_column_int(stmt, 0), - sqlite3_column_int(stmt, 1), outnum); + if (!mk_short_channel_id(scid, sqlite3_column_int(stmt, 0), + sqlite3_column_int(stmt, 1), outnum)) + fatal("wallet_outpoint_spend: invalid scid %u:%u:%u", + sqlite3_column_int(stmt, 0), + sqlite3_column_int(stmt, 1), outnum); db_stmt_done(stmt); return scid; }