askrene: remove non child-friendly fields from struct route_query.
What changed, and why it matters
This is a code cleanup change in Core Lightning's routing plugin (askrene). It removes direct access to the command and plugin objects from an internal data structure used during route calculations, passing only the specific pieces of information the child process needs instead. This is a defensive design improvement, not a fix for an active security bug.
No immediate action required. Treat as routine hardening/refactoring. Reviewers may want to confirm that the new non-const rq->reserved pointer is properly restored/undone on all error paths, as noted in the commit message.
Security signals we found
Reduction of privileged object exposure to forked child process
Replacement of plugin_err with abort in child error path
Defensive refactoring to limit struct route_query capabilities
Evidence from the diff
The commit refactors struct route_query in the askrene plugin so it no longer stores struct command cmd and struct plugin plugin. Instead, it keeps only the command id string (cmd_id) and a direct pointer to the reservation table (reserved). Logging helpers are changed to take struct command explicitly, and maxparts is passed as a function parameter rather than stored in route_query. The child process’s abort-on-error path is also simplified. The change reduces the attack surface of the forked router child by limiting what internal state it can access, but it does not patch any identified vulnerability.
Changed components
plugins/askrene/askrene.cplugins/askrene/askrene.hplugins/askrene/child/entry.cplugins/askrene/child/entry.hplugins/askrene/child/mcf.cplugins/askrene/child/mcf.hplugins/askrene/child/refine.cInspect captured patch +66 / −77
diff --git a/plugins/askrene/askrene.c b/plugins/askrene/askrene.c
index 0ffc12b2..b1384299 100644
--- a/plugins/askrene/askrene.c
+++ b/plugins/askrene/askrene.c
@@ -302,11 +302,12 @@ struct amount_msat get_additional_per_htlc_cost(const struct route_query *rq,
return AMOUNT_MSAT(0);
}
-const char *rq_log(const tal_t *ctx,
- const struct route_query *rq,
- enum log_level level,
- const char *fmt,
- ...)
+PRINTF_FMT(4, 5)
+static const char *cmd_log(const tal_t *ctx,
+ struct command *cmd,
+ enum log_level level,
+ const char *fmt,
+ ...)
{
va_list args;
const char *msg;
@@ -315,14 +316,14 @@ const char *rq_log(const tal_t *ctx,
msg = tal_vfmt(ctx, fmt, args);
va_end(args);
- plugin_notify_message(rq->cmd, level, "%s", msg);
+ plugin_notify_message(cmd, level, "%s", msg);
/* Notifications already get logged at debug. Otherwise reduce
* severity. */
if (level != LOG_DBG)
- plugin_log(rq->plugin,
+ plugin_log(cmd->plugin,
level == LOG_BROKEN ? level : level - 1,
- "%s: %s", rq->cmd->id, msg);
+ "%s: %s", cmd->id, msg);
return msg;
}
@@ -364,7 +365,9 @@ struct getroutes_info {
u32 maxparts;
};
-static void apply_layers(struct askrene *askrene, struct route_query *rq,
+static void apply_layers(struct askrene *askrene,
+ struct command *cmd,
+ struct route_query *rq,
const struct node_id *source,
struct amount_msat amount,
struct gossmap_localmods *localmods,
@@ -376,20 +379,19 @@ static void apply_layers(struct askrene *askrene, struct route_query *rq,
const struct layer *l = find_layer(askrene, layers[i]);
if (!l) {
if (streq(layers[i], "auto.localchans")) {
- plugin_log(rq->plugin, LOG_DBG, "Adding auto.localchans");
+ cmd_log(tmpctx, cmd, LOG_DBG, "Adding auto.localchans");
l = local_layer;
} else if (streq(layers[i], "auto.no_mpp_support")) {
- plugin_log(rq->plugin, LOG_DBG, "Adding auto.no_mpp_support, sorry");
+ cmd_log(tmpctx, cmd, LOG_DBG, "Adding auto.no_mpp_support, sorry");
l = remove_small_channel_layer(layers, askrene, amount, localmods);
} else if (streq(layers[i], "auto.include_fees")) {
- plugin_log(rq->plugin, LOG_DBG,
- "Adding auto.include_fees");
+ cmd_log(tmpctx, cmd, LOG_DBG, "Adding auto.include_fees");
/* This layer takes effect when converting flows
* into routes. */
continue;
} else {
assert(streq(layers[i], "auto.sourcefree"));
- plugin_log(rq->plugin, LOG_DBG, "Adding auto.sourcefree");
+ cmd_log(tmpctx, cmd, LOG_DBG, "Adding auto.sourcefree");
l = source_free_layer(layers, askrene, source, localmods);
}
}
@@ -440,7 +442,8 @@ void get_constraints(const struct route_query *rq,
reserve_sub(rq->reserved, &scidd, rq->layers, max);
}
-static void process_child_logs(struct route_query *rq, int log_fd)
+static void process_child_logs(struct command *cmd,
+ int log_fd)
{
u8 *msg;
while ((msg = wire_sync_read(tmpctx, log_fd)) != NULL) {
@@ -448,7 +451,7 @@ static void process_child_logs(struct route_query *rq, int log_fd)
char *entry;
struct node_id *peer;
if (fromwire_status_log(tmpctx, msg, &level, &peer, &entry))
- rq_log(tmpctx, rq, level, "%s", entry);
+ cmd_log(tmpctx, cmd, level, "%s", entry);
}
}
@@ -463,6 +466,7 @@ static struct command_result *do_getroutes(struct command *cmd,
const char *err, *json;
struct timemono time_start, deadline;
int child_fd, log_fd, child_pid, child_status;
+ s8 *biases;
/* update the gossmap */
if (gossmap_refresh(askrene->gossmap)) {
@@ -473,15 +477,13 @@ static struct command_result *do_getroutes(struct command *cmd,
}
/* build this request structure */
- rq->cmd = cmd;
- rq->plugin = cmd->plugin;
+ rq->cmd_id = cmd->id;
rq->gossmap = askrene->gossmap;
rq->reserved = askrene->reserved;
rq->layers = tal_arr(rq, const struct layer *, 0);
rq->capacities = tal_dup_talarr(rq, fp16_t, askrene->capacities);
/* FIXME: we still need to do something useful with these */
rq->additional_costs = info->additional_costs;
- rq->maxparts = info->maxparts;
/* We also eliminate any local channels we *know* are dying.
* Most channels get 12 blocks grace in case it's a splice,
@@ -508,7 +510,7 @@ static struct command_result *do_getroutes(struct command *cmd,
}
/* apply selected layers to the localmods */
- apply_layers(askrene, rq, &info->source, info->amount, localmods,
+ apply_layers(askrene, cmd, rq, &info->source, info->amount, localmods,
info->layers, info->local_layer);
/* Clear scids with reservations, too, so we don't have to look up
@@ -528,18 +530,19 @@ static struct command_result *do_getroutes(struct command *cmd,
/* localmods can add channels, so we need to allocate biases array
* *afterwards* */
- rq->biases =
+ rq->biases = biases =
tal_arrz(rq, s8, gossmap_max_chan_idx(askrene->gossmap) * 2);
/* Note any channel biases */
for (size_t i = 0; i < tal_count(rq->layers); i++)
- layer_apply_biases(rq->layers[i], askrene->gossmap, rq->biases);
+ layer_apply_biases(rq->layers[i], askrene->gossmap, biases);
/* checkout the source */
const struct gossmap_node *srcnode =
gossmap_find_node(askrene->gossmap, &info->source);
if (!srcnode) {
- err = rq_log(tmpctx, rq, LOG_INFORM, "Unknown source node %s",
+ err = cmd_log(tmpctx, cmd, LOG_INFORM,
+ "Unknown source node %s",
fmt_node_id(tmpctx, &info->source));
goto fail;
}
@@ -548,7 +551,7 @@ static struct command_result *do_getroutes(struct command *cmd,
const struct gossmap_node *dstnode =
gossmap_find_node(askrene->gossmap, &info->dest);
if (!dstnode) {
- err = rq_log(tmpctx, rq, LOG_INFORM,
+ err = cmd_log(tmpctx, cmd, LOG_INFORM,
"Unknown destination node %s",
fmt_node_id(tmpctx, &info->dest));
goto fail;
@@ -558,14 +561,14 @@ static struct command_result *do_getroutes(struct command *cmd,
if (have_layer(info->layers, "auto.no_mpp_support") &&
info->dev_algo != ALGO_SINGLE_PATH) {
info->dev_algo = ALGO_SINGLE_PATH;
- rq_log(tmpctx, rq, LOG_DBG,
+ cmd_log(tmpctx, cmd, LOG_DBG,
"Layer no_mpp_support is active we switch to a "
"single path algorithm.");
}
- if (rq->maxparts == 1 &&
+ if (info->maxparts == 1 &&
info->dev_algo != ALGO_SINGLE_PATH) {
info->dev_algo = ALGO_SINGLE_PATH;
- rq_log(tmpctx, rq, LOG_DBG,
+ cmd_log(tmpctx, cmd, LOG_DBG,
"maxparts == 1: switching to a single path algorithm.");
}
@@ -576,7 +579,7 @@ static struct command_result *do_getroutes(struct command *cmd,
time_from_sec(askrene->route_seconds));
child_fd = fork_router_child(rq, info->dev_algo == ALGO_SINGLE_PATH,
deadline, srcnode, dstnode, info->amount,
- info->maxfee, info->finalcltv, info->maxdelay,
+ info->maxfee, info->finalcltv, info->maxdelay, info->maxparts,
include_fees,
cmd->id, cmd->filter, &log_fd, &child_pid);
if (child_fd == -1) {
@@ -585,7 +588,7 @@ static struct command_result *do_getroutes(struct command *cmd,
}
/* FIXME: Go async! */
- process_child_logs(rq, log_fd);
+ process_child_logs(cmd, log_fd);
close(log_fd);
json = grab_fd_str(cmd, child_fd);
close(child_fd);
@@ -594,9 +597,9 @@ static struct command_result *do_getroutes(struct command *cmd,
struct timerel time_delta = timemono_between(time_mono(), time_start);
/* log the time of computation */
- rq_log(tmpctx, rq, LOG_DBG, "get_routes %s %" PRIu64 " ms",
- WEXITSTATUS(child_status) != 0 ? "failed after" : "completed in",
- time_to_msec(time_delta));
+ cmd_log(tmpctx, cmd, LOG_DBG, "get_routes %s %" PRIu64 " ms",
+ WEXITSTATUS(child_status) != 0 ? "failed after" : "completed in",
+ time_to_msec(time_delta));
if (WIFSIGNALED(child_status)) {
err = tal_fmt(tmpctx, "child died with signal %u",
diff --git a/plugins/askrene/askrene.h b/plugins/askrene/askrene.h
index 85acc650..8d16d7cc 100644
--- a/plugins/askrene/askrene.h
+++ b/plugins/askrene/askrene.h
@@ -32,35 +32,30 @@ struct askrene {
/* Information for a single route query. */
struct route_query {
- /* Command pointer, mainly for command id. */
- struct command *cmd;
-
- /* Plugin pointer, for logging mainly */
- struct plugin *plugin;
-
/* This is *not* updated during a query! Has all layers applied. */
const struct gossmap *gossmap;
- /* We need to take in-flight payments into account */
- const struct reserve_htable *reserved;
+ /* command id to use for reservations we create. */
+ const char *cmd_id;
/* Array of layers we're applying */
const struct layer **layers;
- /* Cache of channel capacities for non-reserved, unknown channels. */
- fp16_t *capacities;
-
/* Compact cache of biases */
- s8 *biases;
+ const s8 *biases;
/* Additional per-htlc cost for local channels */
const struct additional_cost_htable *additional_costs;
+ /* We need to take in-flight payments into account (this is
+ * askrene->reserved, so make sure to undo changes! */
+ struct reserve_htable *reserved;
+
+ /* Cache of channel capacities for non-reserved, unknown channels. */
+ fp16_t *capacities;
+
/* channels we disable during computation to meet constraints */
bitmap *disabled_chans;
-
- /* limit the number of paths in the solution */
- u32 maxparts;
};
/* Given a gossmap channel, get the current known min/max */
@@ -70,14 +65,6 @@ void get_constraints(const struct route_query *rq,
struct amount_msat *min,
struct amount_msat *max);
-/* Say something about this route_query */
-const char *rq_log(const tal_t *ctx,
- const struct route_query *rq,
- enum log_level level,
- const char *fmt,
- ...)
- PRINTF_FMT(4, 5);
-
/* Is there a known additional per-htlc cost for this channel? */
struct amount_msat get_additional_per_htlc_cost(const struct route_query *rq,
const struct short_channel_id_dir *scidd);
diff --git a/plugins/askrene/child/entry.c b/plugins/askrene/child/entry.c
index 893bf9e4..231badd4 100644
--- a/plugins/askrene/child/entry.c
+++ b/plugins/askrene/child/entry.c
@@ -78,8 +78,7 @@ static struct route **convert_flows_to_routes(const tal_t *ctx,
if (!amount_msat_add_fee(&msat, h->base_fee,
h->proportional_fee))
- plugin_err(rq->plugin,
- "Adding fee to amount");
+ abort();
delay += h->delay;
rh->scid = gossmap_chan_scid(rq->gossmap,
@@ -178,7 +177,7 @@ int fork_router_child(struct route_query *rq,
const struct gossmap_node *srcnode,
const struct gossmap_node *dstnode,
struct amount_msat amount, struct amount_msat maxfee,
- u32 finalcltv, u32 maxdelay,
+ u32 finalcltv, u32 maxdelay, size_t maxparts,
bool include_fees,
const char *cmd_id,
struct json_filter *cmd_filter,
@@ -226,7 +225,7 @@ int fork_router_child(struct route_query *rq,
} else {
err = default_routes(rq, rq, deadline, srcnode, dstnode,
amount, maxfee, finalcltv, maxdelay,
- &flows, &probability);
+ maxparts, &flows, &probability);
}
if (err) {
write_all(replyfds[1], err, strlen(err));
diff --git a/plugins/askrene/child/entry.h b/plugins/askrene/child/entry.h
index 020d78b1..646cfdd1 100644
--- a/plugins/askrene/child/entry.h
+++ b/plugins/askrene/child/entry.h
@@ -17,7 +17,7 @@ int fork_router_child(struct route_query *rq,
const struct gossmap_node *srcnode,
const struct gossmap_node *dstnode,
struct amount_msat amount, struct amount_msat maxfee,
- u32 finalcltv, u32 maxdelay,
+ u32 finalcltv, u32 maxdelay, size_t maxparts,
bool include_fees,
const char *cmd_id,
struct json_filter *cmd_filter,
diff --git a/plugins/askrene/child/mcf.c b/plugins/askrene/child/mcf.c
index e5af6fd9..2e438583 100644
--- a/plugins/askrene/child/mcf.c
+++ b/plugins/askrene/child/mcf.c
@@ -1341,6 +1341,7 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
const struct gossmap_node *srcnode,
const struct gossmap_node *dstnode, struct amount_msat amount,
struct amount_msat maxfee, u32 finalcltv, u32 maxdelay,
+ size_t maxparts,
struct flow ***flows, double *probability,
struct flow **(*solver)(const tal_t *, const struct route_query *,
const struct gossmap_node *,
@@ -1551,10 +1552,10 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
/* If we're over the number of parts, try to cram excess into the
* largest-capacity parts */
- if (tal_count(*flows) > rq->maxparts) {
+ if (tal_count(*flows) > maxparts) {
struct amount_msat fee;
- error_message = reduce_num_flows(rq, rq, flows, amount, rq->maxparts);
+ error_message = reduce_num_flows(rq, rq, flows, amount, maxparts);
if (error_message) {
*flows = tal_free(*flows);
return error_message;
@@ -1594,14 +1595,13 @@ linear_routes(const tal_t *ctx, struct route_query *rq,
return child_log(rq, LOG_BROKEN,
"%s: check_htlc_max_limits failed", __func__);
}
- if (tal_count(*flows) > rq->maxparts) {
+ if (tal_count(*flows) > maxparts) {
size_t num_flows = tal_count(*flows);
*flows = tal_free(*flows);
return child_log(rq, LOG_BROKEN,
"%s: the number of flows (%zu) exceeds the limit set "
- "on payment parts (%" PRIu32
- "), please submit a bug report",
- __func__, num_flows, rq->maxparts);
+ "on payment parts (%zu), please submit a bug report",
+ __func__, num_flows, maxparts);
}
return NULL;
@@ -1618,11 +1618,12 @@ const char *default_routes(const tal_t *ctx, struct route_query *rq,
const struct gossmap_node *srcnode,
const struct gossmap_node *dstnode,
struct amount_msat amount, struct amount_msat maxfee,
- u32 finalcltv, u32 maxdelay, struct flow ***flows,
+ u32 finalcltv, u32 maxdelay, size_t maxparts,
+ struct flow ***flows,
double *probability)
{
return linear_routes(ctx, rq, deadline, srcnode, dstnode, amount, maxfee,
- finalcltv, maxdelay, flows, probability, minflow);
+ finalcltv, maxdelay, maxparts, flows, probability, minflow);
}
const char *single_path_routes(const tal_t *ctx, struct route_query *rq,
@@ -1635,6 +1636,6 @@ const char *single_path_routes(const tal_t *ctx, struct route_query *rq,
double *probability)
{
return linear_routes(ctx, rq, deadline, srcnode, dstnode, amount, maxfee,
- finalcltv, maxdelay, flows, probability,
+ finalcltv, maxdelay, 1, flows, probability,
single_path_flow);
}
diff --git a/plugins/askrene/child/mcf.h b/plugins/askrene/child/mcf.h
index 020b72e9..27538c52 100644
--- a/plugins/askrene/child/mcf.h
+++ b/plugins/askrene/child/mcf.h
@@ -17,7 +17,7 @@ const char *default_routes(const tal_t *ctx, struct route_query *rq,
const struct gossmap_node *dstnode,
struct amount_msat amount,
struct amount_msat maxfee, u32 finalcltv,
- u32 maxdelay, struct flow ***flows,
+ u32 maxdelay, size_t maxparts, struct flow ***flows,
double *probability);
/* A wrapper to the single-path constrained solver. */
diff --git a/plugins/askrene/child/refine.c b/plugins/askrene/child/refine.c
index e257c469..fe94535e 100644
--- a/plugins/askrene/child/refine.c
+++ b/plugins/askrene/child/refine.c
@@ -24,10 +24,10 @@ static void get_scidd(const struct gossmap *gossmap,
scidd->dir = flow->dirs[i];
}
-static void destroy_reservations(struct reserve_hop *rhops, struct askrene *askrene)
+static void destroy_reservations(struct reserve_hop *rhops, struct reserve_htable *reserved)
{
for (size_t i = 0; i < tal_count(rhops); i++)
- reserve_remove(askrene->reserved, &rhops[i]);
+ reserve_remove(reserved, &rhops[i]);
}
struct reserve_hop *new_reservations(const tal_t *ctx,
@@ -36,7 +36,7 @@ struct reserve_hop *new_reservations(const tal_t *ctx,
struct reserve_hop *rhops = tal_arr(ctx, struct reserve_hop, 0);
/* Unreserve on free */
- tal_add_destructor2(rhops, destroy_reservations, get_askrene(rq->plugin));
+ tal_add_destructor2(rhops, destroy_reservations, rq->reserved);
return rhops;
}
@@ -59,16 +59,15 @@ static void add_reservation(struct reserve_hop **reservations,
struct amount_msat amt)
{
struct reserve_hop rhop, *prev;
- struct askrene *askrene = get_askrene(rq->plugin);
size_t idx;
/* Update in-place if possible */
prev = find_reservation(*reservations, scidd);
if (prev) {
- reserve_remove(askrene->reserved, prev);
+ reserve_remove(rq->reserved, prev);
if (!amount_msat_accumulate(&prev->amount, amt))
abort();
- reserve_add(askrene->reserved, prev, rq->cmd->id);
+ reserve_add(rq->reserved, prev, rq->cmd_id);
return;
}
rhop.scidd = *scidd;
@@ -76,7 +75,7 @@ static void add_reservation(struct reserve_hop **reservations,
/* We don't have to restrict it to a layer, since it's transitory:
* nobody else will see this. */
rhop.layer = NULL;
- reserve_add(askrene->reserved, &rhop, rq->cmd->id);
+ reserve_add(rq->reserved, &rhop, rq->cmd_id);
/* Set capacities entry to 0 so it get_constraints() looks in reserve. */
idx = gossmap_chan_idx(rq->gossmap, chan);
Why this scored 11/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.