Skip to content

Commit

Permalink
private-etc rework: /etc file groups
Browse files Browse the repository at this point in the history
  • Loading branch information
netblue30 committed Jan 22, 2023
1 parent 8377c54 commit d1124df
Show file tree
Hide file tree
Showing 6 changed files with 205 additions and 149 deletions.
1 change: 1 addition & 0 deletions src/firejail/firejail.h
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,7 @@ void bandwidth_pid(pid_t pid, const char *command, const char *dev, int down, in
void network_set_run_file(pid_t pid);

// fs_etc.c
char *fs_etc_build(char *str);
void fs_resolvconf(void);
void fs_machineid(void);
void fs_private_dir_copy(const char *private_dir, const char *private_run_dir, const char *private_list);
Expand Down
320 changes: 174 additions & 146 deletions src/firejail/fs_etc.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,145 @@
#include <sys/types.h>
#include <time.h>
#include <unistd.h>
#include <dirent.h>
#include <glob.h>

#define ETC_MAX 256
static int etc_cnt = 0;
static char *etc_list[ETC_MAX + 1] = { // plus 1 for ending NULL pointer
"alternatives",
"fonts",
"ld.so.cache",
"ld.so.conf",
"ld.so.conf.d",
"ld.so.preload",
"locale",
"locale.alias",
"locale.conf",

This comment has been minimized.

Copy link
@glitsj16

glitsj16 Jan 22, 2023

Collaborator

Currently locale.gen is not used in any of our profiles using private-etc. Is it really needed? Or is more work following that requires it? In other words, I'm just wondering why a file used to generate locales is allowed in private-etc.

This comment has been minimized.

Copy link
@netblue30

netblue30 Jan 25, 2023

Author Owner

I was wondering what that file is for. Will be removed in the next commit!

"locale.gen",
"localtime",
"nsswitch.conf",
"passwd",
NULL
};

static char*etc_group_network[] = {
"hostname",
"hosts",
"resolv.conf",
"protocols",
NULL
};

static char *etc_group_gnome[] = {
"xdg",
"drirc",
"dconf",
"gtk-2.0",
"gtk-3.0",
NULL
};

static char *etc_group_kde[] = {
"xdg",
"drirc",
"kde4rc",
"kde5rc",
NULL
};

static char *etc_group_sound[] = {
"alsa",
"asound.conf",
"machine-id", // required by PulseAudio
"pulse",
NULL
};

static char *etc_group_tls_ca[] = {
"ca-certificates",

This comment has been minimized.

Copy link
@glitsj16

glitsj16 Jan 22, 2023

Collaborator

Same remark as above. Currently ca-certificates.conf is not used in any of our profiles using private-etc. Is it really needed? Or is more work following that requires it? In other words, I'm just wondering why a file used to filter what goes into /etc/ssl/certs is allowed in private-etc ( ssl being in this group too).

This comment has been minimized.

Copy link
@netblue30

netblue30 Jan 25, 2023

Author Owner

To be removed,thanks!

"ca-certificates.conf",
"crypto-policies",
"pki",
"ssl",
NULL
};

static void etc_copy_group(char **pptr) {
assert(pptr);

while (*pptr != NULL) {
etc_list[etc_cnt++] = *pptr;
etc_list[etc_cnt] = NULL;
pptr++;
}
}

static void etc_add(const char *file) {
assert(file);
if (etc_cnt >= ETC_MAX) {
fprintf(stderr, "Error: size of private_etc list exceeded (%d maximum)\n", ETC_MAX);
exit(1);
}

// look for file in the current list
int i;
for (i = 0; i < etc_cnt; i++) {
if (strcmp(file, etc_list[i]) == 0) {
if (arg_debug)
printf("private-etc arguments: skip %s\n", file);
return;
}
}

char *ptr = strdup(file);
if (!ptr)
errExit("strdup");
etc_list[etc_cnt++] = ptr;
etc_list[etc_cnt] = NULL;
}

// str can be NULL
char *fs_etc_build(char *str) {
while (etc_list[etc_cnt++]);
etc_cnt--;
if (!arg_nonetwork)
etc_copy_group(&etc_group_network[0]);
if (!arg_nosound)
etc_copy_group(&etc_group_sound[0]);

// parsing
if (str) {
char* ptr = strtok(str, ",");
while (ptr) {
// look for standard groups
if (strcmp(ptr, "TLS-CA") == 0)
etc_copy_group(&etc_group_tls_ca[0]);
if (strcmp(ptr, "GNOME") == 0)
etc_copy_group(&etc_group_gnome[0]);
if (strcmp(ptr, "KDE") == 0)
etc_copy_group(&etc_group_kde[0]);
else
etc_add(ptr);
ptr = strtok(NULL, ",");
}
}

// manufacture the new string
int len = 0;
int i;
for (i = 0; i < etc_cnt; i++)
len += strlen(etc_list[i]) + 1; // plus 1 for the trailing ','
char *rv = malloc(len + 1);
if (!rv)
errExit("malloc");
char *ptr = rv;
for (i = 0; i < etc_cnt; i++) {
sprintf(ptr, "%s,", etc_list[i]);
ptr += strlen(etc_list[i]) + 1;
}

return rv;
}

void fs_resolvconf(void) {
if (arg_debug)
Expand Down Expand Up @@ -178,19 +316,11 @@ static int check_dir_or_file(const char *fname) {
}

static void duplicate(const char *fname, const char *private_dir, const char *private_run_dir) {
assert(fname);

if (*fname == '~' || *fname == '/' || strstr(fname, "..")) {
fprintf(stderr, "Error: \"%s\" is an invalid filename\n", fname);
exit(1);
}
invalid_filename(fname, 0); // no globbing

char *src;
if (asprintf(&src, "%s/%s", private_dir, fname) == -1)
errExit("asprintf");

if (check_dir_or_file(src) == 0) {
fwarning("skipping %s for private %s\n", fname, private_dir);
free(src);
return;
}
Expand All @@ -204,8 +334,9 @@ static void duplicate(const char *fname, const char *private_dir, const char *pr

build_dirs(src, dst, strlen(private_dir), strlen(private_run_dir));

// follow links! this will make a copy of the file or directory pointed by the symlink
// follow links by default, thus making a copy of the file or directory pointed by the symlink
// this will solve problems such as NixOS #4887
//
// don't follow links to dynamic directories such as /proc
if (strcmp(src, "/etc/mtab") == 0)
sbox_run(SBOX_ROOT | SBOX_SECCOMP, 3, PATH_FCOPY, src, dst);
Expand All @@ -214,9 +345,38 @@ static void duplicate(const char *fname, const char *private_dir, const char *pr

free(dst);
fs_logger2("clone", src);
free(src);
}

static void duplicate_globbing(const char *fname, const char *private_dir, const char *private_run_dir) {
assert(fname);

if (*fname == '~' || *fname == '/' || strstr(fname, "..")) {
fprintf(stderr, "Error: \"%s\" is an invalid filename\n", fname);
exit(1);
}
invalid_filename(fname, 1); // no globbing

char *pattern;
if (asprintf(&pattern, "%s/%s", private_dir, fname) == -1)
errExit("asprintf");

glob_t globbuf;
int globerr = glob(pattern, GLOB_NOCHECK | GLOB_NOSORT | GLOB_PERIOD, NULL, &globbuf);
if (globerr) {
fprintf(stderr, "Error: failed to glob pattern %s\n", pattern);
exit(1);
}

size_t i;
int len = strlen(private_dir);
for (i = 0; i < globbuf.gl_pathc; i++) {
char *path = globbuf.gl_pathv[i];
duplicate(path + len + 1, private_dir, private_run_dir);
}

globfree(&globbuf);
free(pattern);
}

void fs_private_dir_copy(const char *private_dir, const char *private_run_dir, const char *private_list) {
assert(private_dir);
Expand Down Expand Up @@ -256,10 +416,10 @@ void fs_private_dir_copy(const char *private_dir, const char *private_run_dir, c
fprintf(stderr, "Error: invalid private %s argument\n", private_dir);
exit(1);
}
duplicate(ptr, private_dir, private_run_dir);
duplicate_globbing(ptr, private_dir, private_run_dir);

while ((ptr = strtok(NULL, ",")) != NULL)
duplicate(ptr, private_dir, private_run_dir);
duplicate_globbing(ptr, private_dir, private_run_dir);
free(dlist);
fs_logger_print();
}
Expand Down Expand Up @@ -297,135 +457,3 @@ void fs_private_dir_list(const char *private_dir, const char *private_run_dir, c
fmessage("Private %s installed in %0.2f ms\n", private_dir, timetrace_end());
}

#if 0
void fs_rebuild_etc(void) {
int have_dhcp = 1;
if (cfg.dns1 == NULL && !any_dhcp()) {
// Disabling this option ensures that updates to files using
// rename(2) propagate into the sandbox, in order to avoid
// breaking /etc/resolv.conf (issue #5010).
if (!checkcfg(CFG_ETC_HIDE_BLACKLISTED))
return;
have_dhcp = 0;
}

if (arg_debug)
printf("rebuilding /etc directory\n");
if (mkdir(RUN_DNS_ETC, 0755))
errExit("mkdir");
selinux_relabel_path(RUN_DNS_ETC, "/etc");
fs_logger("tmpfs /etc");

DIR *dir = opendir("/etc");
if (!dir)
errExit("opendir");

struct stat s;
struct dirent *entry;
while ((entry = readdir(dir))) {
if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0)
continue;

// skip files in cfg.profile_rebuild_etc list
// these files are already blacklisted
{
ProfileEntry *prf = cfg.profile_rebuild_etc;
int found = 0;
while (prf) {
if (strcmp(entry->d_name, prf->data + 5) == 0) { // 5 is strlen("/etc/")
found = 1;
break;
}
prf = prf->next;
}
if (found)
continue;
}

// for resolv.conf we might have to create a brand new file later
if (have_dhcp &&
(strcmp(entry->d_name, "resolv.conf") == 0 ||
strcmp(entry->d_name, "resolv.conf.dhclient-new") == 0))
continue;
// printf("linking %s\n", entry->d_name);

char *src;
if (asprintf(&src, "/etc/%s", entry->d_name) == -1)
errExit("asprintf");
if (stat(src, &s) != 0) {
free(src);
continue;
}

char *dest;
if (asprintf(&dest, "%s/%s", RUN_DNS_ETC, entry->d_name) == -1)
errExit("asprintf");

int symlink_done = 0;
if (is_link(src)) {
char *rp =realpath(src, NULL);
if (rp == NULL) {
free(src);
free(dest);
continue;
}
if (symlink(rp, dest))
errExit("symlink");
else
symlink_done = 1;
}
else if (S_ISDIR(s.st_mode))
create_empty_dir_as_root(dest, S_IRWXU);
else
create_empty_file_as_root(dest, S_IRUSR | S_IWUSR);

// bind-mount src on top of dest
if (!symlink_done) {
if (mount(src, dest, NULL, MS_BIND|MS_REC, NULL) < 0)
errExit("mount bind mirroring /etc");
}
fs_logger2("clone", src);

free(src);
free(dest);
}
closedir(dir);

// mount bind our private etc directory on top of /etc
if (arg_debug)
printf("Mount-bind %s on top of /etc\n", RUN_DNS_ETC);
if (mount(RUN_DNS_ETC, "/etc", NULL, MS_BIND|MS_REC, NULL) < 0)
errExit("mount bind mirroring /etc");
fs_logger("mount /etc");

if (have_dhcp == 0)
return;

if (arg_debug)
printf("Creating a new /etc/resolv.conf file\n");
FILE *fp = fopen("/etc/resolv.conf", "wxe");
if (!fp) {
fprintf(stderr, "Error: cannot create /etc/resolv.conf file\n");
exit(1);
}

if (cfg.dns1) {
if (any_dhcp())
fwarning("network setup uses DHCP, nameservers will likely be overwritten\n");
fprintf(fp, "nameserver %s\n", cfg.dns1);
}
if (cfg.dns2)
fprintf(fp, "nameserver %s\n", cfg.dns2);
if (cfg.dns3)
fprintf(fp, "nameserver %s\n", cfg.dns3);
if (cfg.dns4)
fprintf(fp, "nameserver %s\n", cfg.dns4);

// mode and owner
SET_PERMS_STREAM(fp, 0, 0, 0644);

fclose(fp);

fs_logger("create /etc/resolv.conf");
}
#endif
11 changes: 11 additions & 0 deletions src/firejail/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -2044,6 +2044,17 @@ int main(int argc, char **argv, char **envp) {
else if (strcmp(argv[i], "--keep-dev-shm") == 0) {
arg_keep_dev_shm = 1;
}
else if (strcmp(argv[i], "--private-etc") == 0) {
if (checkcfg(CFG_PRIVATE_ETC)) {
if (arg_writable_etc) {
fprintf(stderr, "Error: --private-etc and --writable-etc are mutually exclusive\n");
exit(1);
}
arg_private_etc = 1;
}
else
exit_err_feature("private-etc");
}
else if (strncmp(argv[i], "--private-etc=", 14) == 0) {
if (checkcfg(CFG_PRIVATE_ETC)) {
if (arg_writable_etc) {
Expand Down

7 comments on commit d1124df

@curiosityseeker
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure if this commit is the culprit - but today executing

man firejail

shows the following output:

Error mount: fs_etc.c:196 fs_resolvconf: No such file or directory
Error: proc 274712 cannot sync with peer: unexpected EOF
Peer 274713 unexpectedly exited with status 1

Adding resolv.conf to private-etc fixes it.

@glitsj16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiosityseeker Nice find. I did similar work for some profiles in #5609 but I obviously missed man.profile. Please open a PR for it. We need a regex guru to find all potentially affected profiles :-)

@rusty-snake
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly and resolv.conf should be added to

ALWAYS_REQUIRED=(alternatives ld.so.cache ld.so.preload)

@glitsj16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO no, resolv.conf is not always required. It only needs to be there when DNS resolving is required. So not when a profile has net none or protocol unix (like man.profile has). In fact, for me, man firejail works fine without resolv.conf in private-etc. Not sure why @curiosityseeker needs it though...

@curiosityseeker
Copy link
Collaborator

@curiosityseeker curiosityseeker commented on d1124df Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glitsj16 : Thanks for this hint! Further digging revealed that the culprit was a

dns 127.0.0.1

in my globals.local file (to make sure that every dns request goes through dnscrypt-proxy). Commenting that entry (or adding ignore dns 127.0.0.1 to man.local) fixed the problem - but I'm not sure why.

@glitsj16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiosityseeker Glad to read you've got it working now. Here's my take on why you need to ignore it for the man.profile. If you set a DNS server (or several) for the sandbox, that implies having access to /etc/resolv.conf for DNS resolving to work from inside the sandbox. Because man.profile disables networking (as it should) via net none and protocol unix and hides /etc/resolv.conf by not having it in private-etc, the configured server listening on 127.0.0.1 can't be reached and the sandbox falls apart at this point.

I can see why you'd want to have dns foo in globals.local and IMO it will work fine with most profiles. But there are exceptions so you might have to double-check your profiles/overrides now private-etc is being reworked.

@netblue30
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have new code for resolv.conf. Currently, it is a war zone, so keep an eye on it!

Please sign in to comment.