Skip to content

bitcoin/short_channel_id: validate strict decimal format in short_channel_id_from_str - #9701

Open
bhuvan-somisetty wants to merge 1 commit into
ElementsProject:masterfrom
bhuvan-somisetty:fix-scid-parsing-validation
Open

bhuvan-somisetty wants to merge 1 commit into
ElementsProject:masterfrom
bhuvan-somisetty:fix-scid-parsing-validation

Conversation

@bhuvan-somisetty

Copy link
Copy Markdown
Contributor

Description

Fixes #9700

short_channel_id_from_str previously used sscanf("%ux%ux%hu", &blocknum, &txnum, &outnum) == 3. This had several validation issues:

  • sscanf does not verify that the full string buffer was consumed, silently accepting strings with trailing garbage (e.g. "1x2x3garbage", "1x2x3/0", "1x2x3x4", "1x2x3 4").
  • %hu accepts negative numbers (e.g. "-1"), converting them to unsigned 16-bit integers (65535), which passes mk_short_channel_id() bounds checks.
  • sscanf accepts leading and intermediate whitespace as well as + signs.
  • These issues propagate into short_channel_id_dir_from_str and JSON-RPC parameter parsing.

This change:

  1. Replaces sscanf with strict parsing using memchr() for 'x' delimiters and str_to_u64() for strict decimal parsing of each segment.
  2. Ensures no trailing characters, leading/trailing whitespace, signs, or negative numbers are accepted.
  3. Adds dedicated unit tests in bitcoin/test/run-short_channel_id.c covering canonical formats, edge values, negative numbers, signs, trailing characters, and invalid directions.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.
  • Important All PRs must consider how to reverse any persistent changes for tools/lightning-downgrade (N/A, in-memory string parsing only).

Comment thread bitcoin/test/run-short_channel_id.c Outdated
assert(!short_channel_id_from_str("x2x3", 4, &scid));
assert(!short_channel_id_from_str("1xx3", 4, &scid));

/* Out of range components */

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add test cases for values above 2^32 and 2^16, and for a value above 2^64 (str_to_u64 must reject it)

Comment thread bitcoin/short_channel_id.c Outdated
if (!x2)
return false;

if (memchr(x2 + 1, 'x', strlen - (x2 + 1 - str)))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the third memchr is not needed, str_to_u64() already fails on any character that is not a digit, including x. So 1x2x3x4 is rejected at line 64 anyway

…nnel_id_from_str

short_channel_id_from_str previously used sscanf with "%ux%ux%hu".
This accepted strings with trailing unparsed characters (e.g.
"1x2x3garbage", "1x2x3/0"), negative numbers (via %hu casting to 65535,
e.g. "1x2x-1"), signs ("+1x2x3"), and whitespace (" 1x2x3").

Switch to strict delimiter checking using memchr and strict decimal
parsing with str_to_u64, matching BOLT 7 and preventing invalid short
channel ID strings in RPC and gossip parsing.

Add comprehensive unit tests in bitcoin/test/run-short_channel_id.c.

Changelog-Fixed: bitcoin: reject invalid short_channel_id strings with trailing characters, signs, negative numbers, or whitespace
Fixes: ElementsProject#9700
Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
@bhuvan-somisetty
bhuvan-somisetty force-pushed the fix-scid-parsing-validation branch from cdfa10c to c40008a Compare November 11, 2026 20:03
@bhuvan-somisetty

Copy link
Copy Markdown
Contributor Author

Thanks @Andezion! Removed the extra memchr check and added the >2^16, >2^32, and >2^64 test cases.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bitcoin/short_channel_id: short_channel_id_from_str accepts trailing garbage, negative numbers, and whitespace

2 participants