[bug#65221] [PATCH 0/2] Fix EXTRA-PORTS edge cases

[Ludovic Courtès]

Hi,

ulfvonbelow <striness@tilde.club> skribis:

> EXEC-COMMAND (and, by extension, FORK+EXEC-COMMAND) has several issues:
> 1. Despite it being documented that "all other file descriptors are closed
>    prior to yielding control to COMMAND", this is not currently the case -
>    only other file descriptors that are already marked as FD_CLOEXEC are
>    closed.  For example, if user code happens to have a file descriptor open,
>    for example with call-with-input-file, while EXEC-COMMAND is run, the new
>    process image will inherit that file descriptor.  This may cause some
>    resource waste, but more importantly may cause security issues in certain
>    situations.

Yes.  This has been the case since 0.9.2, as noted in ‘NEWS’:

  Previously, services started indirectly with ‘exec-command’ (which is usually
  the case) would not inherit any file descriptor from shepherd because
  ‘exec-command’ would explicitly close all of them.  However, services started
  with ‘make-system-constructor’ and processes created by some other means, such
  as calling ‘system*’, would inherit some of those descriptors, giving them
  more authority than intended.

  The change here consists in marking all internally-used file descriptors as
  “close-on-exec” (O_CLOEXEC), a feature that’s been available on GNU/Linux and
  GNU/Hurd for years but that so far wasn’t used consistently in shepherd.  This
  is now fixed.  As a side-effect, the file-descriptor-closing loop in
  ‘exec-command’ is now gone.

The FD-closing loop was removed on purpose, in
2c0354258047133db8b885bcc11afdf0def5d885.

Now, as you write, it means that service writers must be careful now and
not leave any non-CLOEXEC file descriptor behind them.

At the time I audited Guix System to check that this was a reasonable
thing to expect and that we could indeed ensure no file descriptors were
leaked.  There’s also ‘tests/close-on-exec.sh’.

If you found cases where it would be necessary, what we could do is have
‘shepherd’ replace ‘call-with-input-file’ & co. with a variant that
opens files as O_CLOEXEC by default.  WDYT?

> 2. EXTRA-PORTS is only honored when either LOG-PORT or LOG-FILE is passed.  I
>    have no idea why this is the case, it isn't documented anywhere, and it
>    isn't intuitive.

#:extra-ports wasn’t really made to be exposed I guess; it was added for
use by systemd-style services in 965f6b61a473ee57a1fc6ec3ea1ad6e35d596031.

> 3. Even when LOG-PORT or LOG-FILE is passed, EXTRA-PORTS may not work as
>    described, because it copies file descriptor contents in an arbitrary
>    order.  For example, suppose that (map fileno EXTRA-PORTS) is (7 6 5 4 3).
>    If the underlying file originally stored in fd N is represented by F(N), it
>    will assign
>    3 <-- F(7)
>    4 <-- F(6)
>    5 <-- F(5)
>    6 <-- F(6)
>    7 <-- F(7)
>
>    In other words, the copying of earlier FDs in EXTRA-PORTS may overwrite
>    later FDs in EXTRA-PORTS.

Good catch!

Could you make a more minimal patch fixing this specific issue, also
adding a test reproducing the problem being fixed?

Thanks for your work!

Ludo’.