From 3aa33bcc9c70d197b5ed0760b12d65dfab4d4da5 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 9 Oct 2020 09:56:03 -0500

Subject: [PATCH 1/7] Log: executor: show CRM_OP_REGISTER rc in debug message

Previously, process_lrmd_signon() would add the rc to the client reply

but not pass it back to process_lrmd_message(), which would always log "OK" in

its debug message, even if the sign-on was rejected.

---

 daemons/execd/execd_commands.c | 21 +++++++++++----------

 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/daemons/execd/execd_commands.c b/daemons/execd/execd_commands.c

index 4d0e457..8487dd4 100644

--- a/daemons/execd/execd_commands.c

+++ b/daemons/execd/execd_commands.c

@@ -1494,10 +1494,10 @@ free_rsc(gpointer data)

     free(rsc);

 }

-static xmlNode *

-process_lrmd_signon(pcmk__client_t *client, xmlNode *request, int call_id)

+static int

+process_lrmd_signon(pcmk__client_t *client, xmlNode *request, int call_id,

+                    xmlNode **reply)

 {

-    xmlNode *reply = NULL;

     int rc = pcmk_ok;

     const char *is_ipc_provider = crm_element_value(request, F_LRMD_IS_IPC_PROVIDER);

     const char *protocol_version = crm_element_value(request, F_LRMD_PROTOCOL_VERSION);

@@ -1508,18 +1508,19 @@ process_lrmd_signon(pcmk__client_t *client, xmlNode *request, int call_id)

         rc = -EPROTO;

     }

-    reply = create_lrmd_reply(__FUNCTION__, rc, call_id);

-    crm_xml_add(reply, F_LRMD_OPERATION, CRM_OP_REGISTER);

-    crm_xml_add(reply, F_LRMD_CLIENTID, client->id);

-    crm_xml_add(reply, F_LRMD_PROTOCOL_VERSION, LRMD_PROTOCOL_VERSION);

-

     if (crm_is_true(is_ipc_provider)) {

         // This is a remote connection from a cluster node's controller

 #ifdef SUPPORT_REMOTE

         ipc_proxy_add_provider(client);

 #endif

     }

-    return reply;

+

+    *reply = create_lrmd_reply(__func__, rc, call_id);

+    crm_xml_add(*reply, F_LRMD_OPERATION, CRM_OP_REGISTER);

+    crm_xml_add(*reply, F_LRMD_CLIENTID, client->id);

+    crm_xml_add(*reply, F_LRMD_PROTOCOL_VERSION, LRMD_PROTOCOL_VERSION);

+

+    return rc;

 }

 static int

@@ -1832,7 +1833,7 @@ process_lrmd_message(pcmk__client_t *client, uint32_t id, xmlNode *request)

 #endif

         do_reply = 1;

     } else if (crm_str_eq(op, CRM_OP_REGISTER, TRUE)) {

-        reply = process_lrmd_signon(client, request, call_id);

+        rc = process_lrmd_signon(client, request, call_id, &reply);

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_RSC_REG, TRUE)) {

         rc = process_lrmd_rsc_register(client, id, request);

-- 

1.8.3.1

From d0002343faa4595e42b790119b7f3037db1130c4 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 9 Oct 2020 15:16:39 -0500

Subject: [PATCH 2/7] Low: executor: mark controller connections to

 pacemaker-remoted as privileged

Previously, pcmk__client_privileged was only set when local clients connected

(as root or hacluster). Now, set it when pacemaker-remoted successfully

completes the TLS handshake with a remote client (i.e., the controller on a

cluster node).

This has no effect as of this commit but will with later commits.

---

 daemons/execd/remoted_tls.c | 5 +++++

 1 file changed, 5 insertions(+)

diff --git a/daemons/execd/remoted_tls.c b/daemons/execd/remoted_tls.c

index 1a1f8b2..c835549 100644

--- a/daemons/execd/remoted_tls.c

+++ b/daemons/execd/remoted_tls.c

@@ -72,6 +72,11 @@ remoted__read_handshake_data(pcmk__client_t *client)

     client->remote->tls_handshake_complete = TRUE;

     crm_notice("Remote client connection accepted");

+    /* Only a client with access to the TLS key can connect, so we can treat

+     * it as privileged.

+     */

+    set_bit(client->flags, pcmk__client_privileged);

+

     // Alert other clients of the new connection

     notify_of_new_client(client);

     return 0;

-- 

1.8.3.1

From 3db100d775aee214fff8f54eae0076a5fcc41c56 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Thu, 15 Oct 2020 15:33:13 -0500

Subject: [PATCH 3/7] Low: executor: return appropriate error code when no

 remote support

---

 daemons/execd/execd_commands.c | 6 +++++-

 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/daemons/execd/execd_commands.c b/daemons/execd/execd_commands.c

index 8487dd4..41c8169 100644

--- a/daemons/execd/execd_commands.c

+++ b/daemons/execd/execd_commands.c

@@ -1509,9 +1509,11 @@ process_lrmd_signon(pcmk__client_t *client, xmlNode *request, int call_id,

     }

     if (crm_is_true(is_ipc_provider)) {

-        // This is a remote connection from a cluster node's controller

 #ifdef SUPPORT_REMOTE

+        // This is a remote connection from a cluster node's controller

         ipc_proxy_add_provider(client);

+#else

+        rc = -EPROTONOSUPPORT;

 #endif

     }

@@ -1830,6 +1832,8 @@ process_lrmd_message(pcmk__client_t *client, uint32_t id, xmlNode *request)

     if (crm_str_eq(op, CRM_OP_IPC_FWD, TRUE)) {

 #ifdef SUPPORT_REMOTE

         ipc_proxy_forward_client(client, request);

+#else

+        rc = -EPROTONOSUPPORT;

 #endif

         do_reply = 1;

     } else if (crm_str_eq(op, CRM_OP_REGISTER, TRUE)) {

-- 

1.8.3.1

From f273f1c16f21ff96983797ed5ceb2978dafe545a Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Thu, 15 Oct 2020 15:33:57 -0500

Subject: [PATCH 4/7] High: executor: restrict certain IPC requests to

 Pacemaker daemons

The executor IPC API allows clients to register resources, request agent

execution, and so forth.

If ACLs are enabled, this could allow an ACL-restricted user to bypass ACLs and

execute any code as root. (If ACLs are not enabled, users in the haclient group

have full access to the CIB, which already gives them that ability, so there is

no additional exposure in that case.)

When ACLs are supported, this commit effectively disables the executor IPC API

for clients that aren't connecting as root or hacluster. Such clients can only

register and poke now.

---

 daemons/execd/execd_commands.c | 91 +++++++++++++++++++++++++++++++++---------

 1 file changed, 73 insertions(+), 18 deletions(-)

diff --git a/daemons/execd/execd_commands.c b/daemons/execd/execd_commands.c

index 41c8169..207eb6a 100644

--- a/daemons/execd/execd_commands.c

+++ b/daemons/execd/execd_commands.c

@@ -1510,8 +1510,12 @@ process_lrmd_signon(pcmk__client_t *client, xmlNode *request, int call_id,

     if (crm_is_true(is_ipc_provider)) {

 #ifdef SUPPORT_REMOTE

-        // This is a remote connection from a cluster node's controller

-        ipc_proxy_add_provider(client);

+        if ((client->remote != NULL) && client->remote->tls_handshake_complete) {

+            // This is a remote connection from a cluster node's controller

+            ipc_proxy_add_provider(client);

+        } else {

+            rc = -EACCES;

+        }

 #else

         rc = -EPROTONOSUPPORT;

 #endif

@@ -1826,12 +1830,26 @@ process_lrmd_message(pcmk__client_t *client, uint32_t id, xmlNode *request)

     int do_notify = 0;

     xmlNode *reply = NULL;

+    bool allowed = true;

+

+#if ENABLE_ACL

+    /* Certain IPC commands may be done only by privileged users (i.e. root or

+     * hacluster) when ACLs are enabled, because they would otherwise provide a

+     * means of bypassing ACLs.

+     */

+    allowed = is_set(client->flags, pcmk__client_privileged);

+#endif

+

     crm_trace("Processing %s operation from %s", op, client->id);

     crm_element_value_int(request, F_LRMD_CALLID, &call_id);

     if (crm_str_eq(op, CRM_OP_IPC_FWD, TRUE)) {

 #ifdef SUPPORT_REMOTE

-        ipc_proxy_forward_client(client, request);

+        if (allowed) {

+            ipc_proxy_forward_client(client, request);

+        } else {

+            rc = -EACCES;

+        }

 #else

         rc = -EPROTONOSUPPORT;

 #endif

@@ -1840,38 +1858,70 @@ process_lrmd_message(pcmk__client_t *client, uint32_t id, xmlNode *request)

         rc = process_lrmd_signon(client, request, call_id, &reply);

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_RSC_REG, TRUE)) {

-        rc = process_lrmd_rsc_register(client, id, request);

-        do_notify = 1;

+        if (allowed) {

+            rc = process_lrmd_rsc_register(client, id, request);

+            do_notify = 1;

+        } else {

+            rc = -EACCES;

+        }

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_RSC_INFO, TRUE)) {

-        reply = process_lrmd_get_rsc_info(request, call_id);

+        if (allowed) {

+            reply = process_lrmd_get_rsc_info(request, call_id);

+        } else {

+            rc = -EACCES;

+        }

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_RSC_UNREG, TRUE)) {

-        rc = process_lrmd_rsc_unregister(client, id, request);

-        /* don't notify anyone about failed un-registers */

-        if (rc == pcmk_ok || rc == -EINPROGRESS) {

-            do_notify = 1;

+        if (allowed) {

+            rc = process_lrmd_rsc_unregister(client, id, request);

+            /* don't notify anyone about failed un-registers */

+            if (rc == pcmk_ok || rc == -EINPROGRESS) {

+                do_notify = 1;

+            }

+        } else {

+            rc = -EACCES;

         }

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_RSC_EXEC, TRUE)) {

-        rc = process_lrmd_rsc_exec(client, id, request);

+        if (allowed) {

+            rc = process_lrmd_rsc_exec(client, id, request);

+        } else {

+            rc = -EACCES;

+        }

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_RSC_CANCEL, TRUE)) {

-        rc = process_lrmd_rsc_cancel(client, id, request);

+        if (allowed) {

+            rc = process_lrmd_rsc_cancel(client, id, request);

+        } else {

+            rc = -EACCES;

+        }

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_POKE, TRUE)) {

         do_notify = 1;

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_CHECK, TRUE)) {

-        xmlNode *data = get_message_xml(request, F_LRMD_CALLDATA); 

-        const char *timeout = crm_element_value(data, F_LRMD_WATCHDOG);

-        CRM_LOG_ASSERT(data != NULL);

-        pcmk__valid_sbd_timeout(timeout);

+        if (allowed) {

+            xmlNode *data = get_message_xml(request, F_LRMD_CALLDATA);

+

+            CRM_LOG_ASSERT(data != NULL);

+            pcmk__valid_sbd_timeout(crm_element_value(data, F_LRMD_WATCHDOG));

+        } else {

+            rc = -EACCES;

+        }

     } else if (crm_str_eq(op, LRMD_OP_ALERT_EXEC, TRUE)) {

-        rc = process_lrmd_alert_exec(client, id, request);

+        if (allowed) {

+            rc = process_lrmd_alert_exec(client, id, request);

+        } else {

+            rc = -EACCES;

+        }

         do_reply = 1;

     } else if (crm_str_eq(op, LRMD_OP_GET_RECURRING, TRUE)) {

-        reply = process_lrmd_get_recurring(request, call_id);

+        if (allowed) {

+            reply = process_lrmd_get_recurring(request, call_id);

+        } else {

+            rc = -EACCES;

+        }

         do_reply = 1;

     } else {

         rc = -EOPNOTSUPP;

@@ -1879,6 +1929,11 @@ process_lrmd_message(pcmk__client_t *client, uint32_t id, xmlNode *request)

         crm_err("Unknown IPC request '%s' from %s", op, client->name);

     }

+    if (rc == -EACCES) {

+        crm_warn("Rejecting IPC request '%s' from unprivileged client %s",

+                 op, pcmk__client_name(client));

+    }

+

     crm_debug("Processed %s operation from %s: rc=%d, reply=%d, notify=%d",

               op, client->id, rc, do_reply, do_notify);

-- 

1.8.3.1

From f13759f6971402dac3bea1aac45214a84d838728 Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 9 Oct 2020 11:16:43 -0500

Subject: [PATCH 5/7] Low: pacemakerd: check client for NULL before using it

... to guard against bugs in client tracking

---

 daemons/pacemakerd/pacemakerd.c | 5 ++++-

 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemons/pacemakerd/pacemakerd.c b/daemons/pacemakerd/pacemakerd.c

index 5ed4626..573ea5a 100644

--- a/daemons/pacemakerd/pacemakerd.c

+++ b/daemons/pacemakerd/pacemakerd.c

@@ -553,9 +553,12 @@ pcmk_ipc_dispatch(qb_ipcs_connection_t * qbc, void *data, size_t size)

     uint32_t id = 0;

     uint32_t flags = 0;

     const char *task = NULL;

+    xmlNode *msg = NULL;

     pcmk__client_t *c = pcmk__find_client(qbc);

-    xmlNode *msg = pcmk__client_data2xml(c, data, &id, &flags);

+    CRM_CHECK(c != NULL, return 0);

+

+    msg = pcmk__client_data2xml(c, data, &id, &flags);

     pcmk__ipc_send_ack(c, id, flags, "ack");

     if (msg == NULL) {

         return 0;

-- 

1.8.3.1

From 021081c1e28b254a0f68143fa55e517f0fcc4edb Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 9 Oct 2020 11:17:18 -0500

Subject: [PATCH 6/7] High: pacemakerd: ignore shutdown requests from

 unprivileged users

The pacemakerd IPC API supports a shutdown request, along with a

command-line interface for using it (pacemakerd --shutdown).

Only the haclient group has access to the IPC. Without ACLs, that group can

already shut down Pacemaker via the CIB, so there's no security implication.

However, it might not be desired to allow ACL-restricted users to shut down

Pacemaker, so block users other than root or hacluster if ACLs are supported.

---

 daemons/pacemakerd/pacemakerd.c | 24 ++++++++++++++++++++----

 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/daemons/pacemakerd/pacemakerd.c b/daemons/pacemakerd/pacemakerd.c

index 573ea5a..2e69bd1 100644

--- a/daemons/pacemakerd/pacemakerd.c

+++ b/daemons/pacemakerd/pacemakerd.c

@@ -566,9 +566,26 @@ pcmk_ipc_dispatch(qb_ipcs_connection_t * qbc, void *data, size_t size)

     task = crm_element_value(msg, F_CRM_TASK);

     if (crm_str_eq(task, CRM_OP_QUIT, TRUE)) {

-        crm_notice("Shutting down in response to IPC request %s from %s",

-                   crm_element_value(msg, F_CRM_REFERENCE), crm_element_value(msg, F_CRM_ORIGIN));

-        pcmk_shutdown(15);

+        bool allowed = true;

+

+#if ENABLE_ACL

+        /* Only allow privileged users (i.e. root or hacluster)

+         * to shut down Pacemaker from the command line (or direct IPC).

+         *

+         * We only check when ACLs are enabled, because without them, any client

+         * with IPC access could shut down Pacemaker via the CIB anyway.

+         */

+        allowed = is_set(c->flags, pcmk__client_privileged);

+#endif

+        if (allowed) {

+            crm_notice("Shutting down in response to IPC request %s from %s",

+                       crm_element_value(msg, F_CRM_REFERENCE),

+                       crm_element_value(msg, F_CRM_ORIGIN));

+            pcmk_shutdown(15);

+        } else {

+            crm_warn("Ignoring shutdown request from unprivileged client %s",

+                     pcmk__client_name(c));

+        }

     } else if (crm_str_eq(task, CRM_OP_RM_NODE_CACHE, TRUE)) {

         crm_trace("Ignoring IPC request to purge node "

-- 

1.8.3.1

From 80eb5ddfd529be02214f38669f1b177535186fbc Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Fri, 9 Oct 2020 11:55:26 -0500

Subject: [PATCH 7/7] Fix: fencer: restrict certain IPC requests to privileged

 users

The fencer IPC API allows clients to register fence devices.

If ACLs are enabled, this could allow an ACL-restricted user to bypass ACLs to

configure fencing. If the user is able to install executables to the standard

fencing agent locations, have arbitrary code executed as root (the standard

locations generally require root for write access, so that is unlikely to be an

issue).

If ACLs are not enabled, users in the haclient group have full access to the

CIB, which already gives them these capabilities, so there is no additional

exposure in that case.

This commit does not restrict unprivileged users from using other fencing API,

such as requesting actual fencing.

---

 daemons/fenced/fenced_commands.c | 41 ++++++++++++++++++++++++++++++++++++----

 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/daemons/fenced/fenced_commands.c b/daemons/fenced/fenced_commands.c

index 859e7b7..a8c90a6 100644

--- a/daemons/fenced/fenced_commands.c

+++ b/daemons/fenced/fenced_commands.c

@@ -2531,6 +2531,18 @@ handle_request(pcmk__client_t *client, uint32_t id, uint32_t flags,

     const char *op = crm_element_value(request, F_STONITH_OPERATION);

     const char *client_id = crm_element_value(request, F_STONITH_CLIENTID);

+    bool allowed = true;

+

+#if ENABLE_ACL

+    /* IPC commands related to fencing configuration may be done only by

+     * privileged users (i.e. root or hacluster) when ACLs are supported,

+     * because all other users should go through the CIB to have ACLs applied.

+     */

+    if (client != NULL) {

+        allowed = is_set(client->flags, pcmk__client_privileged);

+    }

+#endif

+

     crm_element_value_int(request, F_STONITH_CALLOPTS, &call_options);

     if (is_set(call_options, st_opt_sync_call)) {

@@ -2687,27 +2699,43 @@ handle_request(pcmk__client_t *client, uint32_t id, uint32_t flags,

     } else if (crm_str_eq(op, STONITH_OP_DEVICE_ADD, TRUE)) {

         const char *device_id = NULL;

-        rc = stonith_device_register(request, &device_id, FALSE);

+        if (allowed) {

+            rc = stonith_device_register(request, &device_id, FALSE);

+        } else {

+            rc = -EACCES;

+        }

         do_stonith_notify_device(call_options, op, rc, device_id);

     } else if (crm_str_eq(op, STONITH_OP_DEVICE_DEL, TRUE)) {

         xmlNode *dev = get_xpath_object("//" F_STONITH_DEVICE, request, LOG_ERR);

         const char *device_id = crm_element_value(dev, XML_ATTR_ID);

-        rc = stonith_device_remove(device_id, FALSE);

+        if (allowed) {

+            rc = stonith_device_remove(device_id, FALSE);

+        } else {

+            rc = -EACCES;

+        }

         do_stonith_notify_device(call_options, op, rc, device_id);

     } else if (crm_str_eq(op, STONITH_OP_LEVEL_ADD, TRUE)) {

         char *device_id = NULL;

-        rc = stonith_level_register(request, &device_id);

+        if (allowed) {

+            rc = stonith_level_register(request, &device_id);

+        } else {

+            rc = -EACCES;

+        }

         do_stonith_notify_level(call_options, op, rc, device_id);

         free(device_id);

     } else if (crm_str_eq(op, STONITH_OP_LEVEL_DEL, TRUE)) {

         char *device_id = NULL;

-        rc = stonith_level_remove(request, &device_id);

+        if (allowed) {

+            rc = stonith_level_remove(request, &device_id);

+        } else {

+            rc = -EACCES;

+        }

         do_stonith_notify_level(call_options, op, rc, device_id);

     } else if(safe_str_eq(op, CRM_OP_RM_NODE_CACHE)) {

@@ -2727,6 +2755,11 @@ handle_request(pcmk__client_t *client, uint32_t id, uint32_t flags,

   done:

+    if (rc == -EACCES) {

+        crm_warn("Rejecting IPC request '%s' from unprivileged client %s",

+                 crm_str(op), pcmk__client_name(client));

+    }

+

     /* Always reply unless the request is in process still.

      * If in progress, a reply will happen async after the request

      * processing is finished */

-- 

1.8.3.1