SSSD / sssd

Clone

#3784 SUDO: Fix running in unprivileged responder
Merged 5 years ago by jhrozek. Opened 5 years ago by lslebodn.
SSSD/ lslebodn/sssd sudo_nopriv  into  master

file modified
+27 -4 
@@ -67,7 +67,8 @@ 
 

  

  int sudo_process_init(TALLOC_CTX *mem_ctx,
 

                        struct tevent_context *ev,
 

-                       struct confdb_ctx *cdb)
 

+                       struct confdb_ctx *cdb,
 

+                       int pipe_fd)
 

  {
 

      struct resp_ctx *rctx;
 

      struct sss_cmd_table *sudo_cmds;
 
@@ -79,8 +80,8 @@ 
 

      sudo_cmds = get_sudo_cmds();
 

      ret = sss_process_init(mem_ctx, ev, cdb,
 

                             sudo_cmds,
 

-                            NULL, -1,                   /* No public socket */
 

-                            SSS_SUDO_SOCKET_NAME, -1,   /* Private socket only */
 

+                            SSS_SUDO_SOCKET_NAME, pipe_fd,   /* custom permissions on socket */
 

+                            NULL, -1,                   /* No private socket */
 

                             CONFDB_SUDO_CONF_ENTRY,
 

                             SSS_SUDO_SBUS_SERVICE_NAME,
 

                             SSS_SUDO_SBUS_SERVICE_VERSION,
 
@@ -182,6 +183,7 @@ 
 

      char *opt_logger = NULL;
 

      struct main_context *main_ctx;
 

      int ret;
 

+     int pipe_fd = -1;
 

      uid_t uid;
 

      gid_t gid;
 

  
@@ -219,6 +221,27 @@ 
 

  

      sss_set_logger(opt_logger);
 

  

+     if (!is_socket_activated()) {
 

+         /* Create pipe file descriptors here with right ownerschip */
 

+         ret = create_pipe_fd(SSS_SUDO_SOCKET_NAME, &pipe_fd, SSS_DFL_UMASK);
 

+         if (ret != EOK) {
 

+             DEBUG(SSSDBG_FATAL_FAILURE,
 

+                   "create_pipe_fd failed [%d]: %s.\n",
 

+                   ret, sss_strerror(ret));
 

+             return 4;
 

+         }
 

+ 

+         ret = chown(SSS_SUDO_SOCKET_NAME, uid, 0);
 

+         if (ret != EOK) {
 

+             ret = errno;
 

+             close(pipe_fd);
 

+             DEBUG(SSSDBG_FATAL_FAILURE,
 

+                   "create_pipe_fd failed [%d]: %s.\n",
 

+                   ret, sss_strerror(ret));
 

+             return 5;
 

+         }
 

+     }
 

+ 

      ret = server_setup("sssd[sudo]", 0, uid, gid, CONFDB_SUDO_CONF_ENTRY,
 

                         &main_ctx);
 

      if (ret != EOK) {
 
@@ -234,7 +257,7 @@ 
 

  

      ret = sudo_process_init(main_ctx,
 

                              main_ctx->event_ctx,
 

-                             main_ctx->confdb_ctx);
 

+                             main_ctx->confdb_ctx, pipe_fd);
 

      if (ret != EOK) {
 

          return 3;
 

      }

file modified
+1 -2 
@@ -10,8 +10,7 @@ 
 

  ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r sudo
 

  ListenStream=@pipepath@/sudo
 

  SocketUser=@SSSD_USER@
 

- SocketGroup=@SSSD_USER@
 

- SocketMode=0600
 

+ SocketMode=0660
 

  

  [Install]
 

  WantedBy=sssd.service

There are strict checks for private sockets which does not work with
unprivileged responder

Resolves:
https://pagure.io/SSSD/sssd/issue/3778

rebased onto 4ce03c5c6f2d8ac2688f60d7af08b613de8780bb
5 years ago

Could you use SSS_DFL_UMASK instead here?

Could you use fchown here on the fd instead of chown to avoid e.g. Coverity complaining about TOCTOU issues?

Thank you for the patches. I only have two minor issues that I pointed out inline.

Sure, fixed

I had a fchown here but it did not work for me on f27.
It returned 0 but owner was not changed.

Would you mind to try? Maybe I dis some mistake on my side.

BTW feel free to squash following oneliner if it works for you

diff --git a/src/responder/sudo/sudosrv.c b/src/responder/sudo/sudosrv.c
index dd524bd0a..9c0163ca6 100644
--- a/src/responder/sudo/sudosrv.c
+++ b/src/responder/sudo/sudosrv.c
@@ -231,7 +231,7 @@ int main(int argc, const char *argv[])
             return 4;
         }

-        ret = chown(SSS_SUDO_SOCKET_NAME, uid, 0);
+        ret = fchown(pipe_fd, uid, 0);
         if (ret != EOK) {
             close(pipe_fd);
             DEBUG(SSSDBG_FATAL_FAILURE,

IMHO TOCTOU is irrelevant here because socket was already created with strict permissions due to umast 0117 and we are just "downgrading" permissions with chown()

rebased onto 89ef3d67e9f26e238556ed0c07e2a12ce377cc47
5 years ago

Hmm, it doesn't work for me either. And I don't see anything in the manual page that would tell me why. The only strange thing in strace is that the fd number is 0, so I wonder if libc has some optimization for the standard fds in fchown..

But if you agree, I would like to squash this change, because on failure, ret is always -1, but the error code is in errno.

diff --git a/src/responder/sudo/sudosrv.c b/src/responder/sudo/sudosrv.c
index dd524bd0a..6cb5347e7 100644
--- a/src/responder/sudo/sudosrv.c
+++ b/src/responder/sudo/sudosrv.c
@@ -232,8 +232,9 @@ int main(int argc, const char *argv[])
         }

         ret = chown(SSS_SUDO_SOCKET_NAME, uid, 0);
-        if (ret != EOK) {
+        if (ret != 0) {
+            ret = errno;
             close(pipe_fd);
             DEBUG(SSSDBG_FATAL_FAILURE,
                   "create_pipe_fd failed [%d]: %s.\n",
                   ret, sss_strerror(ret));

rebased onto a8a4fab
5 years ago

Hmm, it doesn't work for me either. And I don't see anything in the manual page that would tell me why. The only strange thing in strace is that the fd number is 0, so I wonder if libc has some optimization for the standard fds in fchown..
But if you agree, I would like to squash this change, because on failure, ret is always -1, but the error code is in errno.

Missing errno was a bug. Thank you for catching it. I fixed that.

And if you want to replace EOK with 0 feel free to do that.

-        if (ret != EOK) {
+        if (ret != 0) {

Commit ececbf9 fixes this pull-request

Pull-Request has been merged by jhrozek
5 years ago

Commit 21ea820 fixes this pull-request

Pull-Request has been merged by jhrozek
5 years ago

Commit 4900b8e fixes this pull-request

Pull-Request has been merged by jhrozek
5 years ago
Metadata
None
No Tags
Changes Summary 2
+27 -4
file changed
src/responder/sudo/sudosrv.c
+1 -2
file changed
src/sysv/systemd/sssd-sudo.socket.in
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.