From: Paul Meyer Date: Sat, 13 Jun 2026 07:25:28 +0000 (+0200) Subject: vmspawn: enforce an identifier grammar for QEMU config types/ids/keys X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=6212baaf828f530398723dfbc9ba1d1aa07368cb;p=thirdparty%2Fsystemd.git vmspawn: enforce an identifier grammar for QEMU config types/ids/keys qemu_config_section()/qemu_config_key() write section types and keys unquoted ([type], key = …), but the type check only rejected '\n' while the structurally identical id/key checks rejected more, even though the unquoted type slot is the most sensitive. Replace the three ad-hoc denylists with one allowlist matching QEMU's identifier grammar ([A-Za-z0-9._-], non-empty). This is merely hardening, all callers pass constant input today. For the value check use string_is_safe() for now, which is a bit stricter than QEMUs own config validation. Add test-vmspawn-qemu-config covering both. Co-developed-by: Claude Opus 4.8 Signed-off-by: Paul Meyer --- diff --git a/src/vmspawn/meson.build b/src/vmspawn/meson.build index 6bc31c77c69..76617cc6d46 100644 --- a/src/vmspawn/meson.build +++ b/src/vmspawn/meson.build @@ -7,7 +7,6 @@ endif vmspawn_sources = files( 'vmspawn.c', 'vmspawn-bind-volume.c', - 'vmspawn-qemu-config.c', 'vmspawn-qmp.c', 'vmspawn-varlink.c', 'vmspawn-settings.c', @@ -16,6 +15,7 @@ vmspawn_sources = files( ) vmspawn_extract_sources = files( 'vmspawn-util.c', + 'vmspawn-qemu-config.c', ) executables += [ @@ -29,4 +29,8 @@ executables += [ 'sources' : files('test-vmspawn-util.c'), 'objects' : ['systemd-vmspawn'], }, + test_template + { + 'sources' : files('test-vmspawn-qemu-config.c'), + 'objects' : ['systemd-vmspawn'], + }, ] diff --git a/src/vmspawn/test-vmspawn-qemu-config.c b/src/vmspawn/test-vmspawn-qemu-config.c new file mode 100644 index 00000000000..001a4319815 --- /dev/null +++ b/src/vmspawn/test-vmspawn-qemu-config.c @@ -0,0 +1,120 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "alloc-util.h" +#include "memstream-util.h" +#include "tests.h" +#include "vmspawn-qemu-config.h" + +/* Render a single "key = value" pair through qemu_config_key() and return both the function's return code + * and (on success, when ret is set) the rendered bytes. */ +static int render_key(char **ret, const char *key, const char *value) { + _cleanup_(memstream_done) MemStream m = {}; + FILE *f; + int r; + + assert_se(f = memstream_init(&m)); + + r = qemu_config_key(f, key, value); + if (r < 0) + return r; + + if (ret) + assert_se(memstream_finalize(&m, ret, NULL) >= 0); + + return r; +} + +static int render_section(char **ret, const char *type, const char *id) { + _cleanup_(memstream_done) MemStream m = {}; + FILE *f; + int r; + + assert_se(f = memstream_init(&m)); + + r = qemu_config_section(f, type, id); + if (r < 0) + return r; + + if (ret) + assert_se(memstream_finalize(&m, ret, NULL) >= 0); + + return r; +} + +TEST(qemu_config_key_valid) { + _cleanup_free_ char *out = NULL; + + ASSERT_OK(render_key(&out, "readonly", "on")); + ASSERT_STREQ(out, " readonly = \"on\"\n"); + + /* Keys may carry the full identifier charset. */ + out = mfree(out); + ASSERT_OK(render_key(&out, "confidential-guest-support", "snp0")); + ASSERT_STREQ(out, " confidential-guest-support = \"snp0\"\n"); +} + +TEST(qemu_config_value_permits_path_bytes) { + _cleanup_free_ char *out = NULL; + + /* Legitimate path bytes, including backslash, must pass through a quoted value verbatim. */ + ASSERT_OK(render_key(&out, "file", "/usr/share/edk2/ovmf/OVMF_CODE.fd")); + ASSERT_STREQ(out, " file = \"/usr/share/edk2/ovmf/OVMF_CODE.fd\"\n"); + + out = mfree(out); + ASSERT_OK(render_key(&out, "file", "/weird/but\\legal/path")); + ASSERT_STREQ(out, " file = \"/weird/but\\legal/path\"\n"); + + /* '=', spaces, brackets etc. are all fine inside a quoted value. */ + out = mfree(out); + ASSERT_OK(render_key(&out, "cpus", "1,sockets=1")); + ASSERT_STREQ(out, " cpus = \"1,sockets=1\"\n"); +} + +TEST(qemu_config_value_rejects_quote_and_newline) { + /* These two bytes are the only ones that can break out of the quoted token; both must be refused. */ + ASSERT_ERROR(render_key(NULL, "file", "ab\"cd"), EINVAL); + ASSERT_ERROR(render_key(NULL, "file", "ab\ncd"), EINVAL); +} + +TEST(qemu_config_key_name_rejects_structure) { + /* Key names are emitted unquoted, so they must be plain identifiers. */ + ASSERT_ERROR(render_key(NULL, "fo=o", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "fo\no", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "fo\"o", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "fo o", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "foo]", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "/foo", "v"), EINVAL); + ASSERT_ERROR(render_key(NULL, "", "v"), EINVAL); +} + +TEST(qemu_config_section_valid) { + _cleanup_free_ char *out = NULL; + + ASSERT_OK(render_section(&out, "drive", "ovmf-code")); + ASSERT_STREQ(out, "\n[drive \"ovmf-code\"]\n"); + + out = mfree(out); + ASSERT_OK(render_section(&out, "smp-opts", NULL)); + ASSERT_STREQ(out, "\n[smp-opts]\n"); +} + +TEST(qemu_config_section_type_rejects_structure) { + /* The section type is emitted unquoted as "[type]" — a ']' or newline here would let a caller + * close the header early or open a new section. */ + ASSERT_ERROR(render_section(NULL, "drive]", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "dr\nive", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "dr ive", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "dr\"ive", "id"), EINVAL); + ASSERT_ERROR(render_section(NULL, "", "id"), EINVAL); +} + +TEST(qemu_config_section_id_rejects_structure) { + /* The id is quoted, but ']' and backslash are still hardened against future runtime data. */ + ASSERT_ERROR(render_section(NULL, "drive", "i\"d"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", "i\nd"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", "i]d"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", "i\\d"), EINVAL); + ASSERT_ERROR(render_section(NULL, "drive", ""), EINVAL); +} + +DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/vmspawn/vmspawn-qemu-config.c b/src/vmspawn/vmspawn-qemu-config.c index 08908ff2946..d17e841e35f 100644 --- a/src/vmspawn/vmspawn-qemu-config.c +++ b/src/vmspawn/vmspawn-qemu-config.c @@ -5,22 +5,19 @@ #include "alloc-util.h" #include "errno-util.h" #include "log.h" +#include "string-util.h" #include "vmspawn-qemu-config.h" -static bool qemu_config_type_valid(const char *type) { - return !strchr(type, '\n'); -} - -static bool qemu_config_id_valid(const char *id) { - return !strpbrk(id, "\"\n"); -} - -static bool qemu_config_key_name_valid(const char *key) { - return !strpbrk(key, "=\n"); +/* Enforce QEMU's identifier grammar, so runtime data can never inject config structure. */ +static bool qemu_config_identifier_valid(const char *s) { + return !isempty(s) && in_charset(s, ALPHANUMERICAL ".-_"); } +/* Values are written quoted ('key = "%s"') and QEMU reads them literally between the quotes (no escapes), + * only '"' and newline can break out. Nevertheless run the value through string_is_safe(), which is + * stricter than QEMU's own parser. Relax if a valid use case occurs. */ static bool qemu_config_value_valid(const char *value) { - return !strpbrk(value, "\"\n"); + return string_is_safe(value, STRING_ALLOW_EMPTY | STRING_ALLOW_BACKSLASHES | STRING_ALLOW_GLOBS); } int qemu_config_key(FILE *f, const char *key, const char *value) { @@ -28,10 +25,10 @@ int qemu_config_key(FILE *f, const char *key, const char *value) { assert(key); assert(value); - if (!qemu_config_key_name_valid(key)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config key '%s' contains '=' or newline.", key); + if (!qemu_config_identifier_valid(key)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config key '%s' is not a valid identifier.", key); if (!qemu_config_value_valid(value)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config value '%s' contains quote or newline.", value); + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config value '%s' contains unsafe characters.", value); if (fprintf(f, " %s = \"%s\"\n", key, value) < 0) return -errno_or_else(EIO); @@ -64,12 +61,12 @@ int qemu_config_section_impl(FILE *f, const char *type, const char *id, ...) { assert(f); assert(type); - if (!qemu_config_type_valid(type)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section type '%s' contains newline.", type); + if (!qemu_config_identifier_valid(type)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section type '%s' is not a valid identifier.", type); if (id) { - if (!qemu_config_id_valid(id)) - return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section id '%s' contains quote or newline.", id); + if (!qemu_config_identifier_valid(id)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "QEMU config section id '%s' is not a valid identifier.", id); fprintf(f, "\n[%s \"%s\"]\n", type, id); } else fprintf(f, "\n[%s]\n", type);