[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