[SSSD] [PATCH] Guard against invalid DP messages

Jakub Hrozek jhrozek at redhat.com
Fri Nov 6 20:13:07 UTC 2015


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...)
-------------- next part --------------
>From 6f02c852d22805eb0ab63c2e3237c79b33b36d94 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 effa185f905097e0be1e03596b5fc6da75a7e382..0cbfb0f5b541a17dc152c00e05a293c5ea0971e0 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,
@@ -2208,7 +2179,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 306000f8b72cbb2822a2ddcd8ce4743018651607 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 | 43 +++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 0cbfb0f5b541a17dc152c00e05a293c5ea0971e0..12b04864255101735b54f9ccc89b703b8e00dd75 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -132,6 +132,26 @@ 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)
+{
+    dbus_bool_t ok;
+
+    if (msg_in == NULL) {
+        /* No custom error, just use default */
+        return dp_err_to_string(dp_err_type);
+    }
+
+    ok = dbus_validate_utf8(msg_in, NULL);
+    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 +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);
 
         sbus_request_return_and_finish(dbus_req,
                                        DBUS_TYPE_UINT16, &err_maj,
@@ -797,11 +813,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 +1559,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 +1906,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 9913b487aeb4e01b6abff3b34a47314670faca5d 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.
---
 src/sbus/sssd_dbus_request.c | 40 ++++++++++++++++++++++++++++++++++++-
 src/tests/sbus_tests.c       | 47 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/src/sbus/sssd_dbus_request.c b/src/sbus/sssd_dbus_request.c
index 552bb5da86668f4e94c0fb7699753706a0698c43..d2f2bd5351475a9de89510f46e1a05e53ee533ee 100644
--- a/src/sbus/sssd_dbus_request.c
+++ b/src/sbus/sssd_dbus_request.c
@@ -96,15 +96,54 @@ 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;
+    int type;
+    va_list va_check;
+    const DBusBasicValue *value;
+    dbus_bool_t 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 = dbus_validate_utf8(value->str, NULL);
+             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);
+    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) {
         DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory allocating DBus message\n");
@@ -112,7 +151,6 @@ int sbus_request_return_and_finish(struct sbus_request *dbus_req,
         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..eb3b281b86d9f035191d661d652da3fb514fe615 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,31 @@ START_TEST(test_request_parse_bad_args)
 }
 END_TEST
 
+START_TEST(test_request_dontcrash)
+{
+    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);
+}
+END_TEST
+
 START_TEST(test_introspection)
 {
     TALLOC_CTX *ctx;
@@ -373,6 +419,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