GNU bug report logs - #73925
[PATCH] add access control to daemon socket in shepherd service

Previous Next

Full log

Message #11 received at 73925 <at> debbugs.gnu.org (full text, mbox):

From: Reepca Russelstein <reepca <at> russelstein.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 73925 <at> debbugs.gnu.org
Subject: Re: [bug#73925] [PATCH] add access control to daemon socket in
 shepherd service
Date: Fri, 25 Oct 2024 19:10:32 -0500

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> 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

[0001-services-guix-configuration-add-access-control-to-da.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.