chrootenv: resolve #32802, #32876, #32877, #32878 #32899

lukateras · 2017-12-20T15:39:14Z

Motivation for this change

Show error on chrootenv-inside-chrootenv (chrootenv: detect chrootenv-in-chrootenv scenario #32802)
Show sysctl command when kernel.unpriveleged_userns_clone is available (chrootenv: more helpful error message for Debian users #32876)
Bind-mount all directories and symlink files in / (chrootenv: bind-mount all directories in / #32877)
Replace env_whitelist with env_blacklist (currently empty) (chrootenv: replace environment variable whitelist with a blacklist #32878)

Formatted via clang-format. Valgrind reports "all heap blocks were freed". Steam still works!

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Mic92 · 2017-12-20T22:15:59Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

-    if ((env = getenv(names[i]))) {
-      if (asprintf(ptr++, "%s=%s", names[i], env) < 0)
-        errorf(EX_OSERR, "asprintf");
+#define LEN(x) sizeof(x) / sizeof(*x)


nitpick:

- #define LEN(x) sizeof(x) / sizeof(*x) + #define LEN(x) (sizeof(x) / sizeof(*x))

Mic92 · 2017-12-20T22:19:02Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+char *env_blacklist[] = {};
+
+char **env_filter(char *envp[]) {
+  char **filtered_envp = malloc(sizeof(*envp));


missing check if malloc failed.

Mic92 · 2017-12-20T22:19:06Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+char *env_blacklist[] = {};
+
+char **env_filter(char *envp[]) {
+  char **filtered_envp = malloc(sizeof(*envp));


missing check if malloc failed. Consider to use calloc, zero initialized memory is usually easier to debug.

Mic92 · 2017-12-20T22:24:49Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+    bool blacklisted = false;
+
+    for (size_t i = 0; i < LEN(env_blacklist); i++) {
+      if (!strncmp(*envp, env_blacklist[i], strlen(env_blacklist[i]))) {


What is the gain of strncmp here? You compute strlen first based on env_blacklist[i].

Because I only need to partially match the string. Example of valid entry in env_blacklist would be PWD=, example of *envp that would be blacklisted is PWD=/home/user.

The second parameter should be the index of the first '=' in the string indeed.

@dezgeg I don't think I understand. Could you elaborate?

Sorry, that comment was a brain fart. But consider the case where env_blacklist contains FOO and *envp is FOOBAR.

Yeah, the point was that env_blacklist elements would always end with an equal sign. But unsetenv approach is much better. Also I've discovered environ only today.

Mic92 · 2017-12-20T22:30:13Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+  char **filtered_envp = malloc(sizeof(*envp));
+  size_t n = 0;
+
+  while (*envp != NULL) {


a for loop incrementing envp would be slightly more readable here.

Mic92 · 2017-12-20T23:03:16Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+  if (path == NULL)
+    errorf(EX_OSERR, "malloc");
+
+  if (strcpy(path, dir) < 0)


path is not freed.

It should not be freed, the point is to allocate enough memory to hold combined string to reduce boilerplate. free() happens at other places.

strcpy() can't fail, btw.

Mic92 · 2017-12-20T23:03:23Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+  if (strcpy(path, dir) < 0)
+    errorf(EX_IOERR, "strcpy");
+
+  if (strcat(path, name) < 0)


path is not freed

Neither can strcat().

Mic92 · 2017-12-20T23:04:24Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+
+bool isdir(char *path) {
+  struct stat buf;
+  stat(path, &buf);


Should check return value of stat -> buf could even contain the result of previous values stat calls.

You are right, I'll reimplement this on top of opendir.

Mic92 · 2017-12-20T23:06:22Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+      char *host_target = strjoin("host/", prefix_dirent->d_name);
+
+      if (symlink(host_target, prefix_dirent->d_name) < 0)
+        errorf(EX_IOERR, "symlink");


Directory is not closed and memory is not freed in error case.

I don't understand why this has to be done. I'll read up on it.

I think this should be handled by exit, not manually. Directories, files and memory are handled by default, and if there's something that is not handled in a larger program, atexit callback should be used.

Mic92 · 2017-12-20T23:15:00Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

@@ -170,7 +239,8 @@ int main(int argc, char *argv[]) {
  if (waitpid(cpid, &status, 0) < 0)
    errorf(EX_OSERR, "waitpid");

-  if (nftw(root, nftw_rm, getdtablesize(), FTW_DEPTH | FTW_MOUNT | FTW_PHYS) < 0)
+  if (nftw(root, nftw_rm, getdtablesize(), FTW_DEPTH | FTW_MOUNT | FTW_PHYS) <
+      0)


Write return code to a local variable instead of line wrapping here for readability.

Line wrapping will go away if I rename nftw_rm.

dezgeg · 2017-12-20T23:24:12Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+  while (*envp != NULL) {
+    bool blacklisted = false;
+
+    for (size_t i = 0; i < LEN(env_blacklist); i++) {


~~LEN() returns something completely wrong here~~ Ok it's correct after all.

I don't think so:

$ cat example.c #include <stdio.h> #define LEN(x) sizeof(x) / sizeof(*x) char *zzz[] = {"a", "b", "c"}; int main() { printf("%lu\n", LEN(zzz)); return 0; } $ nix-shell -p gcc gnumake --run 'make example' $ ./example 3

dezgeg · 2017-12-20T23:30:39Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

@@ -161,7 +230,7 @@ int main(int argc, char *argv[]) {

    argv++;

-    if (execvpe(*argv, argv, env_build(env_whitelist, LEN(env_whitelist))) < 0)
+    if (execvpe(*argv, argv, env_filter(environ)) < 0)


I think the code for the blacklist approach would be much simpler if you just used execvp() and just modified the current environment with unsetenv().

I agree, I'm going to do that. Thanks!

dezgeg · 2017-12-20T23:34:30Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+    errorf(EX_OSERR, "mount");
+}
+
+char *strjoin(char *dir, char *name) {


This whole function could just be asprintf(&ret, "%s%s", dir, name)

or in C++: dir + name. ;)

I know it could be asprintf, but it's cleaner this way. Check previous chrootenv version, I've used asprintf.

How is it cleaner to write 13 lines of code to replace 1 line?

It has better semantics.

There is no template, only concatenation, while asprintf has its own DSL.

But every C programmer who reads asprintf(&ret, "%s%s", x, y) immediately knows it's just concatenating two strings.

And in fact you're using this in precisely one place, with a constant string as the other arg. Just

char *host_target = strjoin("host/", prefix_dirent->d_name)

->

char *host_target; asprintf(&host_target, "host/%s", prefix_dirent->d_name);

to make it even more simpler.

OK, point taken. errorf uses the same DSL anyway so it seems there is not much use in implementing simpler asprintf, at least not without dropping errorf.

dezgeg · 2017-12-20T23:35:00Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+  return path;
+}
+
+char *bind_blacklist[] = {".", "..", "bin", "etc", "host", "usr"};


const char * const bind_blacklist[] etc.

dezgeg · 2017-12-20T23:37:24Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

-        errorf(EX_OSERR, "asprintf");
+#define LEN(x) sizeof(x) / sizeof(*x)
+
+char *env_blacklist[] = {};


Isn't this variable totally unused?

Ah it's supposed to be a constant... const char * const then.

It's unused for now.

This variable is supposed to be filled in presumably when @abbradar gets better.

Then I would add a TODO comment.

Mic92 · 2017-12-21T00:32:00Z

pkgs/build-support/build-fhs-userenv/chrootenv.c

+
+  if (setenv("NIX_CHROOTENV", "1", false) < 0)
+    errorf(EX_IOERR, "setenv");
+
  char tmpl[] = "/tmp/chrootenvXXXXXX";
  char *root = mkdtemp(tmpl);


root is not removed in the error case.

* Wrap LEN macro in parantheses * Drop env_filter in favor of stateful environ_blacklist_filter, use execvp instead of execvpe, don't explicitly use environ * Add argument error logging wherever it makes sense * Drop strjoin in favor of asprintf * char* -> const char* where appropriate * Handle stat errors * Print user messages with fputs, not errorf * Abstract away is_str_in (previously bind_blacklisted) * Cleanup temporary directory on error * Some minor syntactic and naming changes Thanks to Jörg Thalheim and Tuomas Tynkkynen for the code review!

lukateras · 2017-12-21T03:50:46Z

@Mic92, @dezgeg: again, thanks for the review. I've fixed most of the problems that you told me about in 08156be (see commit log) except where I explicitly disagree:

I agree that it's preferable to free memory after it has been used but I don't think that memory (or any other resource) should be explicitly handled before exit. Kernel mostly does that for us.
I disagree with @Mic92 in chrootenv: resolve #32802, #32876, #32877, #32878 #32899 (comment): I believe that function name is more informative and better for error reports. However, I've implemented argument logging. Also, now user-targeted messages are printed via fputs and not errorf, i.e. without source filename, line and strerror, and on a separate line. Before:
```
$ ./chrootenv ls
chrootenv.c:100: unshare: run `sudo sysctl -w kernel.unprivileged_userns_clone=1`: operation not permitted
```
After:
```
$ ./chrootenv ls
Run: sudo sysctl -w kernel.unprivileged_userns_clone=1
chrootenv.c:100: unshare: operation not permitted
```
clang-format might have its quirks, but it's a standard way to indent C code. It might handle one line out of 250 rather badly, but on average, it indents code way better than most people do / have time for.

Taken into consideration

lukateras · 2017-12-21T23:49:39Z

I plan to merge this tomorrow.

lukateras added 3 commits December 20, 2017 03:07

chrootenv: print sysctl command for Debian users, fixes NixOS#32876

b3c8c6b

chrootenv: bind-mount all dirs in /, symlink files, closes NixOS#32877

2bc1b50

chrootenv: replace env whitelist with blacklist, closes NixOS#32878

33a9986

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 11-100 labels Dec 20, 2017

chrootenv: error on chrootenv-inside-chrootenv, resolves NixOS#32802

76a3774

lukateras changed the title ~~chrootenv: resolve #32876, #32877, #32878~~ chrootenv: resolve #32802, #32876, #32877, #32878 Dec 20, 2017

lukateras requested a review from Mic92 December 20, 2017 19:33

Mic92 previously requested changes Dec 20, 2017

View reviewed changes

dezgeg reviewed Dec 20, 2017

View reviewed changes

Mic92 reviewed Dec 21, 2017

View reviewed changes

lukateras force-pushed the 20171220.023225/chrootenv branch from c43c660 to 08156be Compare December 21, 2017 03:39

lukateras merged commit 73a0d95 into NixOS:master Dec 22, 2017

srhb mentioned this pull request Dec 27, 2017

chrootenv: breaks when root contains broken symlink #33106

Open

chrootenv: resolve #32802, #32876, #32877, #32878 #32899

chrootenv: resolve #32802, #32876, #32877, #32878 #32899

Conversation

lukateras commented Dec 20, 2017 • edited

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dezgeg Dec 20, 2017 • edited

Choose a reason for hiding this comment

lukateras Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dezgeg Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dezgeg Dec 20, 2017 • edited

Choose a reason for hiding this comment

lukateras Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 21, 2017 • edited

Choose a reason for hiding this comment

lukateras commented Dec 21, 2017 • edited

lukateras commented Dec 21, 2017

lukateras commented Dec 20, 2017 •

edited

dezgeg Dec 20, 2017 •

edited

lukateras Dec 20, 2017 •

edited

lukateras Dec 21, 2017 •

edited

dezgeg Dec 20, 2017 •

edited

dezgeg Dec 20, 2017 •

edited

lukateras Dec 20, 2017 •

edited

lukateras Dec 20, 2017 •

edited

lukateras Dec 21, 2017 •

edited

Mic92 Dec 21, 2017 •

edited

lukateras commented Dec 21, 2017 •

edited