[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