Support scoping abstract unix sockets

[WavyEbuilder]

Closes: #330

[WavyEbuilder]

Flag name is a bit long, but I couldn't think of a shorter one; open to suggestions.

[WavyEbuilder]

Ping

[rusty-snake]

Manpage should be updated too.

[WavyEbuilder]

I noticed some completions in the repo, so I've updated those as well.

[rusty-snake]

tabs vs. spaces

[WavyEbuilder]

How should it be indented then? I noticed that indentation, at least in my editor, in the file seems inconsistent, so I copied the --as-pid-1 section.

[rusty-snake]

you're right it is inconsistent. I just noticed strange indention in GH diff and --cap options looked good.

[smcv]

Please mimic the indentation of the better / more consistent options when adding new options. --cap-add and --cap-drop are better examples to follow.

[smcv]

The commit message should briefly describe why this is here. Presumably the main use-case is to be able to prevent sandboxed processes from accessing your Xorg/Xwayland server's listening socket, as on #330?

Landlock is an optional LSM (although it seems to be on-by-default in at least some distributions, perhaps many distributions - unlike the "big" LSMs AppArmor, SELinux and Smack, which are currently still mutually exclusive, so each distro has to choose no more than one of them to be the default, and if sysadmins are allowed to choose a different "big" LSM then they have to turn off the distro's default). This means we need to consider what will happen if the distro or sysadmin has turned it off, or if the kernel is too old to offer this feature.

At the moment, --scope-abstract-unix-sockets would "fail closed", refusing to start any apps that have that option in their sandboxing parameters. But bwrap is primarily designed to be used by higher-level frameworks like Flatpak, WebKitGTK and Glycin, rather than being used directly by end users, and I suspect that the maintainers of those frameworks would prefer to have a way to "fail open" - having your app run normally, but relying on Xauth to prevent access to the abstract X11 socket, seems like it would be better than nothing.

So do we need an accompanying --scope-abstract-unix-sockets-try option, similar to the relationship between --unshare-cgroup and --unshare-cgroup-try?

[smcv]

What does it mean to "scope" abstract sockets, more precisely? It's OK for the option name and the --help to be brief, but the man page should really give a little more detail about what this means.

If a process outside the sandbox (most commonly Xorg or Xwayland) listens on an abstract Unix socket, does this option prevent processes inside the sandbox from connecting to it, even in the absence of --unshare-net?

Conversely, if a process inside the sandbox listens on an abstract Unix socket, does this option prevent processes outside the sandbox from connecting to it, even in the absence of --unshare-net?

It would probably also be helpful to mention This has the same behaviour as LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET: see <citerefentry><refentrytitle>landlock</refentrytitle><manvolnum>7</manvolnum></citerefentry> for details. but I think at least a brief summary of what this option does is also necessary here.

[WavyEbuilder]

I've included a brief summary of what it means to "scope", but not of what an abstract unix socket is. Would you like me to include that as well, or is it okay as is?

[smcv]

No need to explain what an abstract socket is, especially if you cross-reference to unix(7) which is the canonical description of what they are.

[smcv]

For this initial review pass I've assumed that you didn't change the --help except for re-indenting. I'm leaving this comment as a reminder that someone still needs to check this.

[WavyEbuilder]

Should be a non-issue now that we've changed the option name to be shorter.

[smcv]

Please mimic the indentation of the better / more consistent options when adding new options. --cap-add and --cap-drop are better examples to follow.

[smcv]

.

[reneleonhardt]

@WavyEbuilder Thank you very much for your work! Is your contribution ready to be reviewed again?

[WavyEbuilder]

Forgot about this since holidays. One thing left to do (adding a --scope-abstract-unix-sockets-try option) and it should be ready for review. I'll do this later today or tommorow.

[smcv]

Forgot about this since holidays. One thing left to do (adding a --scope-abstract-unix-sockets-try option) and it should be ready for review. I'll do this later today or tommorow.

Good timing, because if you rebase now that #728 has been merged, CI should work!

[smcv]

(see <citerefentry><refentrytitle>unix</refentrytitle><manvolnum>7</manvolnum></citerefentry>) might be a good thing to mention. Or perhaps it could even be

Scope access to abstract <citerefentry><refentrytitle>unix</refentrytitle><manvolnum>7</manvolnum></citerefentry> sockets. This …

(yeah, I know, Docbook XML is ridiculously verbose but this is what we have)

When writing text that is going to get word-wrapped automatically anyway, it's often good to use "semantic line-breaks": add a line-break in logical positions (like after each sentence) even if the maximum width hasn't yet been reached. That helps to avoid unnecessary diffstat caused by re-wrapping the lines.

It might also be helpful to give the obvious motivating example:

This option will prevent the newly created sandbox from talking to any abstract AF_UNIX sockets,
for example the X11 socket <literal>@/tmp/.X11-unix/X0</literal>,
including …

[smcv]

I'm not sure how to understand the last part of this sentence. It sounds as though you are saying that the sandboxed process(es) can't talk to any abstract Unix socket, even an abstract Unix socket that was itself created by the sandboxed process(es) - and I'd be very surprised if that was true.

Looking at a sufficiently new landlock(7), I see that what it actually says is that it restricts connecting to an abstract UNIX socket created by a process outside the related Landlock domain. That seems more useful.

Putting that together with my other suggestion, perhaps something like:

This option will prevent the newly created sandbox from connecting to abstract AF_UNIX sockets
created outside the sandbox,
for example the X11 socket <literal>@/tmp/.X11-unix/X0</literal>,
even if the network namespace is the same.

[smcv]

"connecting" (as in my suggestion) is a better word than "talking", because connect() is a specific operation that a reader can reason about and use in their security decisions.

[smcv]

So do we need an accompanying --scope-abstract-unix-sockets-try option, similar to the relationship between --unshare-cgroup and --unshare-cgroup-try?

The good news is that this doesn't need such lengthy documentation: it can just say something like "Try to do the same as --scope-abstract-af-unix, but if that isn't possible, continue without that restriction."

[smcv]

Conversely, if a process inside the sandbox listens on an abstract Unix socket, does this option prevent processes outside the sandbox from connecting to it, even in the absence of --unshare-net?

Answering my own question: if I'm reading landlock(7) correctly, processes outside the sandbox would still be able to connect, and the process inside the sandbox would be able to receive those connections. (Which is fine, but I think it's important to be clear about this.)

[WavyEbuilder]

Conversely, if a process inside the sandbox listens on an abstract Unix socket, does this option prevent processes outside the sandbox from connecting to it, even in the absence of --unshare-net?

Answering my own question: if I'm reading landlock(7) correctly, processes outside the sandbox would still be able to connect, and the process inside the sandbox would be able to receive those connections. (Which is fine, but I think it's important to be clear about this.)

Yes this was my findings from a test. Will update the docs

[WavyEbuilder]

So do we need an accompanying --scope-abstract-unix-sockets-try option, similar to the relationship between --unshare-cgroup and --unshare-cgroup-try?

The good news is that this doesn't need such lengthy documentation: it can just say something like "Try to do the same as --scope-abstract-af-unix, but if that isn't possible, continue without that restriction."

I noticed opt_unshare_cgroup_try has "if possible else continue by skipping it" appended to the opt_unshare_group help description, but doing the same for --scope-abstract-unix-sockets-try makes the line unbearably long, so if it's okay, I'm going to reuse this sentence, but slightly shortened, in the --help output too.

[WavyEbuilder]

Should be ready for review again!

[smcv]

This might need some adjustment if there are glibc/kernel combinations where <landlock.h> exists but SYS_landlock_create_ruleset is undefined, but hopefully those don't actually exist in practice. I'll try it in a very old container.

[smcv]

This duplicates the logic for actually applying the restriction between the fail-open and fail-closed branches, which is unfortunate.
I'd prefer to have a function that is always defined, and either applies the restriction, or returns failure with errno and maybe a human-readable message. Something like this:

static bool
scope_abstract_unix_sockets(const char **message_out)
{
#ifdef HAVE_LANDLOCK_H
  if (!(... do the first step))
    {
      *message_out = "failed to check Landlock compatibility";
      return false;
    }

  ... do the remaining steps, similar error handling each time ...

  return true;
#else
  errno = ENOSYS;
  *message_out = "bubblewrap compiled without landlock support";
  return false;
#error
}

In the places that already set errno, you don't need to set it again. In places that fail but don't set errno (I think this is just the if (abi < 6) check right now), you would have to choose some suitable value for errno and set that:

if (abi < 6)
  {
    errno = ENOSYS;
    *message_out = "supported kernel Landlock ABI too old, version 6 or above required";
    return false;
  }

"Function not implemented" seems like a reasonable least-bad representation for "your kernel is too old" (it's also the error code we would get from these syscalls if glibc knows about Landlock but the running kernel does not), but I could also see an argument for using ENOTSUP or EINVAL or even ENOPKG - I'm open to suggestions!

And then its caller can look more like this (untested):

  if (opt_scope_abstract_unix_sockets || opt_scope_abstract_unix_sockets_try)
    {
      const char *message = NULL;

      if (!scope_abstract_unix_sockets (&message))
        {
          if (opt_scope_abstract_unix_sockets)
            die_with_error (message);
          else
            debug ("%s: %s", message, strerror (errno));
        }
    }

(Or the function could return NULL on success and a non-NULL error message on failure, but I think a boolean result is more obvious, even if strictly speaking it's redundant.)

Unfortunately bubblewrap doesn't depend on any useful libraries like GLib or libdbus (by policy, because it's sometimes setuid root, so every dependency adds attack surface), so we have to reinvent error-reporting.

[smcv]

It's worth keeping in mind how this function could be structured so that if we use more Landlock features in future, it can cope with that. For instance we might eventually want to change it from

static bool scope_abstract_unix_sockets(const char **message_out)

to something more like

static bool apply_landlock_restrictions(bool scope_abstract_unix_sockets,
                                        bool apply_some_other_restriction,
                                        ...,
                                        const char **message_out)

But we can cross that bridge when we come to it.

[smcv]

When using Autotools-style HAVE_FOO macros, let's make them consistent with Autotools' systematic naming: the macro for whether we have linux/landlock.h should be named HAVE_LINUX_LANDLOCK_H.

Also please move this below the creation of cdata, so that it can use cdata.set('HAVE_LINUX_LANDLOCK_H', 1) to put it in config.h instead of expanding the compiler command-line.

Many projects do something like this (adapted from https://gitlab.freedesktop.org/dbus/dbus/-/blob/main/meson.build?ref_type=heads, untested):

# Copyright 2019-2020 Salamandar
# Copyright 2022-2026 Collabora Ltd.
# SPDX-License-Identifier: MIT

check_headers = [
    'linux/landlock.h',
]

foreach header : check_headers
    macro = 'HAVE_' + header.underscorify().to_upper()
    cdata.set(macro, cc.check_header(header) ? 1 : false)
endforeach

I agree that it probably it isn't worth doing that yet, but let's choose our naming so that it would be possible to do that in future.