Skip to content

Commit

Permalink
SELINUX_CHILD: only cap_set*id is required
Browse files Browse the repository at this point in the history
:packaging:*Important note for downstream maintainers.*
A set of capabilities required by privileged binaries
was further reduced to:
```
krb5_child cap_dac_read_search,cap_setgid,cap_setuid=p
ldap_child cap_dac_read_search=p
selinux_child cap_setgid,cap_setuid=p
sssd_pam cap_dac_read_search=p
```
Keep in mind that even with limited set of fine graned capabilities,
usual precautions still should be taken while packaging binaries with
file capabilities: it's very important to make sure that those are
executable only by root/sssd service user. For this reason upstream
spec file packages it as:
```
-rwxr-x---. 1 root sssd
```
Failing to do so (i.e. allowing non-privileged users to execute those
binaries) can impose systems installing the package to a security risk.

Reviewed-by: Justin Stephenson <[email protected]>
Reviewed-by: Sumit Bose <[email protected]>
(cherry picked from commit 84baae4)
  • Loading branch information
alexey-tikhonov committed Dec 10, 2024
1 parent 89627db commit 1614c5e
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 37 deletions.
10 changes: 6 additions & 4 deletions Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,9 @@ condconfigexists = ConditionPathExists=\|/etc/sssd/sssd.conf\nConditionDirectory
# - check old ccache / pre-check ccache path (dac_read_search, set*id)
# - read keytab (dac_read_search)
# - store TGT for a given user (set*id)
# - 'selinux_child': currently chown, dac_override, set*id -- to be narrowed
# - 'selinux_child': use libsemanage (set*id)
# - 'sssd_pam': read keytab in gss ops (dac_read_search)
capabilities = CapabilityBoundingSet= CAP_CHOWN CAP_DAC_OVERRIDE CAP_SETGID CAP_SETUID CAP_DAC_READ_SEARCH
capabilities = CapabilityBoundingSet= CAP_SETGID CAP_SETUID CAP_DAC_READ_SEARCH

if BUILD_CONF_SERVICE_USER_SUPPORT
# If non-root service user is supported, monitor might need SET-ID to switch user (deprecated 'sssd.conf::user' option)
Expand Down Expand Up @@ -4730,7 +4730,8 @@ selinux_child_SOURCES = \
src/util/atomic_io.c \
src/util/util.c \
src/util/util_ext.c \
src/util/util_errors.c
src/util/util_errors.c \
src/util/capabilities.c \
$(NULL)
selinux_child_CFLAGS = \
$(AM_CFLAGS) \
Expand All @@ -4741,6 +4742,7 @@ selinux_child_LDADD = \
$(TALLOC_LIBS) \
$(POPT_LIBS) \
$(DHASH_LIBS) \
$(CAP_LIBS) \
$(SEMANAGE_LIBS) \
$(SELINUX_LIBS) \
$(NULL)
Expand Down Expand Up @@ -5543,7 +5545,7 @@ if SSSD_USER
if BUILD_SELINUX
-chgrp $(SSSD_USER) $(DESTDIR)$(sssdlibexecdir)/selinux_child
chmod 750 $(DESTDIR)$(sssdlibexecdir)/selinux_child
-$(SETCAP) cap_chown,cap_dac_override,cap_setuid,cap_setgid=ep $(DESTDIR)$(sssdlibexecdir)/selinux_child
-$(SETCAP) cap_setuid,cap_setgid=p $(DESTDIR)$(sssdlibexecdir)/selinux_child
endif
endif

Expand Down
2 changes: 1 addition & 1 deletion contrib/sssd.spec.in
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ install -D -p -m 0644 %{SOURCE1} %{buildroot}%{_sysusersdir}/sssd.conf
%license COPYING
%attr(770,%{sssd_user},%{sssd_user}) %dir %{keytabdir}
%{_libdir}/%{name}/libsss_ipa.so
%attr(0750,root,%{sssd_user}) %caps(cap_chown,cap_dac_override,cap_setuid,cap_setgid=ep) %{_libexecdir}/%{servicename}/selinux_child
%attr(0750,root,%{sssd_user}) %caps(cap_setuid,cap_setgid=p) %{_libexecdir}/%{servicename}/selinux_child
%{_mandir}/man5/sssd-ipa.5*

%files ad -f sssd_ad.lang
Expand Down
72 changes: 40 additions & 32 deletions src/providers/ipa/selinux_child.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@
along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include "config.h"

#include <sys/types.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <popt.h>
#include <sys/prctl.h>
Expand Down Expand Up @@ -236,6 +237,8 @@ int main(int argc, const char *argv[])
const char *username;
const char *opt_logger = NULL;
long chain_id;
uid_t ruid, euid, suid;
gid_t rgid, egid, sgid;

struct poptOption long_options[] = {
POPT_AUTOHELP
Expand Down Expand Up @@ -291,10 +294,7 @@ int main(int argc, const char *argv[])
DEBUG_INIT(debug_level, opt_logger);
sss_set_debug_backtrace_enable((backtrace == 0) ? false : true);

DEBUG(SSSDBG_TRACE_FUNC, "selinux_child started.\n");
DEBUG(SSSDBG_TRACE_INTERNAL,
"Running with effective IDs: [%"SPRIuid"][%"SPRIgid"].\n",
geteuid(), getegid());
sss_log_process_caps("Starting");

/* The functions semanage_genhomedircon and getseuserbyname use gepwnam_r
* and they might fail to return values if they are not in memory cache.
Expand All @@ -312,31 +312,6 @@ int main(int argc, const char *argv[])
"fail.\n");
}

/* libsemanage calls access(2) which works with real IDs, not effective.
* We need to switch also the real ID to 0.
*/
if (getuid() != 0) {
ret = setuid(0);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"setuid failed: %d, selinux_child might not work!\n", ret);
}
}

if (getgid() != 0) {
ret = setgid(0);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"setgid failed: %d, selinux_child might not work!\n", ret);
}
}

DEBUG(SSSDBG_TRACE_INTERNAL,
"Running with real IDs [%"SPRIuid"][%"SPRIgid"].\n",
getuid(), getgid());

main_ctx = talloc_new(NULL);
if (main_ctx == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
Expand Down Expand Up @@ -376,8 +351,6 @@ int main(int argc, const char *argv[])
goto fail;
}

DEBUG(SSSDBG_TRACE_FUNC, "performing selinux operations\n");

/* When using domain_resolution_order the username will always be
* fully-qualified, what has been causing some SELinux issues as mappings
* for user 'admin' are not applied for '[email protected]'.
Expand All @@ -396,6 +369,32 @@ int main(int argc, const char *argv[])
username = passwd->pw_name;
}

/* libsemanage calls access(2) which works with real IDs, not effective.
* We need to switch also the real ID to 0.
*/
if (getuid() != 0) {
sss_set_cap_effective(CAP_SETUID, true);
ret = setresuid(0, 0, -1);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"setuid() failed: %d, selinux_child might not work!\n", ret);
}
}
if (getgid() != 0) {
sss_set_cap_effective(CAP_SETGID, true);
setgroups(0, NULL);
ret = setresgid(0, 0, -1);
if (ret == -1) {
ret = errno;
DEBUG(SSSDBG_CRIT_FAILURE,
"setgid() failed: %d, selinux_child might not work!\n", ret);
}
}
sss_drop_all_caps();

sss_log_process_caps("Performing selinux operations");

needs_update = seuser_needs_update(username, ibuf->seuser,
ibuf->mls_range);
if (needs_update == true) {
Expand All @@ -406,6 +405,15 @@ int main(int argc, const char *argv[])
}
}

if (getresuid(&ruid, &euid, &suid) == 0) {
setresuid(suid, suid, suid);
}
if (getresgid(&rgid, &egid, &sgid) == 0) {
setresgid(sgid, sgid, sgid);
}

sss_log_process_caps("Sending response");

ret = prepare_response(main_ctx, ret, &resp);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "Failed to prepare response buffer.\n");
Expand Down

0 comments on commit 1614c5e

Please sign in to comment.