>From 4af6d213e2e5996b87b8ed15c1d844fa5c620b66 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 6 Nov 2015 20:54:13 +0100 Subject: [PATCH 3/3] sbus: Check string arguments for valid UTF-8 strings libdbus abort()s when a string argument is not valid UTF-8. Since the arguments sometimes come from untrusted sources, it's better to check the string validity explicitly. --- configure.ac | 9 ++++++++ src/sbus/sssd_dbus_request.c | 46 ++++++++++++++++++++++++++++++++++++++++- src/tests/sbus_tests.c | 49 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index fe3e137b75fd0d42aaa49b1e477d1c5910457f08..3fe824224fd9ef75b3f6f6b793a30cd05e4f3c6d 100644 --- a/configure.ac +++ b/configure.ac @@ -218,10 +218,19 @@ fi if test x$has_dbus != xno; then SAFE_LIBS="$LIBS" LIBS="$DBUS_LIBS" + SAFE_CFLAGS=$CFLAGS + CFLAGS="$CFLAGS $DBUS_CFLAGS" + AC_CHECK_FUNC([dbus_watch_get_unix_fd], AC_DEFINE([HAVE_DBUS_WATCH_GET_UNIX_FD], [1], [Define if dbus_watch_get_unix_fd exists])) + AC_CHECK_TYPES([DBusBasicValue], + [], + [], + [ #include ]) + LIBS="$SAFE_LIBS" + CFLAGS=$SAFE_CFLAGS fi # work around a bug in cov-build from Coverity diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c index 552bb5da86668f4e94c0fb7699753706a0698c43..114e7a4095d51a792d1d5173682149c8699d457a 100644 --- a/src/sbus/sssd_dbus_request.c +++ b/src/sbus/sssd_dbus_request.c @@ -19,6 +19,7 @@ */ #include "util/util.h" +#include "util/sss_utf8.h" #include "sbus/sssd_dbus.h" #include "sbus/sssd_dbus_private.h" @@ -96,23 +97,66 @@ int sbus_request_finish(struct sbus_request *dbus_req, return talloc_free(dbus_req); } +static int sbus_request_valist_check(va_list va, int first_arg_type) +{ + int ret = EOK; +#ifdef HAVE_DBUSBASICVALUE + int type; + va_list va_check; + const DBusBasicValue *value; + bool ok; + + va_copy(va_check, va); + + type = first_arg_type; + while (type != DBUS_TYPE_INVALID) { + value = va_arg(va_check, const DBusBasicValue*); + + if (type == DBUS_TYPE_STRING) { + ok = sss_utf8_check((const uint8_t *) value->str, + strlen(value->str)); + if (!ok) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Back end message [%s] contains invalid non-UTF8 " \ + "characters"); + ret = EINVAL; + break; + } + } + type = va_arg(va_check, int); + } + + va_end(va_check); +#endif /* HAVE_DBUSBASICVALUE */ + return ret; +} + int sbus_request_return_and_finish(struct sbus_request *dbus_req, int first_arg_type, ...) { DBusMessage *reply; + DBusError error = DBUS_ERROR_INIT; dbus_bool_t dbret; va_list va; int ret; + va_start(va, first_arg_type); + ret = sbus_request_valist_check(va, first_arg_type); + if (ret != EOK) { + va_end(va); + dbus_set_error_const(&error, DBUS_ERROR_INVALID_ARGS, INTERNAL_ERROR); + return sbus_request_fail_and_finish(dbus_req, &error); + } + reply = dbus_message_new_method_return(dbus_req->message); if (!reply) { + va_end(va); DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus message\n"); sbus_request_finish(dbus_req, NULL); return ENOMEM; } - va_start(va, first_arg_type); dbret = dbus_message_append_args_valist(reply, first_arg_type, va); va_end(va); diff --git a/src/tests/sbus_tests.c b/src/tests/sbus_tests.c index 598cc536db436032b6d14c6515cd411b1bd2279e..b472659639e3dce0733dde4ed54a55dcb40c191e 100644 --- a/src/tests/sbus_tests.c +++ b/src/tests/sbus_tests.c @@ -43,6 +43,7 @@ #define PILOT_IFACE "test.Pilot" #define PILOT_BLINK "Blink" #define PILOT_EAT "Eat" +#define PILOT_CRASH "Crash" #define PILOT_IFACE_INTROSPECT \ "\n" \ " \n" \ " \n" \ + " \n" \ " \n" \ "\n" @@ -80,6 +82,7 @@ struct pilot_vtable { struct sbus_vtable vtable; sbus_msg_handler_fn Blink; sbus_msg_handler_fn Eat; + sbus_msg_handler_fn Crash; }; const struct sbus_method_meta pilot_methods[] = { @@ -97,6 +100,13 @@ const struct sbus_method_meta pilot_methods[] = { offsetof(struct pilot_vtable, Eat), NULL }, + { + PILOT_CRASH, /* method name */ + NULL, /* in args: manually parsed */ + NULL, /* out args: manually parsed */ + offsetof(struct pilot_vtable, Crash), + NULL + }, { NULL, } }; @@ -169,10 +179,21 @@ static int eat_handler(struct sbus_request *req, void *data) return sbus_request_return_and_finish(req, DBUS_TYPE_INVALID); } +static int crash_handler(struct sbus_request *req, void *data) +{ + /* Pilot crashes by returning a malformed UTF-8 string */ + const char *invalid = "ad\351la\357d"; + + return sbus_request_return_and_finish(req, + DBUS_TYPE_STRING, &invalid, + DBUS_TYPE_INVALID); +} + struct pilot_vtable pilot_impl = { { &pilot_meta, 0 }, .Blink = blink_handler, .Eat = eat_handler, + .Crash = crash_handler, }; static int pilot_test_server_init(struct sbus_connection *server, void *unused) @@ -304,6 +325,33 @@ START_TEST(test_request_parse_bad_args) } END_TEST +START_TEST(test_request_dontcrash) +{ +#ifdef HAVE_DBUSBASICVALUE + TALLOC_CTX *ctx; + DBusConnection *client; + DBusError error = DBUS_ERROR_INIT; + DBusMessage *reply; + + ctx = talloc_new(NULL); + client = test_dbus_setup_mock(ctx, NULL, pilot_test_server_init, NULL); + + reply = test_dbus_call_sync(client, + "/test/leela", + PILOT_IFACE, + PILOT_CRASH, + &error, + DBUS_TYPE_INVALID); /* bad agruments */ + ck_assert(reply == NULL); + ck_assert(dbus_error_is_set(&error)); + ck_assert(dbus_error_has_name(&error, DBUS_ERROR_INVALID_ARGS)); + dbus_error_free(&error); + + talloc_free(ctx); +#endif /* HAVE_DBUSBASICVALUE */ +} +END_TEST + START_TEST(test_introspection) { TALLOC_CTX *ctx; @@ -373,6 +421,7 @@ TCase *create_sbus_tests(void) tcase_add_test(tc, test_raw_handler); tcase_add_test(tc, test_request_parse_ok); tcase_add_test(tc, test_request_parse_bad_args); + tcase_add_test(tc, test_request_dontcrash); tcase_add_test(tc, test_introspection); tcase_add_test(tc, test_sbus_new_error); -- 2.4.3