From 2ff4d0f44ec21332d13de8d8ab8c39c77450ff7d Mon Sep 17 00:00:00 2001 From: ulfvonbelow Date: Thu, 2 Feb 2023 19:29:51 +0000 Subject: crontab: split into crontab and setuid helper crontab-access. If a user did somehow manage to install this crontab as functioning setuid-root in its current state (despite linux ignoring the setuid bit when executing scripts), it would be a very bad thing for them. It currently has several glaring security holes. In approximate order from most to least severe: 1. It blindly calls system() with the user-supplied value of VISUAL or EDITOR, without dropping privileges. I can't fathom what the author was thinking, considering (mcron scripts crontab) is littered with comments and evidence that this is supposed to be a setuid-root program. An attacker could simply run EDITOR='sh #' crontab -e and get a root shell. If you try this, you may find that it coincidentally doesn't work because bash in particular always drops privileges on startup if it detects differing real and effective ids. I don't know whether other shells do this, but it actually doesn't matter as long as you're using glibc, because its system() consults PATH looking for sh. One false entry in there and an attacker is running arbitrary code as root. And crontab doesn't do any sanitizing of *any* environment variables. 2. No attempt is made to sanitize any environment variables. Also, depending on Guile's startup behavior, trying to sanitize them in guile may be too late. A wrapper is needed, which would be needed anyway in order to use a setuid script. 3. No attempt is made to ensure that the temporary file being edited is newly-created, so an attacker could guess or deduce the filename to be used, create it in advance, keep it open while crontab opens it, and overwrite it right before it is copied, allowing them to execute arbitrary code as any user that dared edit their crontab, including root. 4. Its replace mode accepts a filename. It does no validation whatsoever on this, opens it, and copies it to the user's crontab as long as it's valid vixie cron syntax. So for example, crontab /var/cron/tabs/root && crontab --list will let you freely read root's (and in a similar manner any other user's) crontab. Vixie cron includes comments in its valid syntax, so any file that consists entirely of comments can also be dumped. Also, any file for which opening it and reading from it has side-effects can have those side-effects triggered even if it isn't valid vixie cron syntax. 5. Crontabs created in /tmp for editing, as well as crontabs created in /var/cron/tabs, are world-readable with typical inherited umask. (1) and (4) are resolved by splitting crontab into two programs: crontab, which is no longer setuid, and crontab-access, which is. The setuid program no longer opens any files except for the user's crontab and the allow/deny files, and it runs no external programs whatsoever. Crontab is run as the invoking user, so the usual kernel-level permissions checks regarding which files can be opened for reading apply. The editor is run from crontab, as the invoking user, so sanitizing of the environment in the setuid helper has no effect on the editor's environment. (2) to be resolved shortly with a wrapper program. (3) is resolved by using mkstemp. The inability to control the mode it is created with, along with (5), are resolved by setting the umask properly. * src/mcron/scripts/crontab-access.scm: new module. * src/mcron/scripts/crontab.scm: move list, delete, and replace implementation to crontab-access. * src/crontab-access.in: new file to invoke main of crontab-access. * Makefile.am: inform of crontab-access.in and crontab-access.scm. --- Makefile.am | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) (limited to 'Makefile.am') diff --git a/Makefile.am b/Makefile.am index 4afd7f3..4aff2ae 100755 --- a/Makefile.am +++ b/Makefile.am @@ -26,9 +26,9 @@ noinst_SCRIPTS = if MULTI_USER bin_SCRIPTS += bin/crontab -sbin_SCRIPTS = bin/cron +sbin_SCRIPTS = bin/cron bin/crontab-access else -noinst_SCRIPTS += bin/cron bin/crontab +noinst_SCRIPTS += bin/cron bin/crontab bin/crontab-access endif # wrapper to be used in the build environment and for running tests. @@ -68,6 +68,7 @@ pkgscriptdir = $(pkgmoduledir)/scripts dist_pkgscript_DATA = \ src/mcron/scripts/cron.scm \ src/mcron/scripts/crontab.scm \ + src/mcron/scripts/crontab-access.scm \ src/mcron/scripts/mcron.scm pkgscriptgodir = $(pkgmodulegodir)/scripts @@ -77,7 +78,11 @@ compiled_modules = \ $(pkgmodulego_DATA) \ $(pkgscriptgo_DATA) -CLEANFILES = $(compiled_modules) bin/crontab bin/cron bin/mcron +CLEANFILES = $(compiled_modules) \ + bin/crontab \ + bin/crontab-access \ + bin/cron \ + bin/mcron DISTCLEANFILES = src/mcron/config.scm # Unset 'GUILE_LOAD_COMPILED_PATH' altogether while compiling. Otherwise, if @@ -101,6 +106,8 @@ DISTCLEANFILES = src/mcron/config.scm --target="$(host)" --output="$@" "$<" $(devnull_verbose) do_subst = sed -e 's,%PREFIX%,${prefix},g' \ + -e 's,%sbindir%,${sbindir},g' \ + -e 's,%libexecdir%,${libexecdir},g' \ -e 's,%modsrcdir%,${guilesitedir},g' \ -e 's,%modbuilddir%,${guilesitegodir},g' \ -e 's,%localstatedir%,${localstatedir},g' \ @@ -156,6 +163,7 @@ EXTRA_DIST = \ HACKING \ src/cron.in \ src/crontab.in \ + src/crontab-access.in \ src/mcron.in \ tests/init.sh \ $(TESTS) @@ -169,8 +177,9 @@ transform_exe = s/$(EXEEXT)$$//;$(transform);s/$$/$(EXEEXT)/ install-exec-hook: if MULTI_USER - tcrontab=`echo crontab | sed '$(transform_exe)'`; \ - chmod u+s $(DESTDIR)$(bindir)/$${tcrontab} + tcrontab=`echo crontab | sed '$(transform_exe)'`; + tcrontab_access=`echo crontab-access | sed '$(transform_exe)'`; \ + chmod u+s $(DESTDIR)$(sbindir)/$${tcrontab_access} tcron=`echo cron | sed '$(transform_exe)'`; endif tmcron=`echo mcron | sed '$(transform_exe)'`; @@ -180,8 +189,9 @@ installcheck-local: tmcron=`echo mcron | sed '$(transform_exe)'`; \ test -e $(DESTDIR)$(bindir)/$${tmcron} if MULTI_USER - tcrontab=`echo crontab | sed '$(transform_exe)'`; \ - test -u $(DESTDIR)$(bindir)/$${tcrontab} + tcrontab=`echo crontab | sed '$(transform_exe)'`; + tcrontab_access=`echo crontab | sed '$(transform_exe)'`; \ + test -u $(DESTDIR)$(bindir)/$${tcrontab_access} tcron=`echo cron | sed '$(transform_exe)'`; \ test -e $(DESTDIR)$(sbindir)/$${tcron} else !MULTI_USER -- cgit v1.2.3