multiwithdraw: use unique ids on requests.
What the developer wrote
Authored by Rusty Russell
multiwithdraw: use unique ids on requests.
Shades of `efacada7ddf` which did the same thing in multifundchannel:
(ab)used the id, which being a string, gave and id of 34 (").
Also clean up the leftover assert in multifundchannel.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This patch fixes a bug in the multiwithdraw plugin where it reused the JSON-RPC command ID as its own internal request identifier. Because command IDs can be arbitrary strings (for example, a double-quote character becomes the number 34), using them directly as numeric IDs could cause collisions or confusion when tracking multiple in-flight withdrawal requests. The fix introduces a private global counter so each multiwithdraw request gets a unique numeric ID, matching an earlier fix made to the multifundchannel plugin. The patch also removes a leftover debug assertion in multifundchannel.
Apply the patch. Review other plugins for the same anti-pattern of dereferencing cmd->id as a numeric identifier. Consider adding regression tests that exercise multiwithdraw with string, numeric, and special-character JSON-RPC ids to ensure unique internal tracking.
Security signals we found
Use of attacker-influenced pointer/value as internal identifier (CWE-20 / CWE-99 style logic error)
Potential ID collision / request confusion in multiwithdraw plugin
Removal of leftover assertion in related plugin
Pattern repeat of a prior fix (efacada7ddf) indicating a known bug class
Evidence from the diff
In plugins/spender/multiwithdraw.c, json_multiwithdraw previously assigned mw->id = *cmd->id after asserting cmd->id was non-NULL. The cmd->id field is a json_tokener pointer whose integer value depends on the literal bytes of the caller-supplied JSON-RPC id. If a client sends a string id such as ‘”’, the token pointer dereferences to the ASCII value 34, producing a numeric id of 34. This makes internal id allocation predictable and potentially colliding across concurrent or sequential calls. The patch replaces this with a static u64 mw_id counter (mw->id = mw_id++), identical in spirit to commit efacada7ddf which fixed the same pattern in multifundchannel. The multifundchannel.c hunk removes an obsolete assert(cmd->id) that is no longer needed now that mfc->id is also generated from a private counter.
Changed components
plugins/spender/multiwithdraw.cplugins/spender/multifundchannel.cInspect captured patch +4 / −5
diff --git a/plugins/spender/multifundchannel.c b/plugins/spender/multifundchannel.c
index c7223867..7c74bd13 100644
--- a/plugins/spender/multifundchannel.c
+++ b/plugins/spender/multifundchannel.c
@@ -1998,9 +1998,6 @@ json_multifundchannel(struct command *cmd,
NULL))
return command_param_failed();
- /* Should exist; it would only nonexist if it were a notification. */
- assert(cmd->id);
-
mfc->id = ++mfc_id;
mfc->cmd = cmd;
diff --git a/plugins/spender/multiwithdraw.c b/plugins/spender/multiwithdraw.c
index 7950d1d1..c647bf7c 100644
--- a/plugins/spender/multiwithdraw.c
+++ b/plugins/spender/multiwithdraw.c
@@ -17,6 +17,9 @@
Command Access
-----------------------------------------------------------------------------*/
+/* Global counter to create unique md->id values */
+static u64 mw_id;
+
static struct command_result *
json_multiwithdraw(struct command *cmd,
const char *buf,
@@ -173,8 +176,7 @@ json_multiwithdraw(struct command *cmd,
return command_param_failed();
mw->cmd = cmd;
- assert(cmd->id);
- mw->id = *cmd->id;
+ mw->id = mw_id++;
mw->psbt = NULL;
if (!mw->feerate)
Why this scored 42/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.