[SSSD] [PATCH] Guard against invalid DP messages
Jakub Hrozek
jhrozek at redhat.com
Fri Nov 6 15:26:37 UTC 2015
Hi,
the two attached patches fix
https://fedorahosted.org/sssd/ticket/2861
The first removes dp_pam_err_to_string() completely. It was wrong to use
function that calls pam_strerror() for non-PAM callbacks and even worse
that the PAM callback itself didn't call dp_pam_err_to_string..
The second guards against similar mistakes in the future by checking for
valid UTF-8 string. I think we should do this because technically there
can be a 3rd party provider.
I also have another patch that reduced code duplication -- because I
suspect that copy-pasting was the original reason this bug was
introduced. But I need to test the patches more, because they touch all
callbacks..
-------------- 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/2] 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/2] 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
More information about the sssd-devel
mailing list