SymbX/systemd
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bus-map-properties: reject wrong variant type

Browse Source 
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 9f6eb1cd58

Assisted-by: kres (claude-opus-4-7)
Co-developed-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Luca Boccassi
2026-06-11 09:43:09 +02:00
committed by Daan De Meyer
parent 0aaba6eb83
commit 39bc1dfa0f
2 changed files with 195 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/shared/bus-map-properties.c
+21 -13
View File
@@ -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)
src/test/test-bus-util.c
+174
View File
@@ -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);