[SSSD] [PATCH] Guard against invalid DP messages

Jakub Hrozek jhrozek at redhat.com
Wed Nov 11 11:00:31 UTC 2015


On Mon, Nov 09, 2015 at 03:14:31PM +0100, Jakub Hrozek wrote:
> On Mon, Nov 09, 2015 at 12:44:17PM +0100, Sumit Bose wrote:
> > On Fri, Nov 06, 2015 at 09:32:23PM +0100, Jakub Hrozek wrote:
> > > On Fri, Nov 06, 2015 at 09:13:07PM +0100, Jakub Hrozek wrote:
> > > > On Fri, Nov 06, 2015 at 04:59:58PM +0100, Sumit Bose wrote:
> > > > > > @@ -658,11 +678,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);
> > > > > 
> > > > > Wouldn't it be even more safe to do this check in
> > > > > sbus_request_return_and_finish() and not rely on the callers?
> > > > 
> > > > Something like the attached patch? I think we should check in
> > > > sbus_request_return_and_finish() in addition to checking in the callers,
> > > > not instead, because the string in the callback is just a diagnostics
> > > > message and we can recover by using some fallback.
> > > > 
> > > > If we get all the way to sbus_request_return_and_finish(), then failing
> > > > gracefully is better then abort() that libdbus calls, but it's even better
> > > > to finish the request with wrong error message then fail completely.
> > > > 
> > > > (I also still dislike that libdbus aborts...)
> > > 
> > > Also, self-nack. dbus_validate_utf8() is not available on RHEL-6:
> > >     http://sssd-ci.duckdns.org/logs/job/32/17/rhel6/ci-build-debug/ci-make-tests.log
> > > 
> > > I guess we can limit these checks to new OSs or use sss_utf8_check() on
> > > older platforms..
> > 
> > Why not always use sss_utf8_check()? Is there an advantage using
> > dbus_validate_utf8()?
> 
> I don't know how dbus_validate_utf8() works internally, so I tried to
> stick to the dbus-provided version to check dbus types.
> 
> But if both do the same thing (If they don't it would be a bug, sure),
> we can stick to sss_utf8_check()
> 
> (Another advantage might be that we could get away with linking against
> libdbus only)

Hi,

attached are patches that only use sss_utf8_check() and work on RHEL-6
also.
-------------- next part --------------
>From ddd47ebc30316ca988456ec6c1bed68cb157256a 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 a6924296d3c01a11ba3785dcb4af4c45569dbb4b 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 | 45 ++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 5f299a7209d3d1f1e83c625ae9db6d731a5cf5ab..562a15a2d156b4b062acbf1f4e44e4cb7a4058d2 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,27 @@ 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 is invalid, using default\n");
+        return dp_err_to_string(dp_err_type);
+    }
+
+    return msg_in;
+}
+
 #define REQ_PHASE_ACCESS 0
 #define REQ_PHASE_SELINUX 1
 
@@ -658,11 +680,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 +815,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 +1561,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 +1908,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 a22056cc25727a998683106a3137a985da58fc29 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 | 45 +++++++++++++++++++++++++++++++++++++++-
 src/tests/sbus_tests.c       | 49 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 51c0b0da8a7cfb692eabf38a71e42fbb0e40b4b0..c7265f262b17cd95a4156128ea03b122001e60b1 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..9a116eff288daf7b81c273920664d5fd36102286 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,65 @@ 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_CRIT_FAILURE,
+                       "String argument is not valid UTF-8\n");
+                 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