From 9b0b1cb24eec31bd88021236817acd611246ea15 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 19 Nov 2025 10:30:39 +0100 Subject: [PATCH 1/3] vici: Enforce maximum length for names when building a message Otherwise, an integer overflow will shorten the name and cause unpredictable outcomes. --- .../plugins/vici/suites/test_message.c | 19 ++++++++++++++++++ src/libcharon/plugins/vici/vici_builder.c | 20 ++++++++++++------- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/libcharon/plugins/vici/suites/test_message.c b/src/libcharon/plugins/vici/suites/test_message.c index 3bfef1e40..0515fce51 100644 --- a/src/libcharon/plugins/vici/suites/test_message.c +++ b/src/libcharon/plugins/vici/suites/test_message.c @@ -284,6 +284,24 @@ START_TEST(test_builder) } END_TEST +START_TEST(test_builder_limits) +{ + vici_builder_t *b; + + b = vici_builder_create(); + b->add(b, VICI_SECTION_START, "this-section-name-is-too-long-to-be-encoded-this-section-name-is-too-long-to-be-encoded-this-section-name-is-too-long-to-be-encoded-this-section-name-is-too-long-to-be-encoded-this-section-name-is-too-long-to-be-encoded-this-section-name-is-too-long-to-be-encoded"); + b->add(b, VICI_KEY_VALUE, "key1", chunk_from_str("value1")); + b->add(b, VICI_SECTION_END); + ck_assert(!b->finalize(b)); + + b = vici_builder_create(); + b->add(b, VICI_SECTION_START, "section1"); + b->add(b, VICI_KEY_VALUE, "this-key-value-name-is-too-long-to-be-encoded-this-key-value-name-is-too-long-to-be-encoded-this-key-value-name-is-too-long-to-be-encoded-this-key-value-name-is-too-long-to-be-encoded-this-key-value-name-is-too-long-to-be-encoded-this-key-value-name-is-too-long-to-be-encoded", chunk_from_str("value1")); + b->add(b, VICI_SECTION_END); + ck_assert(!b->finalize(b)); +} +END_TEST + START_TEST(test_builder_fmt) { enumerator_t *parse, *tmpl; @@ -426,6 +444,7 @@ Suite *message_suite_create() tc = tcase_create("builder encode"); tcase_add_test(tc, test_builder); + tcase_add_test(tc, test_builder_limits); suite_add_tcase(s, tc); tc = tcase_create("builder format encode"); diff --git a/src/libcharon/plugins/vici/vici_builder.c b/src/libcharon/plugins/vici/vici_builder.c index 3703d909a..b9017c84b 100644 --- a/src/libcharon/plugins/vici/vici_builder.c +++ b/src/libcharon/plugins/vici/vici_builder.c @@ -55,8 +55,7 @@ METHOD(vici_builder_t, add, void, private_vici_builder_t *this, vici_type_t type, ...) { va_list args; - char *name = NULL; - chunk_t value = chunk_empty; + chunk_t name = chunk_empty, value = chunk_empty; va_start(args, type); switch (type) @@ -67,10 +66,10 @@ METHOD(vici_builder_t, add, void, break; case VICI_LIST_START: case VICI_SECTION_START: - name = va_arg(args, char*); + name = chunk_from_str(va_arg(args, char*)); break; case VICI_KEY_VALUE: - name = va_arg(args, char*); + name = chunk_from_str(va_arg(args, char*)); value = va_arg(args, chunk_t); break; case VICI_LIST_ITEM: @@ -83,6 +82,13 @@ METHOD(vici_builder_t, add, void, } va_end(args); + if (name.len > 0xff) + { + DBG1(DBG_ENC, "vici name exceeds size limit (%zu > %u)", + name.len, 0xff); + this->error++; + return; + } if (value.len > 0xffff) { DBG1(DBG_ENC, "vici value exceeds size limit (%zu > %u)", @@ -102,18 +108,18 @@ METHOD(vici_builder_t, add, void, switch (type) { case VICI_SECTION_START: - this->writer->write_data8(this->writer, chunk_from_str(name)); + this->writer->write_data8(this->writer, name); this->section++; break; case VICI_SECTION_END: this->section--; break; case VICI_KEY_VALUE: - this->writer->write_data8(this->writer, chunk_from_str(name)); + this->writer->write_data8(this->writer, name); this->writer->write_data16(this->writer, value); break; case VICI_LIST_START: - this->writer->write_data8(this->writer, chunk_from_str(name)); + this->writer->write_data8(this->writer, name); this->list = TRUE; break; case VICI_LIST_ITEM: From 9fe4f9fc9fed0ec494d6ce29c9dc1fee2b6fa27f Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 19 Nov 2025 10:33:39 +0100 Subject: [PATCH 2/3] settings: Increase buffer size for key patterns/parts when looking up settings The buffer size for individual parts of a key now match the maximum length of names in VICI. The full pattern should accommodate several long names (e.g. for connections and child connections in swanctl) even if no printf-specifiers are used. --- src/libstrongswan/settings/settings.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/libstrongswan/settings/settings.c b/src/libstrongswan/settings/settings.c index 4c47a169c..3bfafbb8f 100644 --- a/src/libstrongswan/settings/settings.c +++ b/src/libstrongswan/settings/settings.c @@ -43,6 +43,17 @@ typedef struct private_settings_t private_settings_t; bool settings_parser_parse_file(section_t *root, char *name); bool settings_parser_parse_string(section_t *root, char *settings); +/** + * Buffer size for complete key name/pattern when looking up settings. + */ +#define KEY_FULL_BUF_LEN 2048 + +/** + * Buffer size for a single part of a key name (matches the maximum length + * for names in VICI). + */ +#define KEY_PART_BUF_LEN 257 + /** * Private data of settings */ @@ -287,7 +298,7 @@ static void find_sections_buffered(private_settings_t *this, section_t *section, static section_t *ensure_section(private_settings_t *this, section_t *section, const char *key, va_list args) { - char buf[128], keybuf[512]; + char buf[KEY_PART_BUF_LEN], keybuf[KEY_FULL_BUF_LEN]; if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf)) { @@ -304,7 +315,7 @@ static section_t *ensure_section(private_settings_t *this, section_t *section, static array_t *find_sections(private_settings_t *this, section_t *section, char *key, va_list args, array_t **sections) { - char buf[128], keybuf[512]; + char buf[KEY_PART_BUF_LEN], keybuf[KEY_FULL_BUF_LEN]; if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf)) { @@ -476,7 +487,7 @@ static void remove_value_buffered(private_settings_t *this, section_t *section, void settings_remove_value(settings_t *settings, char *key, ...) { private_settings_t *this = (private_settings_t*)settings; - char buf[128], keybuf[512]; + char buf[KEY_PART_BUF_LEN], keybuf[KEY_FULL_BUF_LEN]; va_list args; if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf)) @@ -499,7 +510,7 @@ void settings_remove_value(settings_t *settings, char *key, ...) static char *find_value(private_settings_t *this, section_t *section, char *key, va_list args) { - char buf[128], keybuf[512], *value = NULL; + char buf[KEY_PART_BUF_LEN], keybuf[KEY_FULL_BUF_LEN], *value = NULL; array_t *sections = NULL; kv_t *kv; @@ -525,7 +536,7 @@ static char *find_value(private_settings_t *this, section_t *section, static void set_value(private_settings_t *this, section_t *section, char *key, va_list args, char *value) { - char buf[128], keybuf[512]; + char buf[KEY_PART_BUF_LEN], keybuf[KEY_FULL_BUF_LEN]; kv_t *kv; if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf)) @@ -939,7 +950,7 @@ METHOD(settings_t, add_fallback, void, { section_t *section; va_list args; - char buf[512]; + char buf[KEY_FULL_BUF_LEN]; this->lock->write_lock(this->lock); va_start(args, fallback); From 80e2cf44a313eda5ee0e0cc546a0a561f65c67eb Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 19 Nov 2025 10:41:46 +0100 Subject: [PATCH 3/3] swanctl: Allow connection and child config names to be longer VICI imposes an upper limit of 256 characters for names, which we should allow for connection and child configs. --- src/swanctl/commands/load_conns.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/swanctl/commands/load_conns.c b/src/swanctl/commands/load_conns.c index 92be16748..11005535c 100644 --- a/src/swanctl/commands/load_conns.c +++ b/src/swanctl/commands/load_conns.c @@ -202,7 +202,7 @@ static bool add_key_values(vici_req_t *req, settings_t *cfg, char *section) static bool add_sections(vici_req_t *req, settings_t *cfg, char *section) { enumerator_t *enumerator; - char *name, buf[256]; + char *name, buf[1024]; bool ret = TRUE; enumerator = cfg->create_section_enumerator(cfg, section); @@ -236,7 +236,7 @@ static bool load_conn(vici_conn_t *conn, settings_t *cfg, vici_req_t *req; vici_res_t *res; bool ret = TRUE; - char buf[BUF_LEN]; + char buf[1024]; snprintf(buf, sizeof(buf), "%s.%s", "connections", section);