[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