From 4fdbd8ba982d84deb3e23a4fb5aa493b4707fce2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sun, 23 Feb 2025 11:23:14 +1030 Subject: [PATCH] lightningd: catch edits of config files *before* we're committed. Another report, that we crash if it's edited. We should check that too! Reported-by: daywalker90 Signed-off-by: Rusty Russell --- lightningd/configs.c | 78 ++++++++++++++++++++++++++++++++------------ tests/test_misc.py | 49 ++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 20 deletions(-) diff --git a/lightningd/configs.c b/lightningd/configs.c index 4ee885322..a4da9fa90 100644 --- a/lightningd/configs.c +++ b/lightningd/configs.c @@ -416,12 +416,41 @@ static size_t append_to_file(struct lightningd *ld, return strcount(buffer, "\n") + 1; } +static const char *grab_and_check(const tal_t *ctx, + const char *fname, + size_t linenum, + const char *expected, + char ***lines) +{ + char *contents; + + contents = grab_file(tmpctx, fname); + if (!contents) + return tal_fmt(ctx, "Could not load configfile %s: %s", + fname, strerror(errno)); + + /* These are 1-based! */ + assert(linenum > 0); + *lines = tal_strsplit(ctx, contents, "\r\n", STR_EMPTY_OK); + if (linenum >= tal_count(*lines)) + return tal_fmt(ctx, "Configfile %s no longer has %zu lines!", + fname, linenum); + + if (!streq((*lines)[linenum - 1], expected)) + return tal_fmt(ctx, "Configfile %s line %zu changed from %s to %s!", + fname, linenum, + expected, + (*lines)[linenum - 1]); + return NULL; +} + /* This comments out the config file entry or maybe replace one */ static void configfile_replace_var(struct lightningd *ld, const struct configvar *cv, const char *replace) { - char *contents, **lines, *template; + char **lines, *template, *contents; + const char *err; int outfd; switch (cv->src) { @@ -437,21 +466,11 @@ static void configfile_replace_var(struct lightningd *ld, break; } - contents = grab_file(tmpctx, cv->file); - if (!contents) - fatal("Could not load configfile %s: %s", - cv->file, strerror(errno)); - - lines = tal_strsplit(contents, contents, "\r\n", STR_EMPTY_OK); - if (cv->linenum - 1 >= tal_count(lines)) - fatal("Configfile %s no longer has %u lines!", - cv->file, cv->linenum); - - if (!streq(lines[cv->linenum - 1], cv->configline)) - fatal("Configfile %s line %u changed from %s to %s!", - cv->file, cv->linenum, - cv->configline, - lines[cv->linenum - 1]); + /* If it changed *now*, that's fatal: we already set it locally! */ + err = grab_and_check(tmpctx, cv->file, cv->linenum, cv->configline, + &lines); + if (err) + fatal("%s", err); if (replace) lines[cv->linenum - 1] = cast_const(char *, replace); @@ -595,14 +614,12 @@ static bool dir_writable(const char *fname) /* Returns config file name if not writable */ static const char *config_not_writable(const tal_t *ctx, struct command *cmd, - const struct opt_table *ot) + const struct configvar *oldcv) { struct lightningd *ld = cmd->ld; - struct configvar *oldcv; const char *fname; /* If it exists before, we will need to replace that file (rename) */ - oldcv = configvar_first(ld->configvars, opt_names_arr(tmpctx, ot)); if (oldcv && oldcv->file) { /* We will rename */ if (!dir_writable(oldcv->file)) @@ -645,11 +662,32 @@ static struct command_result *json_setconfig(struct command *cmd, assert(!(ot->type & OPT_MULTI)); if (!*transient) { - const char *fname = config_not_writable(cmd, cmd, ot); + const struct configvar *cv; + const char *fname; + + cv = configvar_first(cmd->ld->configvars, + opt_names_arr(tmpctx, ot)); + + fname = config_not_writable(cmd, cmd, cv); if (fname) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Cannot write to config file %s", fname); + + /* Check if old config has changed (so we couldn't be able + * to comment it out! */ + if (cv && cv->file) { + const char *changed; + char **lines; + + changed = grab_and_check(tmpctx, + cv->file, cv->linenum, + cv->configline, + &lines); + if (changed) + return command_fail(cmd, JSONRPC2_INVALID_PARAMS, + "%s", changed); + } } /* We use arg = NULL to tell callback it's only for testing */ diff --git a/tests/test_misc.py b/tests/test_misc.py index 9f5f49bb3..b8e26ff05 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -4278,6 +4278,55 @@ def test_setconfig_access(node_factory, bitcoind): os.chmod(includedir, 0o700) +def test_setconfig_changed(node_factory, bitcoind): + """Test that we correctly fail (not crash) if config file changed""" + l1 = node_factory.get_node(start=False) + + netconfigfile = os.path.join(l1.daemon.opts.get("lightning-dir"), TEST_NETWORK, 'config') + with open(netconfigfile, 'w') as file: + file.write("min-capacity-sat=100") + l1.start() + + assert l1.rpc.listconfigs(config="min-capacity-sat")['configs']['min-capacity-sat']['value_int'] == 100 + + # Change it underneath + with open(netconfigfile, 'w') as file: + file.write("#some comment\nmin-capacity-sat=100") + + # This will fail. + with pytest.raises(RpcError, match=f'Configfile {netconfigfile} line 1 changed from min-capacity-sat=100 to #some comment!'): + l1.rpc.check("setconfig", config="min-capacity-sat", val=9999) + with pytest.raises(RpcError, match=f'Configfile {netconfigfile} line 1 changed from min-capacity-sat=100 to #some comment!'): + l1.rpc.setconfig(config="min-capacity-sat", val=9999) + + # Restore it. + with open(netconfigfile, 'w') as file: + file.write("min-capacity-sat=100") + + # Succeeds + l1.rpc.setconfig(config="min-capacity-sat", val=9999) + + # Now mess with config.setconfig... + setconfigfile = netconfigfile + ".setconfig" + with open(setconfigfile, 'w') as file: + pass + + # Now this will fail (truncated) + with pytest.raises(RpcError, match=f'Configfile {setconfigfile} no longer has 2 lines'): + l1.rpc.check("setconfig", config="min-capacity-sat", val=9999) + with pytest.raises(RpcError, match=f'Configfile {setconfigfile} no longer has 2 lines'): + l1.rpc.setconfig(config="min-capacity-sat", val=9999) + + # This will fail (changed) + with open(setconfigfile, 'w') as file: + file.write("# Created and update by setconfig, but you can edit this manually when node is stopped.\nmin-capacity-sat=999") + + with pytest.raises(RpcError, match=f'Configfile {setconfigfile} line 2 changed from min-capacity-sat=9999 to min-capacity-sat=999!'): + l1.rpc.check("setconfig", config="min-capacity-sat", val=9999) + with pytest.raises(RpcError, match=f'Configfile {setconfigfile} line 2 changed from min-capacity-sat=9999 to min-capacity-sat=999!'): + l1.rpc.setconfig(config="min-capacity-sat", val=9999) + + @unittest.skipIf(os.getenv('TEST_DB_PROVIDER', 'sqlite3') != 'sqlite3', "deletes database, which is assumed sqlite3") def test_recover_command(node_factory, bitcoind): l1, l2 = node_factory.get_nodes(2)