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 <noreply@anthropic.com>
Signed-off-by: Paul Meyer <katexochen0@gmail.com>

diff --git a/src/vmspawn/meson.build b/src/vmspawn/meson.build
index 6bc31c77c692d537fcc6bd53e3f8087ebcae9e8c..76617cc6d46e3b5cefb30690b232b725c2f27983 100644 (file)
--- 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 (file)
index 0000000..001a431
--- /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 08908ff294692757789ebfdc20427644d90a1513..d17e841e35f324c6f169ee3c77841bbd98c64b98 100644 (file)
--- 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);