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.

3 files changed, 278 insertions, 143 deletions

diff --git a/src/crontab-access.in b/src/crontab-access.in
new file mode 100644
index 0000000..569d147
--- /dev/null
+++ b/src/crontab-access.in
@@ -0,0 +1,45 @@
+#!%GUILE% --no-auto-compile
+-*- scheme -*-
+!#
+
+;;;; crontab -- run jobs at scheduled times
+;;; Copyright © 2003, 2020  Dale Mellor <mcron-lsfnyl@rdmp.org>
+;;; Copyright © 2015, 2016, 2018  Mathieu Lirzin <mthl@gnu.org>
+;;;
+;;; This file is part of GNU Mcron.
+;;;
+;;; GNU Mcron is free software: you can redistribute it and/or modify
+;;; it under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation, either version 3 of the License, or
+;;; (at your option) any later version.
+;;;
+;;; GNU Mcron is distributed in the hope that it will be useful,
+;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Mcron.  If not, see <http://www.gnu.org/licenses/>.
+
+
+(unless (getenv "MCRON_UNINSTALLED")
+  (set! %load-path (cons "%modsrcdir%" %load-path))
+  (set! %load-compiled-path (cons "%modbuilddir%" %load-compiled-path)))
+
+(use-modules (mcron scripts crontab)
+             (mcron command-line-processor))
+
+(process-command-line  (command-line)
+   application "crontab"
+   version     "%VERSION%"
+   usage       "[-u user] { -R | -l | -r }"
+   help-preamble "the default operation is to list."
+   option (--user=     -u  "the user whose files are to be manipulated")
+   option (--replace   -R  "replace this userʼs crontab via stdin")
+   option (--list      -l  "list this userʼs crontab")
+   option (--remove    -r  "delete the userʼs crontab")
+   bug-address "%PACKAGE_BUGREPORT%"
+   copyright   "2003, 2016, 2020  Free Software Foundation, Inc."
+   license     GPLv3)
+
+((@ (mcron scripts crontab-access) main) --user --replace --list --remove)
diff --git a/src/mcron/scripts/crontab-access.scm b/src/mcron/scripts/crontab-access.scm
new file mode 100644
index 0000000..d97fc62
--- /dev/null
+++ b/src/mcron/scripts/crontab-access.scm
@@ -0,0 +1,121 @@
+;;;; crontab -- edit user's cron tabs
+;;; Copyright © 2003, 2004 Dale Mellor <>
+;;; Copyright © 2016 Mathieu Lirzin <mthl@gnu.org>
+;;;
+;;; This file is part of GNU Mcron.
+;;;
+;;; GNU Mcron is free software: you can redistribute it and/or modify
+;;; it under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation, either version 3 of the License, or
+;;; (at your option) any later version.
+;;;
+;;; GNU Mcron is distributed in the hope that it will be useful,
+;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with GNU Mcron.  If not, see <http://www.gnu.org/licenses/>.
+(define-module (mcron scripts crontab-access)
+  #:use-module (ice-9 rdelim)
+  #:use-module (mcron config)
+  #:use-module (mcron utils)
+  #:use-module (mcron vixie-specification)
+  #:export (main))
+
+(define (hit-server user-name)
+  "Tell the running cron daemon that the user corresponding to
+USER-NAME has modified his crontab.  USER-NAME is written to the
+'/var/cron/socket' UNIX socket."
+  (catch #t
+    (lambda ()
+      (let ((socket (socket AF_UNIX SOCK_STREAM 0)))
+        (connect socket AF_UNIX config-socket-file)
+        (display user-name socket)
+        (close socket)))
+    (lambda (key . args)
+      (display "Warning: a cron daemon is not running.\n"))))
+
+(define (in-access-file? file name)
+  "Scan FILE which should contain one user name per line (such as
+'/var/cron/allow' and '/var/cron/deny').  Return #t if NAME is in there, and
+#f otherwise.  If FILE cannot be opened, a value that is neither #t nor #f
+is returned."
+  (catch #t
+    (lambda ()
+      (with-input-from-file file
+        (lambda ()
+          (let loop ((input (read-line)))
+            (cond ((eof-object? input)
+                   #f)
+                  ((string=? input name)
+                   #t)
+                  (else
+                   (loop (read-line))))))))
+    (const '())))
+
+(define (main --user --replace --list --remove)
+  (when config-debug  (debug-enable 'backtrace))
+  (let ((crontab-real-user
+         ;; This program should have been installed SUID root. Here we get
+         ;; the passwd entry for the real user who is running this program.
+         (passwd:name (getpw (getuid)))))
+
+    ;; If the real user is not allowed to use crontab due to the
+    ;; /var/cron/allow and/or /var/cron/deny files, bomb out now.
+    (if (or (eq? (in-access-file? config-allow-file crontab-real-user) #f)
+            (eq? (in-access-file? config-deny-file crontab-real-user) #t))
+        (mcron-error 6 "Access denied by system operator."))
+
+    ;; Check that no more than one of the mutually exclusive options are
+    ;; being used.
+    (when (<  1  (+ (if --list 1 0) (if --remove 1 0) (if --replace 1 0)))
+      (mcron-error 7 "Only one of options -l, -r or -R can be used."))
+
+    ;; Check that a non-root user is trying to read someone else's files.
+    (when (and (not (zero? (getuid))) --user)
+      (mcron-error 8 "Only root can use the -u option."))
+
+    ;; Crontabs being written should not have global or group access.
+    (umask #o077)
+
+    (letrec* ( ;; Iff the --user option is given, the crontab-user may be
+              ;; different from the real user.
+              (crontab-user (or --user crontab-real-user))
+              ;; So now we know which crontab file we will be manipulating.
+              (crontab-file
+               (string-append config-spool-dir "/" crontab-user)))
+      ;; There are three possible actions: list, remove, and replace (via
+      ;; stdin).
+      (cond
+       ;; In the remove personality we simply make an effort to delete the
+       ;; crontab and wake the daemon. No worries if this fails.
+       (--remove (catch #t (λ ()  (delete-file crontab-file)
+                              (hit-server crontab-user))
+                   noop))
+
+       ;; Read crontab from stdin, verify it, replace it, wake daemon.
+       (--replace
+        (let ((input-string (read-string)))
+          (catch-mcron-error
+           (read-vixie-port (open-input-string input-string))
+           (unless (file-exists? config-spool-dir)
+             (mkdir config-spool-dir #o700))
+           (with-output-to-file crontab-file
+             (λ () (display input-string))))
+          (hit-server crontab-user)))
+
+       ;; In the list personality, we simply open the crontab and copy it
+       ;; character-by-character to the standard output. If anything goes
+       ;; wrong, it can only mean that this user does not have a crontab
+       ;; file.
+       (else ;; --list or no action specified
+        (catch #t
+          (λ ()
+            (with-input-from-file crontab-file
+              (λ ()
+                (do ((input (read-char) (read-char)))
+                    ((eof-object? input))
+                  (display input)))))
+          (λ (key . args)
+            (mcron-error 17 "No crontab for " crontab-user " exists.\n"))))))))
diff --git a/src/mcron/scripts/crontab.scm b/src/mcron/scripts/crontab.scm
index 0c5c47f..57566f4 100644
--- a/src/mcron/scripts/crontab.scm
+++ b/src/mcron/scripts/crontab.scm
@@ -19,26 +19,12 @@
 
 (define-module (mcron scripts crontab)
   #:use-module (ice-9 rdelim)
+  #:use-module (srfi srfi-1)
   #:use-module (mcron config)
   #:use-module (mcron utils)
   #:use-module (mcron vixie-specification)
   #:export (main))
 
-(define (hit-server user-name)
-  "Tell the running cron daemon that the user corresponding to
-USER-NAME has modified his crontab.  USER-NAME is written to the
-'/var/cron/socket' UNIX socket."
-  (catch #t
-    (lambda ()
-      (let ((socket (socket AF_UNIX SOCK_STREAM 0)))
-        (connect socket AF_UNIX config-socket-file)
-        (display user-name socket)
-        (close socket)))
-    (lambda (key . args)
-      (display "Warning: a cron daemon is not running.\n"))))
-
-
-
 ;; Display the prompt and wait for user to type his choice. Return #t if the
 ;; answer begins with 'y' or 'Y', return #f if it begins with 'n' or 'N',
 ;; otherwise ask again.
@@ -56,23 +42,6 @@ USER-NAME has modified his crontab.  USER-NAME is written to the
 
 
 
-(define (in-access-file? file name)
-  "Scan FILE which should contain one user name per line (such as
-'/var/cron/allow' and '/var/cron/deny').  Return #t if NAME is in there, and
-#f otherwise.  if FILE cannot be opened, a error is signaled."
-  (catch #t
-    (lambda ()
-      (with-input-from-file file
-        (lambda ()
-          (let loop ((input (read-line)))
-            (cond ((eof-object? input)
-                   #f)
-                  ((string=? input name)
-                   #t)
-                  (else
-                   (loop (read-line))))))))
-    (const '())))
-
 
 ;;;
 ;;; Entry point.
@@ -80,117 +49,117 @@ USER-NAME has modified his crontab.  USER-NAME is written to the
 
 (define (main --user --edit --list --remove files)
   (when config-debug  (debug-enable 'backtrace))
-  (let ((crontab-real-user
-         ;; This program should have been installed SUID root. Here we get
-         ;; the passwd entry for the real user who is running this program.
-         (passwd:name (getpw (getuid)))))
-
-    ;; If the real user is not allowed to use crontab due to the
-    ;; /var/cron/allow and/or /var/cron/deny files, bomb out now.
-    (if (or (eq? (in-access-file? config-allow-file crontab-real-user) #f)
-            (eq? (in-access-file? config-deny-file crontab-real-user) #t))
-        (mcron-error 6 "Access denied by system operator."))
-
-    ;; Check that no more than one of the mutually exclusive options are
-    ;; being used.
-      (when (<  1  (+ (if --edit 1 0) (if --list 1 0) (if --remove 1 0)))
-        (mcron-error 7 "Only one of options -e, -l or -r can be used."))
-
-      ;; Check that a non-root user is trying to read someone else's files.
-      (when (and (not (zero? (getuid))) --user)
-        (mcron-error 8 "Only root can use the -u option."))
-
-      (letrec* (;; Iff the --user option is given, the crontab-user may be
-                ;; different from the real user.
-                (crontab-user (or --user crontab-real-user))
-                ;; So now we know which crontab file we will be manipulating.
-                (crontab-file
-                         (string-append config-spool-dir "/" crontab-user)))
-        ;; There are four possible sub-personalities to the crontab
-        ;; personality: list, remove, edit and replace (when the user uses no
-        ;; options but supplies file names on the command line).
+  ;; Check that no more than one of the mutually exclusive options are
+  ;; being used.
+  (when (<  1  (+ (if --edit 1 0) (if --list 1 0) (if --remove 1 0)))
+    (mcron-error 7 "Only one of options -e, -l or -r can be used."))
+
+  ;; Check that a non-root user is trying to read someone else's files.
+  ;; This will be enforced in the setuid helper 'crontab-access', but good
+  ;; to let the user know early.
+  (when (and (not (zero? (getuid))) --user)
+    (mcron-error 8 "Only root can use the -u option."))
+
+  ;; Crontabs being edited should not be global or group-readable.
+  (umask #o077)
+
+  (let ((user-args (if --user (list "-u" --user) '())))
+    (define (usable-crontab-access? filename)
+      (and=> (stat filename #f)
+             (λ (st)
+               (or (zero? (getuid))
+                   (and (not (zero? (logand #o4000 (stat:mode st))))
+                        (zero? (stat:uid st))
+                        (access? filename X_OK))))))
+
+    (define crontab-access
+      (find usable-crontab-access?
+            (map (λ (f) (string-append f "/crontab-access"))
+                 (cons config-sbin-dir
+                       (or (and=> (getenv "PATH") parse-path)
+                           '())))))
+
+    (define (exec-crontab-access . args)
+      (catch #t
+        (λ ()
+          (apply execlp crontab-access crontab-access
+                 (append user-args args))
+          (mcron-error 18 "Couldn't execute `crontab-access'."))
+        (λ args
+          (apply mcron-error 18 "Couldn't execute `crontab-access'." args))))
+
+    (define (crontab-access-dup2 srcs dsts closes . args)
+      (let ((pid (primitive-fork)))
         (cond
-         ;; In the list personality, we simply open the crontab and copy it
-         ;; character-by-character to the standard output. If anything goes
-         ;; wrong, it can only mean that this user does not have a crontab
-         ;; file.
-         (--list
+         ((zero? pid)
+          (for-each dup2 srcs dsts)
+          (for-each close-fdes closes)
+          (apply exec-crontab-access args))
+         (else
+          (waitpid pid)))))
+
+    (define (try-replace file)
+      (call-with-input-file file
+        (λ (port)
+          (crontab-access-dup2 (list (fileno port)) '(0) '() "-R"))))
+
+    (unless crontab-access
+      (mcron-error 18 "Couldn't find a usable `crontab-access'."))
+
+    ;; There are four possible sub-personalities to the crontab
+    ;; personality: list, remove, edit and replace (when the user uses no
+    ;; options but supplies file names on the command line).
+    (cond
+     (--list (exec-crontab-access "-l"))
+     (--remove (exec-crontab-access "-r"))
+
+     ;; In the edit personality, we determine the name of a temporary file and
+     ;; an editor command, copy an existing crontab file (if it is there) to
+     ;; the temporary file, once the editor returns we try to replace any
+     ;; existing crontab file.  If this fails, we give user a choice of
+     ;; editing the file again or quitting the program and losing all changes
+     ;; made.
+     (--edit
+      (let* ((template (string-append config-tmp-dir
+                                      "/crontab."
+                                      (number->string (getpid))
+                                      ".XXXXXX"))
+             (temp-file (call-with-port (mkstemp template "w")
+                          (λ (port)
+                            (crontab-access-dup2 (list (fileno port)) '(1)
+                                                 '(2) "-l")
+                            (chmod port #o600)
+                            (port-filename port)))))
+        (define (exit/cleanup status)
+          (false-if-exception (delete-file temp-file))
+          (primitive-exit status))
+
+        (let retry ()
           (catch #t
-            (λ ()
-              (with-input-from-file crontab-file
-                (λ ()
-                  (do ((input (read-char) (read-char)))
-                      ((eof-object? input))
-                    (display input)))))
-            (λ (key . args)
-              (display (string-append "No crontab for "
-                                      crontab-user
-                                      " exists.\n")))))
-
-         ;; In the edit personality, we determine the name of a temporary file
-         ;; and an editor command, copy an existing crontab file (if it is
-         ;; there) to the temporary file, making sure the ownership is set so
-         ;; the real user can edit it; once the editor returns we try to read
-         ;; the file to check that it is parseable (but do nothing more with
-         ;; the configuration), and if it is okay (this program is still
-         ;; running!) we move the temporary file to the real crontab, wake the
-         ;; cron daemon up, and remove the temporary file. If the parse fails,
-         ;; we give user a choice of editing the file again or quitting the
-         ;; program and losing all changes made.
-         (--edit
-          (let ((temp-file (string-append config-tmp-dir
-                                          "/crontab."
-                                          (number->string (getpid)))))
-            (catch #t
-              (λ () (copy-file crontab-file temp-file))
-              (λ (key . args) (with-output-to-file temp-file noop)))
-            (chown temp-file (getuid) (getgid))
-            (let retry ()
-              (system (string-append
-                       (or (getenv "VISUAL") (getenv "EDITOR") "vi")
-                       " "
-                       temp-file))
-              (catch 'mcron-error
-                (λ () (read-vixie-file temp-file))
-                (λ (key exit-code . msg)
-                  (apply mcron-error 0 msg)
-                  (if (get-yes-no "Edit again?")
-                      (retry)
-                      (begin
-                        (mcron-error 0 "Crontab not changed")
-                        (primitive-exit 0))))))
-            (copy-file temp-file crontab-file)
-            (delete-file temp-file)
-            (hit-server crontab-user)))
-
-         ;; In the remove personality we simply make an effort to delete the
-         ;; crontab and wake the daemon. No worries if this fails.
-         (--remove (catch #t (λ ()  (delete-file crontab-file)
-                                    (hit-server crontab-user))
-                          noop))
-
-         ;; XXX: This comment is wrong.
-         ;; In the case of the replace personality we loop over all the
-         ;; arguments on the command line, and for each one parse the file to
-         ;; make sure it is parseable (but subsequently ignore the
-         ;; configuration), and all being well we copy it to the crontab
-         ;; location; we deal with the standard input in the same way but
-         ;; different. :-) In either case the server is woken so that it will
-         ;; read the newly installed crontab.
-         ((not (null? files))
-          (let ((input-file (car files)))
-            (catch-mcron-error
-             (if (string=? input-file "-")
-                 (let ((input-string (read-string)))
-                   (read-vixie-port (open-input-string input-string))
-                   (with-output-to-file crontab-file
-                     (λ () (display input-string))))
+            (λ () (system (string-append
+                           (or (getenv "VISUAL") (getenv "EDITOR") "vi")
+                           " "
+                           temp-file)))
+            (λ _ (exit/cleanup 1)))
+          (case (status:exit-val (cdr (try-replace temp-file)))
+            ((9 10 11)
+             (if (get-yes-no "Edit again?")
+                 (retry)
                  (begin
-                   (read-vixie-file input-file)
-                   (copy-file input-file crontab-file))))
-            (hit-server crontab-user)))
-
-         ;; The user is being silly. The message here is identical to the one
-         ;; Vixie cron used to put out, for total compatibility.
-         (else (mcron-error 15
-                 "usage error: file name must be specified for replace."))))))
+                   (mcron-error 0 "Crontab not changed")
+                   (exit/cleanup 0))))
+            (else => exit/cleanup)))))
+
+     ;; Replace crontab with given file or stdin.  If it is a file, it must
+     ;; be opened here and not in the setuid helper, to prevent accessing
+     ;; unauthorized files.
+     ((not (null? files))
+      (let ((input-file (car files)))
+        (unless (string=? input-file "-")
+          (dup2 (fileno (open-file input-file "r")) 0))
+        (exec-crontab-access "-R")))
+
+     ;; The user is being silly. The message here is identical to the one
+     ;; Vixie cron used to put out, for total compatibility.
+     (else (mcron-error 15
+             "usage error: file name must be specified for replace.")))))