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.

Reviewed-by: Sumit Bose <sbose@redhat.com>

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 <dbus/dbus.h> ])
+
     LIBS="$SAFE_LIBS"
+    CFLAGS=$SAFE_CFLAGS
 fi
 
 # work around a bug in cov-build from Coverity

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,
+                         "sbus message argument [%s] contains invalid " \
+                         "non-UTF8 characters", value->str);
+                 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);
 

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 \
         "<!DOCTYPE node PUBLIC \"-//freedesktop//DTD D-BUS Object Introspection 1.0//EN\"\n" \
@@ -72,6 +73,7 @@
         "  <interface name=\"test.Pilot\">\n" \
         "    <method name=\"Blink\" />\n" \
         "    <method name=\"Eat\" />\n" \
+        "    <method name=\"Crash\" />\n" \
         "  </interface>\n" \
         "</node>\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);