On Fri, Nov 13, 2015 at 01:40:16PM +0100, Sumit Bose wrote: > > Hi, > > > > attached are patches that only use sss_utf8_check() and work on RHEL-6 > > also. > > The patches look good and fixing the issue as expected. CI passes as > well http://sssd-ci.duckdns.org/logs/job/33/16/summary.html (the RHEL6 > failure is unrelated). ACK > > I just have a minor comment > > > > > +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"); > > Maybe something like > > DEBUG(SSSDBG_MINOR_FAILURE, > "Back end message [%s] contains invalid non-UTF8 character, " \ > "using default\n", msg_in); > > would be more specific and would help to better identify the source of the > message as well? Similar for sbus_request_valist_check(). > > But to fix the issue at hand and not to delay the release this can of > course be added later as well.
No, I think it's important to have good debug messages. I used the one you suggested, there are no other changes in the patches.
>From 9f50257515cdb0e839fa31130dbfa5c5454225d7 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@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
>From 7efcdf55f881ecba2159431651f25aa55ee52240 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@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
>From 4af6d213e2e5996b87b8ed15c1d844fa5c620b66 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <jhro...@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..114e7a4095d51a792d1d5173682149c8699d457a 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, + "Back end message [%s] contains invalid non-UTF8 " \ + "characters"); + 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
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel