From 19b5291579c7f3ac8c1a3f04c95dce828aa247c4 Mon Sep 17 00:00:00 2001
From: "Mark A. Grondona" <mark.grondona@gmail.com>
Date: Sun, 29 Mar 2020 15:41:39 +0000
Subject: [PATCH 1/3] shell: do not allow multiuser access to shell services by
 default

Problem: Shell services registered by flux_shell_service_register()
are "open to all" by default, and require a call to shell_svc_allowed()
in each msg handler in order to secure the service against multi-user
access. However, this design requires repetitive calls in every service
message handler, and makes it more likely that plugins install insecure
services when this call is forgotten. Furthermore, shell_svc_allowed()
is not even exported publicly in the shell.h API, so it is impossible
to create secure services via external shell plugins.

Internally wrap all message handlers installed by shell_svc_register()
and call shell_svc_allowed() so that all services are secure by default.

If a use case ever arises that requires multiuser access to a shell
service, then a separate api call can be created to export an
insecure service, so that the security of the service is explicit.

Fixes #2876
---
 src/shell/shell.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/src/shell/shell.c b/src/shell/shell.c
index aad738db9d77..ee08aa7d2185 100644
--- a/src/shell/shell.c
+++ b/src/shell/shell.c
@@ -516,16 +516,66 @@ int flux_shell_add_event_handler (flux_shell_t *shell,
     return 0;
 }
 
+struct service_wrap_arg
+{
+    flux_shell_t *shell;
+    flux_msg_handler_f cb;
+    void *arg;
+};
+
+static void shell_service_wrap (flux_t *h,
+                                flux_msg_handler_t *mh,
+                                const flux_msg_t *msg,
+                                void *arg)
+{
+    struct service_wrap_arg *sarg = arg;
+
+    if (shell_svc_allowed (sarg->shell->svc, msg) < 0)
+        goto error;
+    (*sarg->cb) (h, mh, msg, sarg->arg);
+    return;
+error:
+    if (flux_respond_error (h, msg, errno, NULL) < 0)
+        shell_log_errno ("flux_respond");
+}
+
+static struct service_wrap_arg *
+service_wrap_arg_create (flux_shell_t *shell,
+                        flux_msg_handler_f cb,
+                        void *arg)
+{
+    struct service_wrap_arg *sarg = calloc (1, sizeof (*sarg));
+    if (!sarg)
+        return NULL;
+    sarg->shell = shell;
+    sarg->cb = cb;
+    sarg->arg = arg;
+    return sarg;
+}
+
 int flux_shell_service_register (flux_shell_t *shell,
                                  const char *method,
                                  flux_msg_handler_f cb,
                                  void *arg)
 {
+    struct service_wrap_arg *sarg = NULL;
+
     if (!shell || !method || !cb) {
         errno = EINVAL;
         return -1;
     }
-    return shell_svc_register (shell->svc, method, cb, arg);
+    if (!(sarg = service_wrap_arg_create (shell, cb, arg)))
+        return -1;
+
+    if (flux_shell_aux_set (shell, NULL, sarg, free) < 0) {
+        free (sarg);
+        return -1;
+    }
+
+    return shell_svc_register (shell->svc,
+                               method,
+                               shell_service_wrap,
+                               sarg);
 }
 
 flux_future_t *flux_shell_rpc_pack (flux_shell_t *shell,

From f74033ee332e45999fab9f0de09d1997eafe8558 Mon Sep 17 00:00:00 2001
From: "Mark A. Grondona" <mark.grondona@gmail.com>
Date: Sun, 29 Mar 2020 15:53:08 +0000
Subject: [PATCH 2/3] shell: remove shell_svc_allowed from service callbacks

Since shell services registered by flux_shell_service_register()
are now secure by default, remove redundant calls to shell_svc_allowed()
in message handlers in the input and output services.
---
 src/shell/input.c  | 2 --
 src/shell/output.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/src/shell/input.c b/src/shell/input.c
index fe88cc4206ce..a2eab665154d 100644
--- a/src/shell/input.c
+++ b/src/shell/input.c
@@ -167,8 +167,6 @@ static void shell_input_stdin_cb (flux_t *h,
     bool eof = false;
     json_t *o;
 
-    if (shell_svc_allowed (in->shell->svc, msg) < 0)
-        goto error;
     if (flux_request_unpack (msg, NULL, "o", &o) < 0)
         goto error;
     if (iodecode (o, NULL, NULL, NULL, NULL, &eof) < 0)
diff --git a/src/shell/output.c b/src/shell/output.c
index c0e9594333e4..5dde7d1ca9ea 100644
--- a/src/shell/output.c
+++ b/src/shell/output.c
@@ -403,8 +403,6 @@ static void shell_output_write_cb (flux_t *h,
     json_t *o;
     json_t *entry;
 
-    if (shell_svc_allowed (out->shell->svc, msg) < 0)
-        goto error;
     if (flux_request_unpack (msg, NULL, "o", &o) < 0)
         goto error;
     if (iodecode (o, NULL, NULL, NULL, NULL, &eof) < 0)

From d31233698ccaacf62f8d963c9303a067a28b764b Mon Sep 17 00:00:00 2001
From: "Mark A. Grondona" <mark.grondona@gmail.com>
Date: Sun, 29 Mar 2020 15:54:35 +0000
Subject: [PATCH 3/3] testsuite: add check for proctable service security

Add test to ensure proctable shell service is not accessible to
all users in t2610-job-shell-mpir.t.
---
 t/t2610-job-shell-mpir.t | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t2610-job-shell-mpir.t b/t/t2610-job-shell-mpir.t
index a0bd0a91441b..c3088d45235b 100755
--- a/t/t2610-job-shell-mpir.t
+++ b/t/t2610-job-shell-mpir.t
@@ -34,4 +34,21 @@ for test in 1:1 2:2 2:4 4:4 4:8 4:7; do
         flux job attach ${id}
     '
 done
+
+
+test_expect_success 'flux-shell: test security of proctable method' '
+    id=$(flux mini submit -o stop-tasks-in-exec /bin/true) &&
+    flux job wait-event -vt 5 -p guest.exec.eventlog \
+        -m sync=true ${id} shell.start &&
+    shell_rank=$(shell_leader_rank $id) &&
+    shell_service=$(shell_service $id) &&
+    ( export FLUX_HANDLE_USERID=9999 &&
+      export FLUX_HANDLE_ROLEMASK=0x2 &&
+      test_expect_code 1 ${mpir} $shell_rank $shell_service
+    ) &&
+    ${mpir} $(shell_leader_rank $id) $(shell_service $id) &&
+    flux job kill -s CONT ${id} &&
+    flux job attach ${id}
+'
+
 test_done