From 39bc1dfa0f4a4c78b365b2ded51e8ec578f93a58 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 10 Jun 2026 16:17:02 +0100 Subject: [PATCH] bus-map-properties: reject wrong variant type bus_message_map_all_properties() fed the peer-supplied wire signature straight into the variant dispatch and never compared it against the declared prop->signature, so map_basic() wrote at the wire type's native width into a slot sized for the declared type (over-wide numeric writes, peer-controlled pointers into char** slots later freed by strv_free()). Compare against prop->signature and skip the variant on mismatch. This is in practice not a problem as the servers are trusted, and this only affects clients. Follow-up for 9f6eb1cd58f2ddf2eb6ba0e4de056e13d938af75 Assisted-by: kres (claude-opus-4-7) Co-developed-by: Claude Opus 4.8 --- src/shared/bus-map-properties.c | 34 ++++--- src/test/test-bus-util.c | 174 ++++++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 13 deletions(-) diff --git a/src/shared/bus-map-properties.c b/src/shared/bus-map-properties.c index 49b1e44030..d695ca4fc2 100644 --- a/src/shared/bus-map-properties.c +++ b/src/shared/bus-map-properties.c @@ -173,21 +173,29 @@ int bus_message_map_all_properties( if (r < 0) return bus_log_parse_error_debug(r); - r = sd_bus_message_enter_container(m, SD_BUS_TYPE_VARIANT, contents); - if (r < 0) - return bus_log_parse_error_debug(r); + if (prop->signature && !streq(contents, prop->signature)) { + log_debug("Unexpected type for property '%s': expected '%s', got '%s', ignoring.", + member, prop->signature, contents); + r = sd_bus_message_skip(m, "v"); + if (r < 0) + return bus_log_parse_error_debug(r); + } else { + r = sd_bus_message_enter_container(m, SD_BUS_TYPE_VARIANT, contents); + if (r < 0) + return bus_log_parse_error_debug(r); - v = (uint8_t *)userdata + prop->offset; - if (map[i].set) - r = prop->set(sd_bus_message_get_bus(m), member, m, reterr_error, v); - else - r = map_basic(m, flags, v); - if (r < 0) - return bus_log_parse_error_debug(r); + v = (uint8_t*) userdata + prop->offset; + if (prop->set) + r = prop->set(sd_bus_message_get_bus(m), member, m, reterr_error, v); + else + r = map_basic(m, flags, v); + if (r < 0) + return bus_log_parse_error_debug(r); - r = sd_bus_message_exit_container(m); - if (r < 0) - return bus_log_parse_error_debug(r); + r = sd_bus_message_exit_container(m); + if (r < 0) + return bus_log_parse_error_debug(r); + } } else { r = sd_bus_message_skip(m, "v"); if (r < 0) diff --git a/src/test/test-bus-util.c b/src/test/test-bus-util.c index 6361d7f17a..55d094afc1 100644 --- a/src/test/test-bus-util.c +++ b/src/test/test-bus-util.c @@ -2,6 +2,8 @@ #include "sd-bus.h" +#include "bus-internal.h" +#include "bus-map-properties.h" #include "bus-util.h" #include "log.h" #include "tests.h" @@ -44,4 +46,176 @@ TEST(destroy_callback) { ASSERT_EQ(n_called, 1); } +/* Guard field detects an over-wide write spilling past the declared slot. */ +struct numeric_target { + uint32_t value; + uint32_t guard; +}; + +struct strv_target { + char **list; + char **guard; +}; + +#define GUARD_PATTERN UINT32_C(0x5A5A5A5A) + +/* Unconnected bus, only good for constructing messages locally. */ +static void map_properties_fake_bus(sd_bus **ret) { + sd_bus *bus = NULL; + + ASSERT_OK(sd_bus_new(&bus)); + bus->state = BUS_RUNNING; /* Fake state to allow message creation */ + *ret = bus; +} + +static void map_properties_seal_for_read(sd_bus_message *m) { + ASSERT_OK(sd_bus_message_seal(m, 1, 0)); + ASSERT_OK(sd_bus_message_rewind(m, true)); +} + +/* Over-wide scalar wire type must not write past the declared slot. */ +TEST(map_all_properties_numeric_type_confusion) { + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + + struct numeric_target data = { .value = 0, .guard = GUARD_PATTERN }; + + static const struct bus_properties_map map[] = { + { "Value", "u", NULL, offsetof(struct numeric_target, value) }, + {}, + }; + + map_properties_fake_bus(&bus); + + ASSERT_OK(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/", "foo.bar", "Get")); + ASSERT_OK(sd_bus_message_append(m, "a{sv}", 1, "Value", "t", (uint64_t) 0xAABBCCDD11223344ULL)); + map_properties_seal_for_read(m); + + ASSERT_OK(bus_message_map_all_properties(m, map, /* flags= */ 0, /* reterr_error= */ NULL, &data)); + + /* Skipped, adjacent field intact. */ + ASSERT_EQ(data.guard, GUARD_PATTERN); +} + +/* Scalar "t" against a declared "as" char** slot must be skipped, not planted+freed. */ +TEST(map_all_properties_pointer_type_confusion) { + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + + char *guard_marker = NULL; + struct strv_target data = { .guard = &guard_marker }; + + static const struct bus_properties_map map[] = { + { "List", "as", NULL, offsetof(struct strv_target, list) }, + {}, + }; + + map_properties_fake_bus(&bus); + + ASSERT_OK(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/", "foo.bar", "Get")); + ASSERT_OK(sd_bus_message_append(m, "a{sv}", 1, "List", "t", (uint64_t) 0xDEADBEEFCAFEBABEULL)); + map_properties_seal_for_read(m); + + ASSERT_OK(bus_message_map_all_properties(m, map, /* flags= */ 0, /* reterr_error= */ NULL, &data)); + + ASSERT_NULL(data.list); + ASSERT_PTR_EQ(data.guard, &guard_marker); +} + +/* Mismatched array element type ("ao" vs "as") must be skipped. "ao" is one read_strv_extend() + * accepts, so the signature guard, not the reader, is what rejects it. */ +TEST(map_all_properties_array_type_confusion) { + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + + char *guard_marker = NULL; + struct strv_target data = { .guard = &guard_marker }; + + static const struct bus_properties_map map[] = { + { "List", "as", NULL, offsetof(struct strv_target, list) }, + {}, + }; + + map_properties_fake_bus(&bus); + + ASSERT_OK(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/", "foo.bar", "Get")); + ASSERT_OK(sd_bus_message_append(m, "a{sv}", 1, "List", "ao", 1, "/foo/bar")); + map_properties_seal_for_read(m); + + ASSERT_OK(bus_message_map_all_properties(m, map, /* flags= */ 0, /* reterr_error= */ NULL, &data)); + + /* Skipped before map_basic()'s strv branch runs. */ + ASSERT_NULL(data.list); + ASSERT_PTR_EQ(data.guard, &guard_marker); +} + +/* Correctly typed "u" variant is still read into the slot. */ +TEST(map_all_properties_correct_type) { + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + + struct numeric_target data = { .value = 0, .guard = GUARD_PATTERN }; + + static const struct bus_properties_map map[] = { + { "Value", "u", NULL, offsetof(struct numeric_target, value) }, + {}, + }; + + map_properties_fake_bus(&bus); + + ASSERT_OK(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/", "foo.bar", "Get")); + ASSERT_OK(sd_bus_message_append(m, "a{sv}", 1, "Value", "u", (uint32_t) 42)); + map_properties_seal_for_read(m); + + ASSERT_OK(bus_message_map_all_properties(m, map, /* flags= */ 0, /* reterr_error= */ NULL, &data)); + + ASSERT_EQ(data.value, UINT32_C(42)); + ASSERT_EQ(data.guard, GUARD_PATTERN); +} + +static int record_set_called(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *reterr_error, void *userdata) { + bool *called = ASSERT_PTR(userdata); + int r; + + /* Consume the value, like a real set callback would. */ + r = sd_bus_message_skip(m, NULL); + if (r < 0) + return r; + + *called = true; + return 0; +} + +/* Wrong-type variant must be skipped before a property-set callback runs. */ +TEST(map_all_properties_set_callback_type_confusion) { + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; + + bool called = false; + + static const struct bus_properties_map map[] = { + { "List", "as", record_set_called, 0 }, + {}, + }; + + map_properties_fake_bus(&bus); + + /* Wrong type: the set callback must not run. */ + ASSERT_OK(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/", "foo.bar", "Get")); + ASSERT_OK(sd_bus_message_append(m, "a{sv}", 1, "List", "t", (uint64_t) 0xDEADBEEFCAFEBABEULL)); + map_properties_seal_for_read(m); + + ASSERT_OK(bus_message_map_all_properties(m, map, /* flags= */ 0, /* reterr_error= */ NULL, &called)); + ASSERT_FALSE(called); + + /* Matching type: the set callback runs as before. */ + m = sd_bus_message_unref(m); + ASSERT_OK(sd_bus_message_new_method_call(bus, &m, "foo.bar", "/", "foo.bar", "Get")); + ASSERT_OK(sd_bus_message_append(m, "a{sv}", 1, "List", "as", 1, "item")); + map_properties_seal_for_read(m); + + ASSERT_OK(bus_message_map_all_properties(m, map, /* flags= */ 0, /* reterr_error= */ NULL, &called)); + ASSERT_TRUE(called); +} + DEFINE_TEST_MAIN(LOG_DEBUG);