diff mbox series

[bug#73925] add access control to daemon socket in shepherd service

Message ID 87wmhvhgg7.fsf@russelstein.xyz
State New
Headers show
Series [bug#73925] add access control to daemon socket in shepherd service | expand

Commit Message

Reepca Russelstein Oct. 26, 2024, 12:10 a.m. UTC 
Ludovic Courtès <ludo@gnu.org> writes:

>> +                  ;; Ensure that a fresh directory is used, in case the old
>> +                  ;; one was more permissive and processes have a file
>> +                  ;; descriptor referencing it hanging around, ready to use
>> +                  ;; with openat.
>> +                  (false-if-exception
>> +                   (delete-file-recursively "/var/guix/daemon-socket"))
>> +                  (let ((perms #$(logand socket-directory-perms
>> +                                         (lognot #o022))))
>> +                    (mkdir "/var/guix/daemon-socket" perms)
>> +                    ;; Override umask
>> +                    (chmod "/var/guix/daemon-socket" perms))
>
> Speaking of ‘openat’, maybe use ‘mkdir-p/perms’ instead of doing it in
> two steps?

PERMS is passed directly to mkdir; the umask may cause the permissions
the directory is created with to be less permissive than those, but
never more.  The only reason I call chmod here is because the umask may
happen to be more strict than PERMS.  mkdir-p/perms creates the
directory with the permissions initially restricted only by the umask,
then later chmods it in a separate step, leaving a window during which
the directory is likely world-executable and world-readable.  So while
mkdir-p/perms would be an improvement on the "make sure no components
are symlinks" front, it would be a downgrade in restricting access to
the directory.

This behavior can be remedied by ensuring that the final call to
'mkdirat' passes in the specified permission bits.  I've submitted a
patch to do this in issue #74002.

There's also a minor annoyance in that the 'owner' argument that
mkdir-p/perms wants MUST be a passwd object.  This means that the uid
and gid to use can't be specified independently, nor can they be
specified as -1 or 0, you *have* to do (getpwnam "root") or something
similar.

For now I'm going to keep this part as-is, since currently using
mkdir-p/perms would neither make it more secure nor more concise.

The attached patch incorporates all the other changes you've mentioned.

- reepca
diff mbox series

Patch

From b8ea0288a35c27912580bd7fe861dd6e497f4c33 Mon Sep 17 00:00:00 2001
Message-ID: <b8ea0288a35c27912580bd7fe861dd6e497f4c33.1729840060.git.reepca@russelstein.xyz>
From: Reepca Russelstein <reepca@russelstein.xyz>
Date: Sat, 19 Oct 2024 22:43:27 -0500
Subject: [PATCH] services: guix-configuration: add access control to daemon
 socket.

* gnu/services/base.scm
  (guix-configuration-socket-directory-{permissions,group,user}): new fields.
  (guix-shepherd-service): use them.
* doc/guix.texi: document them.

Change-Id: I8f4c2e20392ced47c09812e62903c87cc0f4a97a
---
 doc/guix.texi         | 12 ++++++++++++
 gnu/services/base.scm | 38 ++++++++++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index cb758f9005..fb750bd449 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -19775,6 +19775,18 @@  Base Services
 Environment variables to be set before starting the daemon, as a list of
 @code{key=value} strings.
 
+@item @code{socket-directory-permissions} (default: @code{#o755})
+Permissions to set for the directory @file{/var/guix/daemon-socket}.
+This, together with @code{socket-directory-group} and
+@code{socket-directory-user}, determines who can connect to the build
+daemon via its Unix socket.  TCP socket operation is unaffected by
+these.
+
+@item @code{socket-directory-user} (default: @code{#f})
+@itemx @code{socket-directory-group} (default: @code{#f})
+User and group owning the @file{/var/guix/daemon-socket} directory or
+@code{#f} to keep the user or group as root.
+
 @end table
 @end deftp
 
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index fd2cc9d17a..0bd60c5eb5 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -1880,7 +1880,14 @@  (define-record-type* <guix-configuration>
   (build-machines   guix-configuration-build-machines ;list of gexps | '()
                     (default '()))
   (environment      guix-configuration-environment  ;list of strings
-                    (default '())))
+                    (default '()))
+  (socket-directory-permissions
+   guix-configuration-socket-directory-permissions
+   (default #o755))
+  (socket-directory-group guix-configuration-socket-directory-group
+                          (default #f))
+  (socket-directory-user guix-configuration-socket-directory-user
+                         (default #f)))
 
 (define %default-guix-configuration
   (guix-configuration))
@@ -1944,7 +1951,9 @@  (define (guix-shepherd-service config)
     (guix build-group build-accounts authorize-key? authorized-keys
           use-substitutes? substitute-urls max-silent-time timeout
           log-compression discover? extra-options log-file
-          http-proxy tmpdir chroot-directories environment)
+          http-proxy tmpdir chroot-directories environment
+          socket-directory-permissions socket-directory-group
+          socket-directory-user)
     (list (shepherd-service
            (documentation "Run the Guix daemon.")
            (provision '(guix-daemon))
@@ -1954,11 +1963,13 @@  (define (guix-shepherd-service config)
                           shepherd-discover-action))
            (modules '((srfi srfi-1)
                       (ice-9 match)
-                      (gnu build shepherd)))
+                      (gnu build shepherd)
+                      (guix build utils)))
            (start
             (with-imported-modules `(((guix config) => ,(make-config.scm))
                                      ,@(source-module-closure
-                                        '((gnu build shepherd))
+                                        '((gnu build shepherd)
+                                          (guix build utils))
                                         #:select? not-config?))
               #~(lambda args
                   (define proxy
@@ -1969,7 +1980,26 @@  (define (guix-shepherd-service config)
                   (define discover?
                     (or (getenv "discover") #$discover?))
 
+                  (mkdir-p "/var/guix")
+                  ;; Ensure that a fresh directory is used, in case the old
+                  ;; one was more permissive and processes have a file
+                  ;; descriptor referencing it hanging around, ready to use
+                  ;; with openat.
+                  (false-if-exception
+                   (delete-file-recursively "/var/guix/daemon-socket"))
+                  (let ((perms #$(logand socket-directory-permissions
+                                         (lognot #o022))))
+                    (mkdir "/var/guix/daemon-socket" perms)
+                    ;; Override umask
+                    (chmod "/var/guix/daemon-socket" perms))
+
+                  (let* ((user #$socket-directory-user)
+                         (uid (if user (passwd:uid (getpwnam user)) -1))
+                         (group #$socket-directory-group)
+                         (gid (if group (group:gid (getgrnam group)) -1)))
+                    (chown "/var/guix/daemon-socket" uid gid))
+
                   ;; Start the guix-daemon from a container, when supported,
                   ;; to solve an installation issue. See the comment below for
                   ;; more details.

-- 
2.45.2