x11 none vs disable-X11.inc

[kmk3]

Why is there both x11 none and disable-X11.inc (#4462)?

Should currently both always be used together?

From src/firejail/x11.c (the function is called with x11 none):

void x11_block(void) {
#ifdef HAVE_X11
	// check abstract socket presence and network namespace options
	if ((!arg_nonetwork && !arg_netns && !cfg.bridge0.configured && !cfg.interface0.configured)
	&& x11_abstract_sockets_present()) {
		fprintf(stderr, "ERROR: --x11=none specified, but abstract X11 socket still accessible.\n"
			"Additional setup required. To block abstract X11 socket you can either:\n"
			" * use network namespace in firejail (--net=none, --net=...)\n"
			" * add \"-nolisten local\" to xserver options\n"
			"   (eg. to your display manager config, or /etc/X11/xinit/xserverrc)\n");
		exit(1);
	}

	// blacklist sockets
	char *cmd = strdup("blacklist /tmp/.X11-unix");
	if (!cmd)
		errExit("strdup");
	profile_check_line(cmd, 0, NULL);
	profile_add(cmd);

	// blacklist .Xauthority
	cmd = strdup("blacklist ${HOME}/.Xauthority");
	if (!cmd)
		errExit("strdup");
	profile_check_line(cmd, 0, NULL);
	profile_add(cmd);
	const char *xauthority = env_get("XAUTHORITY");
	if (xauthority) {
		char *line;
		if (asprintf(&line, "blacklist %s", xauthority) == -1)
			errExit("asprintf");
		profile_check_line(line, 0, NULL);
		profile_add(line);
	}

	// clear environment
	env_store("DISPLAY", RMENV);
	env_store("XAUTHORITY", RMENV);
#endif
}

disable-X11.inc:

# This file is overwritten during software install.
# Persistent customizations should go in a .local file.
include disable-X11.local

blacklist /tmp/.X11-unix
blacklist ${HOME}/.Xauthority
blacklist ${RUNUSER}/gdm/Xauthority
blacklist ${RUNUSER}/.mutter-Xwaylandauth*
blacklist ${RUNUSER}/xauth_*
#blacklist ${RUNUSER}/[[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]]-[[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]]-[[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]]-[[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]]-[[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]][[:xdigit:]]
blacklist /tmp/xauth*
blacklist /tmp/.ICE-unix
blacklist ${RUNUSER}/ICEauthority
rmenv DISPLAY
rmenv XAUTHORITY

Other than the common paths, it looks like disable-X11.inc blocks some extra
files in ${RUNUSER} and in /tmp, while x11 none (i.e.: arg_x11_block)
blocks the path stored in the $XAUTHORITY env var and also prevents
~/.Xauthority from being bind-mounted. From src/firejail/fs_home.c:

static int store_xauthority(void) {
	EUID_ASSERT();
	if (arg_x11_block)
		return 0;

	// put a copy of .Xauthority in XAUTHORITY_FILE
	char *dest = RUN_XAUTHORITY_FILE;
	char *src;
	if (asprintf(&src, "%s/.Xauthority", cfg.homedir) == -1)
		errExit("asprintf");

	struct stat s;
	if (lstat(src, &s) == 0) {
		if (S_ISLNK(s.st_mode)) {
			fwarning("invalid .Xauthority file\n");
			free(src);
			return 0;
		}

		// create an empty file as root, and change ownership to user
		EUID_ROOT();
		FILE *fp = fopen(dest, "we");
		if (fp) {
			fprintf(fp, "\n");
			SET_PERMS_STREAM(fp, getuid(), getgid(), 0600);
			fclose(fp);
		}
		else
			errExit("fopen");
		EUID_USER();

		copy_file_as_user(src, dest, 0600); // regular user
		selinux_relabel_path(dest, src);
		fs_logger2("clone", dest);
		free(src);
		return 1; // file copied
	}

	free(src);
	return 0;
}

Shouldn't at least the blacklisting be centralized in one place?

Misc: I noticed this on #4841.

Cc: @rusty-snake

[rusty-snake]

Originally there was only x11 none to disable X11. The problem with it is that Xorg listens on an abstract socket by default in the most distros and x11 none hard-fails in this case => we can use it only if net none is set (net eth0 can not be used upstream).

firejail/etc/templates/profile.template

Line 177 in b0eb973

# Prefer 'x11 none' instead of 'disable-X11.inc' if 'net none' is set

For all other profiles (cli/tui but inet) we used only blacklist /tmp/.X11-unix. disable-X11.inc goes the next steps and blacklists also known Xauthority files, ICE socket, ... .

---

This should make x11 none work always:

-		fprintf(stderr, "ERROR: --x11=none specified, but abstract X11 socket still accessible.\n"
+		fprintf(stderr, "WARN: --x11=none specified, but abstract X11 socket still accessible.\n"
			"Additional setup required. To block abstract X11 socket you can either:\n"
			" * use network namespace in firejail (--net=none, --net=...)\n"
			" * add \"-nolisten local\" to xserver options\n"
			"   (eg. to your display manager config, or /etc/X11/xinit/xserverrc)\n");
-		exit(1);

[kmk3]

@rusty-snake

I noticed that many profiles contain blacklist /tmp/.X11-unix even though
disable-X11.inc exists and seems to have the same purpose (while handling
some extra related files).

Thoughts on replacing the former with the latter in all profiles?

Also for consistency, how about adding disable-X11.inc on all profiles that
contain x11 none? Some of them also have blacklist /tmp/.X11-unix.

[rusty-snake]

I noticed that many profiles contain blacklist /tmp/.X11-unix even though
disable-X11.inc exists and seems to have the same purpose (while handling
some extra related files).

Thoughts on replacing the former with the latter in all profiles?

Sounds the right way to go.

Also for consistency, how about adding disable-X11.inc on all profiles that
contain x11 none?

x11 none is still better I guess.

Some of them also have blacklist /tmp/.X11-unix.

Is redundant.

[kmk3]

I noticed that many profiles contain blacklist /tmp/.X11-unix even though
disable-X11.inc exists and seems to have the same purpose (while handling
some extra related files).
Thoughts on replacing the former with the latter in all profiles?

Sounds the right way to go.

Alright, I'll try to change the profiles.

Also for consistency, how about adding disable-X11.inc on all profiles that
contain x11 none?

x11 none is still better I guess.

I mean to keep x11 none and also add disable-X11.inc (which currently blocks
more files).

Some of them also have blacklist /tmp/.X11-unix.

Is redundant.

Yes and including disable-X11.inc might also be a bit redudant if x11 none is
used, though it seems clearer than having the following:

$ git grep -I '#include disable-X11.inc ' -- etc
etc/profile-a-l/clac.profile:#include disable-X11.inc # x11 none
etc/profile-a-l/daisy.profile:#include disable-X11.inc # x11 none
etc/profile-a-l/gnome-keyring-daemon.profile:#include disable-X11.inc # x11 none

[rusty-snake]

I mean to keep x11 none and also add disable-X11.inc (which currently blocks
more files).

The most paths seem to be known Xauthority paths, where x11 none just looks at $XAUTHORITY IIRC.

[kmk3]

I mean to keep x11 none and also add disable-X11.inc (which currently
blocks more files).

The most paths seem to be known Xauthority paths, where x11 none just looks
at $XAUTHORITY IIRC.

We could also make x11 none include disable-X11.inc (instead of duplicating
the blacklists).

In which case maybe it would be better to keep using the commented version
to avoid duplicate includes:

#include disable-X11.inc # x11 none

Also, thoughts on renaming disable-X11.inc to disable-x11.inc (lowercase)
so that it is sorted last instead of first when using LC_ALL=C?

[kmk3]

PRs created:

• profiles: replace x11 socket blacklist with disable-X11.inc #6286
• profiles: sort blacklist sections #6289
• profiles: rename disable-X11.inc to disable-x11.inc #6294