[SSSD] [PATCH] Guard against invalid DP messages

Jakub Hrozek jhrozek at redhat.com
Fri Nov 13 13:56:53 UTC 2015


On Fri, Nov 13, 2015 at 02:27:05PM +0100, Sumit Bose wrote:
> > +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");
> 
> value->str is missing, additionally sbus_request_return_and_finish() is
> used by the responders as well so 'Back end message' should be changed
> to 'S-Bus message' or similar.
> 
> bye,
> Sumit

Oops, sorry..
-------------- next part --------------
>From 9f50257515cdb0e839fa31130dbfa5c5454225d7 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 6 Nov 2015 12:33:59 +0100
Subject: [PATCH 1/3] DP: Drop dp_pam_err_to_string

Resolves:
    https://fedorahosted.org/sssd/ticket/2861

All back end requests were using pam_strerror() to print additional info
about why request failed. Since pam_strerror() returns localized message
and we don't know the locale beforehand, this message failed to be
transferred through D-Bus, resulting in a crash.
---
 src/providers/data_provider_be.c | 75 ++++++++++++----------------------------
 1 file changed, 23 insertions(+), 52 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 104016ca19c093dcbb4b0c1011f06e4bb4e2a49c..5f299a7209d3d1f1e83c625ae9db6d731a5cf5ab 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -115,6 +115,23 @@ struct bet_queue_item {
 
 };
 
+static const char *dp_err_to_string(int dp_err_type)
+{
+    switch (dp_err_type) {
+    case DP_ERR_OK:
+        return "Success";
+    case DP_ERR_OFFLINE:
+        return "Provider is Offline";
+    case DP_ERR_TIMEOUT:
+        return "Request timed out";
+    case DP_ERR_FATAL:
+    default:
+        return "Internal Error";
+    }
+
+    return "Unknown Error";
+}
+
 #define REQ_PHASE_ACCESS 0
 #define REQ_PHASE_SELINUX 1
 
@@ -616,37 +633,6 @@ static void be_reset_offline(struct be_ctx *ctx)
     be_run_online_cb(ctx);
 }
 
-static char *dp_pam_err_to_string(TALLOC_CTX *memctx, int dp_err_type, int errnum)
-{
-    switch (dp_err_type) {
-    case DP_ERR_OK:
-        return talloc_asprintf(memctx, "Success (%s)",
-                               pam_strerror(NULL, errnum));
-        break;
-
-    case DP_ERR_OFFLINE:
-        return talloc_asprintf(memctx,
-                               "Provider is Offline (%s)",
-                               pam_strerror(NULL, errnum));
-        break;
-
-    case DP_ERR_TIMEOUT:
-        return talloc_asprintf(memctx,
-                               "Request timed out (%s)",
-                               pam_strerror(NULL, errnum));
-        break;
-
-    case DP_ERR_FATAL:
-    default:
-        return talloc_asprintf(memctx,
-                               "Internal Error (%s)",
-                               pam_strerror(NULL, errnum));
-        break;
-    }
-
-    return NULL;
-}
-
 static void get_subdomains_callback(struct be_req *req,
                                     int dp_err_type,
                                     int errnum,
@@ -659,7 +645,7 @@ static void get_subdomains_callback(struct be_req *req,
 
     DEBUG(SSSDBG_TRACE_FUNC, "Backend returned: (%d, %d, %s) [%s]\n",
               dp_err_type, errnum, errstr?errstr:"<NULL>",
-              dp_pam_err_to_string(req, dp_err_type, errnum));
+              dp_err_to_string(dp_err_type));
 
     be_queue_next_request(req, BET_SUBDOMAINS);
 
@@ -675,12 +661,7 @@ static void get_subdomains_callback(struct be_req *req,
         if (errstr) {
             err_msg = errstr;
         } else {
-            err_msg = dp_pam_err_to_string(req, dp_err_type, errnum);
-        }
-        if (!err_msg) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Failed to set err_msg, Out of memory?\n");
-            err_msg = "OOM";
+            err_msg = dp_err_to_string(dp_err_type);
         }
 
         sbus_request_return_and_finish(dbus_req,
@@ -819,12 +800,7 @@ static void acctinfo_callback(struct be_req *req,
         if (errstr) {
             err_msg = errstr;
         } else {
-            err_msg = dp_pam_err_to_string(req, dp_err_type, errnum);
-        }
-        if (!err_msg) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Failed to set err_msg, Out of memory?\n");
-            err_msg = "OOM";
+            err_msg = dp_err_to_string(dp_err_type);
         }
 
         sbus_request_return_and_finish(dbus_req,
@@ -1365,7 +1341,7 @@ static void be_pam_handler_callback(struct be_req *req,
 
     DEBUG(SSSDBG_CONF_SETTINGS, "Backend returned: (%d, %d, %s) [%s]\n",
               dp_err_type, errnum, errstr?errstr:"<NULL>",
-              dp_pam_err_to_string(req, dp_err_type, errnum));
+              dp_err_to_string(dp_err_type));
 
     pd = talloc_get_type(be_req_get_data(req), struct pam_data);
 
@@ -1918,12 +1894,7 @@ static void be_autofs_handler_callback(struct be_req *req,
         if (errstr) {
             err_msg = errstr;
         } else {
-            err_msg = dp_pam_err_to_string(req, dp_err_type, errnum);
-        }
-        if (!err_msg) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  "Failed to set err_msg, Out of memory?\n");
-            err_msg = "OOM";
+            err_msg = dp_err_to_string(dp_err_type);
         }
 
         sbus_request_return_and_finish(dbus_req,
@@ -2211,7 +2182,7 @@ static void check_online_callback(struct be_req *req, int dp_err_type,
 
     DEBUG(SSSDBG_CONF_SETTINGS, "Backend returned: (%d, %d, %s) [%s]\n",
               dp_err_type, errnum, errstr?errstr:"<NULL>",
-              dp_pam_err_to_string(req, dp_err_type, errnum));
+              dp_err_to_string(dp_err_type));
 
     req->be_ctx->check_online_ref_count--;
 
-- 
2.4.3

-------------- next part --------------
>From 7efcdf55f881ecba2159431651f25aa55ee52240 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 6 Nov 2015 15:45:19 +0100
Subject: [PATCH 2/3] DP: Check callback messages for valid UTF-8

https://fedorahosted.org/sssd/ticket/2861

Messages passed from Data Provider to responder must be valid UTF-8
strings. Because providers might not be completely under our control,
we need to check if the messages we receive are valid UTF-8 and if they
are not, use a fallback.
---
 src/providers/data_provider_be.c | 46 ++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 5f299a7209d3d1f1e83c625ae9db6d731a5cf5ab..a9b33b5c6a047f21ebc779fd6a73a109ceadec9e 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -37,6 +37,7 @@
 #include <security/pam_modules.h>
 
 #include "util/util.h"
+#include "util/sss_utf8.h"
 #include "confdb/confdb.h"
 #include "db/sysdb.h"
 #include "sbus/sssd_dbus.h"
@@ -132,6 +133,28 @@ static const char *dp_err_to_string(int dp_err_type)
     return "Unknown Error";
 }
 
+static const char *safe_be_req_err_msg(const char *msg_in,
+                                       int dp_err_type)
+{
+    bool ok;
+
+    if (msg_in == NULL) {
+        /* No custom error, just use default */
+        return dp_err_to_string(dp_err_type);
+    }
+
+    ok = sss_utf8_check((const uint8_t *) msg_in,
+                        strlen(msg_in));
+    if (!ok) {
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              "Back end message [%s] contains invalid non-UTF8 character, " \
+              "using default\n", msg_in);
+        return dp_err_to_string(dp_err_type);
+    }
+
+    return msg_in;
+}
+
 #define REQ_PHASE_ACCESS 0
 #define REQ_PHASE_SELINUX 1
 
@@ -658,11 +681,7 @@ static void get_subdomains_callback(struct be_req *req,
          */
         err_maj = dp_err_type;
         err_min = errnum;
-        if (errstr) {
-            err_msg = errstr;
-        } else {
-            err_msg = dp_err_to_string(dp_err_type);
-        }
+        err_msg = safe_be_req_err_msg(errstr, dp_err_type);
 
         sbus_request_return_and_finish(dbus_req,
                                        DBUS_TYPE_UINT16, &err_maj,
@@ -797,11 +816,7 @@ static void acctinfo_callback(struct be_req *req,
 
         err_maj = dp_err_type;
         err_min = errnum;
-        if (errstr) {
-            err_msg = errstr;
-        } else {
-            err_msg = dp_err_to_string(dp_err_type);
-        }
+        err_msg = safe_be_req_err_msg(errstr, dp_err_type);
 
         sbus_request_return_and_finish(dbus_req,
                                        DBUS_TYPE_UINT16, &err_maj,
@@ -1547,10 +1562,13 @@ static void be_sudo_handler_callback(struct be_req *req,
                                      int dp_ret,
                                      const char *errstr)
 {
+    const char *err_msg = NULL;
     struct sbus_request *dbus_req;
+
     dbus_req = (struct sbus_request *)(req->pvt);
 
-    be_sudo_handler_reply(dbus_req, dp_err, dp_ret, errstr);
+    err_msg = safe_be_req_err_msg(errstr, dp_err);
+    be_sudo_handler_reply(dbus_req, dp_err, dp_ret, err_msg);
 
     talloc_free(req);
 }
@@ -1891,11 +1909,7 @@ static void be_autofs_handler_callback(struct be_req *req,
 
         err_maj = dp_err_type;
         err_min = errnum;
-        if (errstr) {
-            err_msg = errstr;
-        } else {
-            err_msg = dp_err_to_string(dp_err_type);
-        }
+        err_msg = safe_be_req_err_msg(errstr, dp_err_type);
 
         sbus_request_return_and_finish(dbus_req,
                                        DBUS_TYPE_UINT16, &err_maj,
-- 
2.4.3

-------------- next part --------------
>From 6a608ef46eedc005adba0e9c1ec21d5d1b9bd5d1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
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 <dbus/dbus.h> ])
+
     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..db0a23fb71569e7ab3a7077d714bb60e060af9b0 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,
+                         "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);
 
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 \
         "<!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);
 
-- 
2.4.3



More information about the sssd-devel mailing list