Re: Setuid programs

[Christopher Lemmer Webber]

Christopher Lemmer Webber writes:

> Ludovic Courtès writes:
>
>> Hi,
>>
>> Gábor Boskovits <boskovits@gmail.com> skribis:
>>
>>> I have two reasons for that: backwards compatibility is really
>>> important, so we should not break it, and I believe this would not be
>>> hard to do.
>>> On the other hand it would be nice to have a more integrated backend,
>>> and move as many things into the services infrastructure as practical,
>>> and I think this is a good candidate for that. Wdyt?
>>
>> There’s already ‘setuid-program-service-type’.  I think the way forward
>> would be to:
>>
>>   1. Define the <setuid-program> record type you propose.
>>
>>   2. Have ‘setuid-program-service-type’ accept that through its
>>      extensions.  When it receives something else, it should
>>      transparently turn it into a <setuid-program> record, for backward
>>      compatibility, and emit a deprecation warning.
>>
>>   3. Document the OS ‘setuid-programs’ field as taking a list of such
>>      records.
>>
>> How does that sound?
>>
>> Thanks,
>> Ludo’.
>
> This sounds like a good plan.  I'm taking a stab at it, but there's a
> good chance I'll get it wrong, so review will be seriously needed.
> Let's find out how I do!

I've attached a patch that includes my plan for the setuid stuff.  I
could submit this to guix-patches I suppose if that would be better.
But I wonder if I should actually just rebase the wip-postfix on top of
master, apply this, and then start working on setting up postfix to make
use of it.

What do you think of this approach?

>From cab9f7c017fb2ea0c8dc80084c3c269fa8e85378 Mon Sep 17 00:00:00 2001
From: Christopher Lemmer Webber <cwebber@dustycloud.org>
Date: Sun, 15 Nov 2020 16:58:52 -0500
Subject: [PATCH] services: setuid: More specific setuid support.

New record <setuid-program> with fields for setting the specific user and
group, as well as specifically selecting the setuid and setgid bits, for a
program within the setuid-program-service.

* gnu/services.scm (<setuid-program>): New record type.
  (setuid-program, make-setuid-program, setuid-program?)
  (setuid-program-program, stuid-program-setuid?, setuid-program-setgid?)
  (setuid-program-user, setuid-program-group): New variables, export them.
  (setuid-program-entry): New variable, a procedure used for the
  service-extension of activation-service-type as set up by
  setuid-program-service-type.  Unpacks the <setuid-program> record,
  handing off within the gexp to activate-setuid-programs.
  (setuid-program-service-type): Make use of setuid-program-entry.
* gnu/build/activation.scm (activate-setuid-programs): Update to expect a
  ftagged list for each program entry, pre-unpacked from the <setuid-program>
  record before being handed to this procedure.
---
 gnu/build/activation.scm | 40 ++++++++++++++++----------------
 gnu/services.scm         | 49 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 67 insertions(+), 22 deletions(-)

diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index 4b67926e88..a2bdfd5aa5 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -229,13 +229,6 @@ they already exist."
 (define (activate-setuid-programs programs)
   "Turn PROGRAMS, a list of file names, into setuid programs stored under
 %SETUID-DIRECTORY."
-  (define (make-setuid-program prog)
-    (let ((target (string-append %setuid-directory
-                                 "/" (basename prog))))
-      (copy-file prog target)
-      (chown target 0 0)
-      (chmod target #o6555)))
-
   (format #t "setting up setuid programs in '~a'...~%"
           %setuid-directory)
   (if (file-exists? %setuid-directory)
@@ -247,18 +240,27 @@ they already exist."
                          string<?))
       (mkdir-p %setuid-directory))
 
-  (for-each (lambda (program)
-              (catch 'system-error
-                (lambda ()
-                  (make-setuid-program program))
-                (lambda args
-                  ;; If we fail to create a setuid program, better keep going
-                  ;; so that we don't leave %SETUID-DIRECTORY empty or
-                  ;; half-populated.  This can happen if PROGRAMS contains
-                  ;; incorrect file names: <https://bugs.gnu.org/38800>.
-                  (format (current-error-port)
-                          "warning: failed to make '~a' setuid-root: ~a~%"
-                          program (strerror (system-error-errno args))))))
+  (for-each (match-lambda
+              [('setuid-program src-path setuid? setgid? uid gid)
+               (catch 'system-error
+                 (lambda ()
+                   (let ((target (string-append %setuid-directory
+                                                "/" (basename src-path)))
+                         (mode (+ #o0555                   ; base permissions
+                                  (if setuid? #o4000 0)    ; setuid bit
+                                  (if setgid? #o2000 0)))) ; setgid bit
+                     (copy-file src-path target)
+                     (chown target uid gid)
+                     (chmod target mode)))
+                 (lambda args
+                   ;; If we fail to create a setuid program, better keep going
+                   ;; so that we don't leave %SETUID-DIRECTORY empty or
+                   ;; half-populated.  This can happen if PROGRAMS contains
+                   ;; incorrect file names: <https://bugs.gnu.org/38800>.
+                   (format (current-error-port)
+                           "warning: failed to make '~a' setuid-root: ~a~%"
+                           (setuid-program-program program)
+                           (strerror (system-error-errno args)))))])
             programs))
 
 (define (activate-special-files special-files)
diff --git a/gnu/services.scm b/gnu/services.scm
index 4b30399adc..7e03808489 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -87,6 +87,14 @@
             ambiguous-target-service-error-service
             ambiguous-target-service-error-target-type
 
+            setuid-program
+            setuid-program?
+            setuid-program-program
+            setuid-program-setuid?
+            setuid-program-setgid?
+            setuid-program-user
+            setuid-program-group
+
             system-service-type
             provenance-service-type
             sexp->system-provenance
@@ -773,13 +781,48 @@ directory."
 FILES must be a list of name/file-like object pairs."
   (service etc-service-type files))
 
+(define-record-type* <setuid-program> setuid-program make-setuid-program
+  setuid-program?
+  ;; Path to program to link with setuid permissions
+  (program       setuid-program-program)          ;string
+  ;; Whether to set user setuid bit
+  (setuid?       setuid-program-setuid?           ;boolean
+                 (default #t))
+  ;; Whether to set user setgid bit
+  (setgid?       setuid-program-setgid?           ;boolean
+                 (default #t))
+  ;; The user this should be set to (defaults to root)
+  (user          setuid-program-user              ;integer
+                 (default 0))
+  ;; Group we want to set this to (defaults to root)
+  (group         setuid-program-group             ;integer
+                 (default 0)))
+
+(define (setuid-program-entry programs)
+  #~(activate-setuid-programs
+     ;; convert into a tagged list structure as expected by
+     ;; activate-setuid-programs
+     (list #$@(map (match-lambda
+                     [(? setuid-program? sp)
+                      #~(list 'setuid-program
+                              #$(setuid-program-program sp)
+                              #$(setuid-program-setuid? sp)
+                              #$(setuid-program-setgid? sp)
+                              #$(setuid-program-user sp)
+                              #$(setuid-program-group sp))]
+                     ;; legacy, non-<setuid-program> structure
+                     [program
+                      ;; TODO: Spit out a warning here?
+                      #~(list 'setuid-program
+                              #$program
+                              #t #t 0 0)])
+                   programs))))
+
 (define setuid-program-service-type
   (service-type (name 'setuid-program)
                 (extensions
                  (list (service-extension activation-service-type
-                                          (lambda (programs)
-                                            #~(activate-setuid-programs
-                                               (list #$@programs))))))
+                                          setuid-program-entry)))
                 (compose concatenate)
                 (extend append)
                 (description
-- 
2.29.1