URL: https://github.com/SSSD/sssd/pull/183 Author: fidencio Title: #183: More socket-activation fixes Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/183/head:pr183 git checkout pr183
From f1683d696fe257bebd7f69393b97140287d8832a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Tue, 7 Mar 2017 16:55:41 +0100 Subject: [PATCH 1/2] SYSTEMD: Ensure the service is shutdown in case the socket is stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid leaving the service running in case the admin calls `systemctl stop sssd-@responder@.socket`. Resolves: https://pagure.io/SSSD/sssd/issue/3323 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/sysv/systemd/sssd-autofs.service.in | 2 ++ src/sysv/systemd/sssd-nss.service.in | 2 ++ src/sysv/systemd/sssd-pac.service.in | 2 ++ src/sysv/systemd/sssd-pam.service.in | 2 ++ src/sysv/systemd/sssd-ssh.service.in | 2 ++ src/sysv/systemd/sssd-sudo.service.in | 2 ++ 6 files changed, 12 insertions(+) diff --git a/src/sysv/systemd/sssd-autofs.service.in b/src/sysv/systemd/sssd-autofs.service.in index 32ea6e1..2f4196b 100644 --- a/src/sysv/systemd/sssd-autofs.service.in +++ b/src/sysv/systemd/sssd-autofs.service.in @@ -3,6 +3,8 @@ Description=SSSD AutoFS Service responder Documentation=man:sssd.conf(5) After=sssd.service BindsTo=sssd.service +After=sssd-autofs.socket +BindsTo=sssd-autofs.socket RefuseManualStart=true [Install] diff --git a/src/sysv/systemd/sssd-nss.service.in b/src/sysv/systemd/sssd-nss.service.in index e2f68bc..6b8bfcf 100644 --- a/src/sysv/systemd/sssd-nss.service.in +++ b/src/sysv/systemd/sssd-nss.service.in @@ -3,6 +3,8 @@ Description=SSSD NSS Service responder Documentation=man:sssd.conf(5) After=sssd.service BindsTo=sssd.service +After=sssd-nss.socket +BindsTo=sssd-nss.socket RefuseManualStart=true [Install] diff --git a/src/sysv/systemd/sssd-pac.service.in b/src/sysv/systemd/sssd-pac.service.in index ffbfdec..841f48a 100644 --- a/src/sysv/systemd/sssd-pac.service.in +++ b/src/sysv/systemd/sssd-pac.service.in @@ -3,6 +3,8 @@ Description=SSSD PAC Service responder Documentation=man:sssd.conf(5) After=sssd.service BindsTo=sssd.service +After=sssd-pac.socket +BindsTo=sssd-pac.socket RefuseManualStart=true [Install] diff --git a/src/sysv/systemd/sssd-pam.service.in b/src/sysv/systemd/sssd-pam.service.in index 6dec46f..aa413d2 100644 --- a/src/sysv/systemd/sssd-pam.service.in +++ b/src/sysv/systemd/sssd-pam.service.in @@ -3,6 +3,8 @@ Description=SSSD PAM Service responder Documentation=man:sssd.conf(5) After=sssd.service BindsTo=sssd.service +After=sssd-pam.socket +BindsTo=sssd-pam.socket RefuseManualStart=true [Install] diff --git a/src/sysv/systemd/sssd-ssh.service.in b/src/sysv/systemd/sssd-ssh.service.in index 6f233b4..d35b85e 100644 --- a/src/sysv/systemd/sssd-ssh.service.in +++ b/src/sysv/systemd/sssd-ssh.service.in @@ -3,6 +3,8 @@ Description=SSSD SSH Service responder Documentation=man:sssd.conf(5) After=sssd.service BindsTo=sssd.service +After=sssd-ssh.socket +BindsTo=sssd-ssh.socket RefuseManualStart=true [Install] diff --git a/src/sysv/systemd/sssd-sudo.service.in b/src/sysv/systemd/sssd-sudo.service.in index b59bcbc..9b0da3b 100644 --- a/src/sysv/systemd/sssd-sudo.service.in +++ b/src/sysv/systemd/sssd-sudo.service.in @@ -3,6 +3,8 @@ Description=SSSD Sudo Service responder Documentation=man:sssd.conf(5) man:sssd-sudo(5) After=sssd.service BindsTo=sssd.service +After=sssd-sudo.socket +BindsTo=sssd-sudo.socket RefuseManualStart=true [Install] From b5c9321b2d13e40ee7d9a63c5b9c8eb534a53a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Tue, 7 Mar 2017 23:15:30 +0100 Subject: [PATCH 2/2] Avoid TOCTOU vulnerability with the log files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The time-of-check time-of-use bug can occur on SSSD as the ownership of the log files is changed during the ExecStartPre but the file itself is only opened during the server_setup(). This issue was brough up while working on a fix for 3322[0]. A few conversations happened around this very same issue and some of the solutions are: - Pass two new arguments to the responders when initializing the processes. These arguments would be "user" and "group" and would take the user and group names. The problem with this solution is that SSSD would end up calling getpwnam() in order to get the uid and gid and it would be a circular dependency, which we really want to avoid during the service startup (as pointed by Stephen Gallagher). - Considering the sssd user has a static id, pass its uid and gid using the "uid" and "gid" arguments. This one would probably work well, but we cannot ensure that we're in sync with another distros which take advantage of the non-privileged user. - Provide our own binary for doing chown() without calling getpwnam() and use it instead of /usr/bin/chown. In my particular opinion we should avoiding taking this path, avoid keeping/maintaining our own implementation of chown. - Always run the socket-activated services as root. This is a really strong thing to do, but ensures that we won't have the TUCTOU issue and we're not going to call getpwnam() during the service initialization. The downside of this solution is that downstream distros would have to patch SSSD in case they want to support running the socket-activated services as unprivileged user. The path I've chosen is a mix of the second and the last options. We start passing the uid and gid arguments to the socket-activated services and also drop support to run them as unprivileged user (at least for now). As part of this patch open_and_fchown_debug_file() has also been added in order to ensure we open the file, with the file opened we change its ownership and keep the file opened while it's being used, avoiding then the TOCTOU vulnerability. This patch indirectly solves 3322[0]. [0]: https://pagure.io/SSSD/sssd/issue/3322 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- Makefile.am | 4 +- src/sysv/systemd/sssd-autofs.service.in | 6 +-- src/sysv/systemd/sssd-autofs.socket.in | 2 - src/sysv/systemd/sssd-nss.service.in | 3 +- src/sysv/systemd/sssd-nss.socket.in | 2 - src/sysv/systemd/sssd-pac.service.in | 6 +-- src/sysv/systemd/sssd-pac.socket.in | 2 - src/sysv/systemd/sssd-pam-priv.socket.in | 2 - src/sysv/systemd/sssd-pam.service.in | 6 +-- src/sysv/systemd/sssd-pam.socket.in | 2 - src/sysv/systemd/sssd-ssh.service.in | 6 +-- src/sysv/systemd/sssd-ssh.socket.in | 2 - src/sysv/systemd/sssd-sudo.service.in | 6 +-- src/sysv/systemd/sssd-sudo.socket.in | 2 - src/util/debug.c | 78 ++++++++++++++++++++++++++++---- src/util/server.c | 24 +++++----- src/util/util.h | 2 + 17 files changed, 89 insertions(+), 66 deletions(-) diff --git a/Makefile.am b/Makefile.am index 34da1f0..1265f61 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4150,11 +4150,9 @@ edit_cmd = $(SED) \ -e 's|@sbindir[@]|$(sbindir)|g' \ -e 's|@environment_file[@]|$(environment_file)|g' \ -e 's|@localstatedir[@]|$(localstatedir)|g' \ - -e 's|@logpath[@]|$(logpath)|g' \ -e 's|@libexecdir[@]|$(libexecdir)|g' \ -e 's|@pipepath[@]|$(pipepath)|g' \ - -e 's|@prefix[@]|$(prefix)|g' \ - -e 's|@SSSD_USER[@]|$(SSSD_USER)|g' + -e 's|@prefix[@]|$(prefix)|g' replace_script = \ @rm -f $@ $@.tmp; \ diff --git a/src/sysv/systemd/sssd-autofs.service.in b/src/sysv/systemd/sssd-autofs.service.in index 2f4196b..2aaf5d6 100644 --- a/src/sysv/systemd/sssd-autofs.service.in +++ b/src/sysv/systemd/sssd-autofs.service.in @@ -11,9 +11,5 @@ RefuseManualStart=true Also=sssd-autofs.socket [Service] -ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_autofs.log -ExecStart=@libexecdir@/sssd/sssd_autofs --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_autofs --uid 0 --gid 0 --debug-to-files --socket-activated Restart=on-failure -User=@SSSD_USER@ -Group=@SSSD_USER@ -PermissionsStartOnly=true diff --git a/src/sysv/systemd/sssd-autofs.socket.in b/src/sysv/systemd/sssd-autofs.socket.in index 201b33d..1cd1392 100644 --- a/src/sysv/systemd/sssd-autofs.socket.in +++ b/src/sysv/systemd/sssd-autofs.socket.in @@ -9,8 +9,6 @@ Conflicts=shutdown.target [Socket] ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r autofs ListenStream=@pipepath@/autofs -SocketUser=@SSSD_USER@ -SocketGroup=@SSSD_USER@ [Install] WantedBy=sssd.service diff --git a/src/sysv/systemd/sssd-nss.service.in b/src/sysv/systemd/sssd-nss.service.in index 6b8bfcf..448725a 100644 --- a/src/sysv/systemd/sssd-nss.service.in +++ b/src/sysv/systemd/sssd-nss.service.in @@ -11,6 +11,5 @@ RefuseManualStart=true Also=sssd-nss.socket [Service] -ExecStartPre=-/bin/chown root:root @logpath@/sssd_nss.log -ExecStart=@libexecdir@/sssd/sssd_nss --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_nss --uid 0 --gid 0 --debug-to-files --socket-activated Restart=on-failure diff --git a/src/sysv/systemd/sssd-nss.socket.in b/src/sysv/systemd/sssd-nss.socket.in index 39d30e8..83c1285 100644 --- a/src/sysv/systemd/sssd-nss.socket.in +++ b/src/sysv/systemd/sssd-nss.socket.in @@ -9,8 +9,6 @@ Conflicts=shutdown.target [Socket] ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r nss ListenStream=@pipepath@/nss -SocketUser=@SSSD_USER@ -SocketGroup=@SSSD_USER@ [Install] WantedBy=sssd.service diff --git a/src/sysv/systemd/sssd-pac.service.in b/src/sysv/systemd/sssd-pac.service.in index 841f48a..d6cde93 100644 --- a/src/sysv/systemd/sssd-pac.service.in +++ b/src/sysv/systemd/sssd-pac.service.in @@ -11,9 +11,5 @@ RefuseManualStart=true Also=sssd-pac.socket [Service] -ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_pac.log -ExecStart=@libexecdir@/sssd/sssd_pac --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_pac --uid 0 --gid 0 --debug-to-files --socket-activated Restart=on-failure -User=@SSSD_USER@ -Group=@SSSD_USER@ -PermissionsStartOnly=true diff --git a/src/sysv/systemd/sssd-pac.socket.in b/src/sysv/systemd/sssd-pac.socket.in index 40dec44..4388e9f 100644 --- a/src/sysv/systemd/sssd-pac.socket.in +++ b/src/sysv/systemd/sssd-pac.socket.in @@ -9,8 +9,6 @@ Conflicts=shutdown.target [Socket] ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r pac ListenStream=@pipepath@/pac -SocketUser=@SSSD_USER@ -SocketGroup=@SSSD_USER@ [Install] WantedBy=sssd.service diff --git a/src/sysv/systemd/sssd-pam-priv.socket.in b/src/sysv/systemd/sssd-pam-priv.socket.in index 27f2cf7..bfaf5b0 100644 --- a/src/sysv/systemd/sssd-pam-priv.socket.in +++ b/src/sysv/systemd/sssd-pam-priv.socket.in @@ -11,8 +11,6 @@ Conflicts=shutdown.target ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r pam Service=sssd-pam.service ListenStream=@pipepath@/private/pam -SocketUser=root -SocketGroup=root SocketMode=0600 [Install] diff --git a/src/sysv/systemd/sssd-pam.service.in b/src/sysv/systemd/sssd-pam.service.in index aa413d2..dca3345 100644 --- a/src/sysv/systemd/sssd-pam.service.in +++ b/src/sysv/systemd/sssd-pam.service.in @@ -11,9 +11,5 @@ RefuseManualStart=true Also=sssd-pam.socket sssd-pam-priv.socket [Service] -ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_pam.log -ExecStart=@libexecdir@/sssd/sssd_pam --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_pam --uid 0 --gid 0 --debug-to-files --socket-activated Restart=on-failure -User=@SSSD_USER@ -Group=@SSSD_USER@ -PermissionsStartOnly=true diff --git a/src/sysv/systemd/sssd-pam.socket.in b/src/sysv/systemd/sssd-pam.socket.in index cbbb762..d8b5faa 100644 --- a/src/sysv/systemd/sssd-pam.socket.in +++ b/src/sysv/systemd/sssd-pam.socket.in @@ -10,8 +10,6 @@ Conflicts=shutdown.target [Socket] ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r pam ListenStream=@pipepath@/pam -SocketUser=root -SocketGroup=root [Install] WantedBy=sssd.service diff --git a/src/sysv/systemd/sssd-ssh.service.in b/src/sysv/systemd/sssd-ssh.service.in index d35b85e..a2c1902 100644 --- a/src/sysv/systemd/sssd-ssh.service.in +++ b/src/sysv/systemd/sssd-ssh.service.in @@ -11,9 +11,5 @@ RefuseManualStart=true Also=sssd-ssh.socket [Service] -ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_ssh.log -ExecStart=@libexecdir@/sssd/sssd_ssh --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_ssh --uid 0 --gid 0 --debug-to-files --socket-activated Restart=on-failure -User=@SSSD_USER@ -Group=@SSSD_USER@ -PermissionsStartOnly=true diff --git a/src/sysv/systemd/sssd-ssh.socket.in b/src/sysv/systemd/sssd-ssh.socket.in index 4772ef3..97c3d62 100644 --- a/src/sysv/systemd/sssd-ssh.socket.in +++ b/src/sysv/systemd/sssd-ssh.socket.in @@ -9,8 +9,6 @@ Conflicts=shutdown.target [Socket] ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r ssh ListenStream=@pipepath@/ssh -SocketUser=@SSSD_USER@ -SocketGroup=@SSSD_USER@ [Install] WantedBy=sssd.service diff --git a/src/sysv/systemd/sssd-sudo.service.in b/src/sysv/systemd/sssd-sudo.service.in index 9b0da3b..2923a03 100644 --- a/src/sysv/systemd/sssd-sudo.service.in +++ b/src/sysv/systemd/sssd-sudo.service.in @@ -11,9 +11,5 @@ RefuseManualStart=true Also=sssd-sudo.socket [Service] -ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_sudo.log -ExecStart=@libexecdir@/sssd/sssd_sudo --debug-to-files --socket-activated +ExecStart=@libexecdir@/sssd/sssd_sudo --uid 0 --gid 0 --debug-to-files --socket-activated Restart=on-failure -User=@SSSD_USER@ -Group=@SSSD_USER@ -PermissionsStartOnly=true diff --git a/src/sysv/systemd/sssd-sudo.socket.in b/src/sysv/systemd/sssd-sudo.socket.in index c9abb87..e83bed5 100644 --- a/src/sysv/systemd/sssd-sudo.socket.in +++ b/src/sysv/systemd/sssd-sudo.socket.in @@ -9,8 +9,6 @@ Conflicts=shutdown.target [Socket] ExecStartPre=@libexecdir@/sssd/sssd_check_socket_activated_responders -r sudo ListenStream=@pipepath@/sudo -SocketUser=@SSSD_USER@ -SocketGroup=@SSSD_USER@ [Install] WantedBy=sssd.service diff --git a/src/util/debug.c b/src/util/debug.c index ca4fa4c..2e6fe46 100644 --- a/src/util/debug.c +++ b/src/util/debug.c @@ -406,6 +406,45 @@ int open_debug_file_ex(const char *filename, FILE **filep, bool want_cloexec) return EOK; } +int open_and_fchown_debug_file(uid_t uid, gid_t gid) +{ + char *logpath; + int fd = -1; + int ret; + + ret = asprintf(&logpath, "%s/%s.log", LOG_PATH, debug_log_file); + if (ret == -1) { + ret = ENOMEM; + goto done; + } + + errno = 0; + fd = open(logpath, FD_CLOEXEC, O_WRONLY); + if (fd == -1) { + ret = errno; + goto done; + } + + ret = fchown(fd, uid, gid); + if (ret == -1) { + ret = errno; + close(fd); + goto done; + } + + ret = set_debug_file_from_fd(fd); + if (ret != EOK) { + close(fd); + goto done; + } + + ret = EOK; + +done: + free(logpath); + return ret; +} + int open_debug_file(void) { return open_debug_file_ex(NULL, NULL, true); @@ -414,10 +453,32 @@ int open_debug_file(void) int rotate_debug_files(void) { int ret; - errno_t error; if (!debug_to_file) return EOK; + ret = close_debug_files(); + if (ret != 0) { + sss_log(SSS_LOG_ALERT, "Attempting to open new file anyway. " + "Be aware that this is a resource leak\n"); + } + + return open_debug_file(); +} + +void talloc_log_fn(const char *message) +{ + DEBUG(SSSDBG_FATAL_FAILURE, "%s\n", message); +} + +int close_debug_files(void) +{ + int ret; + errno_t error; + + if (debug_file == NULL) { + return EOK; + } + do { error = 0; ret = fclose(debug_file); @@ -441,16 +502,13 @@ int rotate_debug_files(void) */ sss_log(SSS_LOG_ALERT, "Could not close debug file [%s]. [%d][%s]\n", debug_log_file, error, strerror(error)); - sss_log(SSS_LOG_ALERT, "Attempting to open new file anyway. " - "Be aware that this is a resource leak\n"); + ret = error; + goto done; } - debug_file = NULL; + ret = EOK; - return open_debug_file(); -} - -void talloc_log_fn(const char *message) -{ - DEBUG(SSSDBG_FATAL_FAILURE, "%s\n", message); +done: + debug_file = NULL; + return ret; } diff --git a/src/util/server.c b/src/util/server.c index 9c94418..f0fc336 100644 --- a/src/util/server.c +++ b/src/util/server.c @@ -472,13 +472,13 @@ int server_setup(const char *name, int flags, sss_strerror(ret), ret); } - if (!is_socket_activated()) { - ret = chown_debug_file(NULL, uid, gid); - if (ret != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, - "Cannot chown the debug files, debugging might not work!\n"); - } + ret = open_and_fchown_debug_file(uid, gid); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Cannot chown the debug files, debugging might not work!\n"); + } + if (!is_socket_activated()) { ret = become_user(uid, gid); if (ret != EOK) { DEBUG(SSSDBG_FUNC_DATA, @@ -660,13 +660,13 @@ int server_setup(const char *name, int flags, return EIO; } - /* open log file if told so */ - if (debug_to_file) { - ret = open_debug_file(); + if (debug_to_file == 0) { + ret = close_debug_files(); if (ret != EOK) { - DEBUG(SSSDBG_FATAL_FAILURE, "Error setting up logging (%d) " - "[%s]\n", ret, strerror(ret)); - return ret; + DEBUG(SSSDBG_MINOR_FAILURE, + "Cannot close the debug file, even if it's explicitly set " + "to not debug to file: [%d] [%s]\n", + ret, sss_strerror(ret)); } } diff --git a/src/util/util.h b/src/util/util.h index a2dc89b..f44a076 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -158,6 +158,8 @@ int chown_debug_file(const char *filename, uid_t uid, gid_t gid); int open_debug_file_ex(const char *filename, FILE **filep, bool want_cloexec); int open_debug_file(void); int rotate_debug_files(void); +int open_and_fchown_debug_file(uid_t uid, gid_t gid); +int close_debug_files(void); void talloc_log_fn(const char *msg); /* From sss_log.c */
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
