[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[maxim.cournoyer]

[-- Attachment #1.1: Type: text/plain, Size: 708 bytes --]

Hello!

The following patches provide a mean to specify a user and group for a
setuid program, and uses that to fix a setgid permission issue in the
context of the opensmtpd service.

Christopher, you should be able to leverage this new facility to
configure the uid/gid of the sendmail program to that of the smtpq user,
like this:

--8<---------------cut here---------------start------------->8---
(operating-system)
  [...]
  (setuid-programs (cons (list (file-append sendmail "/usr/sbin/sendmail") "smtpq")
                           %setuid-programs))
--8<---------------cut here---------------end--------------->8---

The smtpq user is created as part of the OpenSMTPD service definition.

Thank you,


[-- Attachment #1.2: 0001-services-Allow-configuring-the-ownership-of-setuid-p.patch --]
[-- Type: text/x-patch, Size: 7620 bytes --]

From e1b8840da16fb531f6607892ebf08f2d5472b962 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sun, 7 Jun 2020 23:01:49 -0400
Subject: [PATCH 1/3] services: Allow configuring the ownership of setuid
 programs.

Fixes <http://issues.guix.info/41485>.

* gnu/build/activation.scm (activate-setuid-programs): Update doc.  Allow a
program entry to be a list that may include a user and a group.
[make-setuid-program] New USER and GROUP keyword parameters.  Move the error
handling inside the MAKE-SETUID-PROGRAM helper procedure.
* gnu/services.scm (setuid-program-service-type): Update doc.
* doc/guix.texi (Setuid Programs): Update doc.
---
 doc/guix.texi            | 17 +++++++++++---
 gnu/build/activation.scm | 48 +++++++++++++++++++++++++---------------
 gnu/services.scm         | 17 ++++++++++++--
 3 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 056bf011f6..83d7344bd8 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -26429,14 +26429,25 @@ should be setuid root.
 
 The @code{setuid-programs} field of an @code{operating-system}
 declaration contains a list of G-expressions denoting the names of
-programs to be setuid-root (@pxref{Using the Configuration System}).
-For instance, the @command{passwd} program, which is part of the Shadow
-package, can be designated by this G-expression (@pxref{G-Expressions}):
+programs to be setuid (@pxref{Using the Configuration System}).  The
+user and group ownership of the setuid program default to @code{root},
+but can be specified by declaring them along the file name of the
+program.  For instance, the @command{passwd} program, which is part of
+the Shadow package, can be designated as a setuid-root porgram by this
+G-expression (@pxref{G-Expressions}):
 
 @example
 #~(string-append #$shadow "/bin/passwd")
 @end example
 
+As a second example, the @command{smtpctl} program, which is part of the
+OpenSMTPD package, requires to have its group set to @samp{smtpq}.
+This can be specified using:
+
+@example
+(list (file-append opensmtpd "/bin/smtpctl") "smtpq" "smtpq")
+@end example
+
 A default set of setuid programs is defined by the
 @code{%setuid-programs} variable of the @code{(gnu system)} module.
 
diff --git a/gnu/build/activation.scm b/gnu/build/activation.scm
index 30f5e87d5a..6be3664d44 100644
--- a/gnu/build/activation.scm
+++ b/gnu/build/activation.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2015 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -227,14 +228,28 @@ they already exist."
   "/run/setuid-programs")
 
 (define (activate-setuid-programs programs)
-  "Turn PROGRAMS, a list of file names, into setuid programs stored under
-%SETUID-DIRECTORY."
-  (define (make-setuid-program prog)
+  "Turn PROGRAMS, a list of file names and/or of nested lists composed of a
+file name, a user and a group, into setuid programs stored under
+%SETUID-DIRECTORY.  The user and group default to \"root\" and affect the
+ownership of the associated file name."
+  (define* (make-setuid-program prog #:key (user "root") (group user))
     (let ((target (string-append %setuid-directory
                                  "/" (basename prog))))
-      (copy-file prog target)
-      (chown target 0 0)
-      (chmod target #o6555)))
+      (catch 'system-error
+        (lambda ()
+          (let ((uid (passwd:uid (getpwnam user)))
+                (gid (group:gid (getgrnam group))))
+            (copy-file prog target)
+            (chown target uid gid)
+            (chmod target #o6555)))
+        (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 (~a:~a): ~a~%"
+                  prog user group (strerror (system-error-errno args)))))))
 
   (format #t "setting up setuid programs in '~a'...~%"
           %setuid-directory)
@@ -247,18 +262,15 @@ 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
+              ((program user group)
+               (make-setuid-program program #:user user #:group group))
+              ((program user)
+               (make-setuid-program program #:user user))
+              ((program)
+               (make-setuid-program program))
+              (program
+               (make-setuid-program program)))
             programs))
 
 (define (activate-special-files special-files)
diff --git a/gnu/services.scm b/gnu/services.scm
index 2e4648bf78..19a1c38ceb 100644
--- a/gnu/services.scm
+++ b/gnu/services.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2016 Chris Marusich <cmmarusich@gmail.com>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -681,12 +682,24 @@ FILES must be a list of name/file-like object pairs."
                  (list (service-extension activation-service-type
                                           (lambda (programs)
                                             #~(activate-setuid-programs
-                                               (list #$@programs))))))
+                                               (quote (#$@programs)))))))
                 (compose concatenate)
                 (extend append)
                 (description
                  "Populate @file{/run/setuid-programs} with the specified
-executables, making them setuid-root.")))
+executables, making them setuid.  The PROGRAMS entries extending the
+setuid-program-service-type is a list of file-like objects.  Alternatively to
+file-like objects, nested lists containing a file-like object, a user and a
+group can be used to control the ownership of the associated file.
+
+Example:
+
+(list (file-append shadow \"/bin/passwd\")
+      (list (file-append opensmtpd \"/bin/smtpctl\") \"root\" \"smtpq\"))
+
+The @command{passwd} program has both its user and group set to the
+default \"root\" while the @command{smtpctl} program has its user set to
+\"root\" and its group set to \"smtpq\".")))
 
 (define (packages->profile-entry packages)
   "Return a system entry for the profile containing PACKAGES."
-- 
2.26.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-services-opensmtpd-Remove-unused-binding.patch --]
[-- Type: text/x-patch, Size: 1538 bytes --]

From 01c1ab83bf6f5a8158a993de2fa0048f6d172a73 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sun, 7 Jun 2020 23:49:25 -0400
Subject: [PATCH 2/3] services: opensmtpd: Remove unused binding.

* gnu/services/mail.scm (opensmtpd-activation): Remove unused SMTPD variable
binding.
---
 gnu/services/mail.scm | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index cfcaf4601b..7c49d99e9f 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1665,15 +1665,14 @@ match from local for any action outbound
 (define opensmtpd-activation
   (match-lambda
     (($ <opensmtpd-configuration> package config-file)
-     (let ((smtpd (file-append package "/sbin/smtpd")))
-       #~(begin
-           (use-modules (guix build utils))
-           ;; Create mbox and spool directories.
-           (mkdir-p "/var/mail")
-           (mkdir-p "/var/spool/smtpd")
-           (chmod "/var/spool/smtpd" #o711)
-           (mkdir-p "/var/spool/mail")
-           (chmod "/var/spool/mail" #o711))))))
+     #~(begin
+         (use-modules (guix build utils))
+         ;; Create mbox and spool directories.
+         (mkdir-p "/var/mail")
+         (mkdir-p "/var/spool/smtpd")
+         (chmod "/var/spool/smtpd" #o711)
+         (mkdir-p "/var/spool/mail")
+         (chmod "/var/spool/mail" #o711)))))
 
 (define %opensmtpd-pam-services
   (list (unix-pam-service "smtpd")))
-- 
2.26.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: 0003-services-opensmtpd-Fix-the-setgid-problem-for-the-sm.patch --]
[-- Type: text/x-patch, Size: 1702 bytes --]

From 52a1a031e6a7c0196cf17d0bd32061d02b453df8 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sun, 7 Jun 2020 23:52:00 -0400
Subject: [PATCH 3/3] services: opensmtpd: Fix the setgid problem for the
 smtpctl utility.

The utility was complaining that it wasn't setgid to the group ID of the
"smtpq" group.

* gnu/services/mail.scm (opensmtpd-service-type): Extend the
setuid-program-service-type with the smtpctl program.
---
 gnu/services/mail.scm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gnu/services/mail.scm b/gnu/services/mail.scm
index 7c49d99e9f..96efbd951d 100644
--- a/gnu/services/mail.scm
+++ b/gnu/services/mail.scm
@@ -1662,6 +1662,11 @@ match from local for any action outbound
          (home-directory "/var/empty")
          (shell (file-append shadow "/sbin/nologin")))))
 
+(define (opensmtpd-setuid-programs opensmtpd-configuration)
+  (let ((smtpctl (file-append (opensmtpd-configuration-package
+                               opensmtpd-configuration) "/sbin/smtpctl")))
+    (list (list smtpctl "smtpq"))))
+
 (define opensmtpd-activation
   (match-lambda
     (($ <opensmtpd-configuration> package config-file)
@@ -1683,6 +1688,8 @@ match from local for any action outbound
    (extensions
     (list (service-extension account-service-type
                              (const %opensmtpd-accounts))
+          (service-extension setuid-program-service-type
+                             opensmtpd-setuid-programs)
           (service-extension activation-service-type
                              opensmtpd-activation)
           (service-extension pam-root-service-type
-- 
2.26.2


[-- Attachment #1.5: Type: text/plain, Size: 7 bytes --]


Maxim

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]


maxim.cournoyer@gmail.com writes:

> The following patches provide a mean to specify a user and group for a
> setuid program, and uses that to fix a setgid permission issue in the
> context of the opensmtpd service.
>
> Christopher, you should be able to leverage this new facility to
> configure the uid/gid of the sendmail program to that of the smtpq user,
> like this:
>
> --8<---------------cut here---------------start------------->8---
> (operating-system)
>   [...]
>   (setuid-programs (cons (list (file-append sendmail "/usr/sbin/sendmail") "smtpq")
>                            %setuid-programs))
> --8<---------------cut here---------------end--------------->8---
>
> The smtpq user is created as part of the OpenSMTPD service definition.
>
> Thank you,
>
>
> Maxim

Well, thank you for looking in to this Maxim. I've had a brief look
through the patches, although I don't know enough about this area to
comment properly on them.

I wonder if it's worth using a record type to make it possible to pass
the user and group values to the service. That would probably result in
more readable configuration than just using a list of varying length.

Specifically on the diff:

- (list #$@programs))))))
+ (quote (#$@programs)))))))

This change here will mean that you can't pass some values in, as they
won't be evaluated. #~(string-append sendmail "/usr/sbin/sendmail")
would no longer work for example.

Thanks again,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 962 bytes --]

[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[Brice Waegeneire]

Hello Maxim,

Thank you for the patchset!

maxim.cournoyer@gmail.com writes:

> The following patches provide a mean to specify a user and group for a
> setuid program, and uses that to fix a setgid permission issue in the
> context of the opensmtpd service.

I applied it to try to use wireshark as non-root[0]:

--8<---------------cut here---------------start------------->8---
(simple-service 'wireshark-group account-service-type
                   (list (user-group (name "wireshark") (system? #t))))
(simple-service 'wireshark-dumpcap setuid-program-service-type
                   (list (list (file-append wireshark "/bin/dumpcap")
                               "root" "wireshark")))
--8<---------------cut here---------------end--------------->8---

And unfortunately the first run of “guix reconfigure“ failed to make
“dumpcap“ as a setuid, but subsequent run succeeded:

--8<---------------cut here---------------start------------->8---
[…]
setting up setuid programs in '/run/setuid-programs'...
warning: failed to make '/gnu/store/vdlk9rli5k5svy8p7bhf90ln03ybnxgj-wireshark-3.2.4/bin/dumpcap' setuid (root:wireshark): Success
populating /etc from /gnu/store/hxjyvg80zjaxfynjyk3jgqsn9249azmx-etc...
[…]
--8<---------------cut here---------------end--------------->8---

I guess it's because at first there wasn't a wireshark group on my
system, adding the group and the setuid program was done in the same
run, but “setting up setuid programs” is done before “populating /etc”
(comprising /etc/passwd) which in effect ended up trying to setuid
“dumpcap“ before the “wireshark“ group exists. And subsequent runs
succeeded creating a setuid “dumpcap” because the new group was already
on the system, it was created during the first run.

Populating /etc before setting up /run/setuid-programs should fix that
issue but maybe there is reason behind the current order of execution.

> Christopher, you should be able to leverage this new facility to
> configure the uid/gid of the sendmail program to that of the smtpq user,
> like this:
>
> (operating-system)
>   [...]
>   (setuid-programs (cons (list (file-append sendmail "/usr/sbin/sendmail") "smtpq")
>                            %setuid-programs))
>

Aside from that I wonder if specifying user and group in a list is
future proof, maybe using a record would be more Guixy. In particular I
would like to be able to set capabilities (as with “setcap“) on binaries
since the store don't support it[1]; if that's even possible but it's an
other issue.

[0]: https://wiki.wireshark.org/CaptureSetup/CapturePrivileges#Most_UNIXes
[1]: https://lists.gnu.org/archive/html/help-guix/2016-11/msg00046.html

- Brice

[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[Jonathan Brielmaier]

It's http://issues.guix.gnu.org/41763.

What does us block from merging this? It hits me hard when using OpenSMTPD.

[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

Jonathan Brielmaier 写道：
> What does us block from merging this?

Reading [0], Chris & Brice bring up two good points that I don't 
see addressed: using a record instead of a list & not breaking 
gexps, although fixing one would probably moot the other.

Kind regards,

T G-R

[0]: http://issues.guix.gnu.org/41763

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

bug#41763: services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[Maxim Cournoyer]

Hello,

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> Jonathan Brielmaier 写道：
>> What does us block from merging this?
>
> Reading [0], Chris & Brice bring up two good points that I don't see
> addressed: using a record instead of a list & not breaking gexps,
> although fixing one would probably moot the other.
>
> Kind regards,
>
> T G-R
>
> [0]: http://issues.guix.gnu.org/41763

Closing in favor of https://issues.guix.gnu.org/44700.

Thanks,

Maxim

[bug#41763] services: opensmtpd: Fix the setgid problem for the smtpctl utility.

[Tobias Geerinckx-Rice via Guix-patches via]

> Closing in favor of https://issues.guix.gnu.org/44700.

Yes please.  Thanks.

T G-R

Sent from a Web browser. Excuse or enjoy my brevity.