From 8a7f0dfd68a785d254f7156c4b57c8809eb4bbbe Mon Sep 17 00:00:00 2001 From: Jude Taylor Date: Thu, 12 Nov 2015 19:00:16 -0800 Subject: [PATCH 1/9] use per-derivation sandbox profiles --- src/libstore/build.cc | 61 +++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index f5f91d617..5cd695b1c 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -778,6 +778,8 @@ private: DirsInChroot dirsInChroot; typedef map Environment; Environment env; + typedef string SandboxProfile; + SandboxProfile additionalSandboxProfile; /* Hash rewriting. */ HashRewrites rewritesToTmp, rewritesFromTmp; @@ -1919,30 +1921,34 @@ void DerivationGoal::startBuilder() for (auto & i : closure) dirsInChroot[i] = i; - string allowed = settings.get("allowed-impure-host-deps", string(DEFAULT_ALLOWED_IMPURE_PREFIXES)); - PathSet allowedPaths = tokenizeString(allowed); + if(!SANDBOX_ENABLED) { + string allowed = settings.get("allowed-impure-host-deps", string(DEFAULT_ALLOWED_IMPURE_PREFIXES)); + PathSet allowedPaths = tokenizeString(allowed); - /* This works like the above, except on a per-derivation level */ - Strings impurePaths = tokenizeString(get(drv->env, "__impureHostDeps")); + /* This works like the above, except on a per-derivation level */ + Strings impurePaths = tokenizeString(get(drv->env, "__impureHostDeps")); - for (auto & i : impurePaths) { - bool found = false; - /* Note: we're not resolving symlinks here to prevent - giving a non-root user info about inaccessible - files. */ - Path canonI = canonPath(i); - /* If only we had a trie to do this more efficiently :) luckily, these are generally going to be pretty small */ - for (auto & a : allowedPaths) { - Path canonA = canonPath(a); - if (canonI == canonA || isInDir(canonI, canonA)) { - found = true; - break; - } - } - if (!found) - throw Error(format("derivation '%1%' requested impure path ‘%2%’, but it was not in allowed-impure-host-deps (‘%3%’)") % drvPath % i % allowed); + for (auto & i : impurePaths) { + bool found = false; + /* Note: we're not resolving symlinks here to prevent + giving a non-root user info about inaccessible + files. */ + Path canonI = canonPath(i); + /* If only we had a trie to do this more efficiently :) luckily, these are generally going to be pretty small */ + for (auto & a : allowedPaths) { + Path canonA = canonPath(a); + if (canonI == canonA || isInDir(canonI, canonA)) { + found = true; + break; + } + } + if (!found) + throw Error(format("derivation '%1%' requested impure path ‘%2%’, but it was not in allowed-impure-host-deps (‘%3%’)") % drvPath % i % allowed); - dirsInChroot[i] = i; + dirsInChroot[i] = i; + } + } else { + additionalSandboxProfile = get(drv->env, "__sandboxProfile"); } #if CHROOT_ENABLED @@ -2471,8 +2477,6 @@ void DerivationGoal::runChild() /* This has to appear before import statements */ sandboxProfile += "(version 1)\n"; - sandboxProfile += (format("(import \"%1%/nix/sandbox-defaults.sb\")\n") % settings.nixDataDir).str(); - /* Violations will go to the syslog if you set this. Unfortunately the destination does not appear to be configurable */ if (settings.get("darwin-log-sandbox-violations", false)) { sandboxProfile += "(deny default)\n"; @@ -2528,13 +2532,20 @@ void DerivationGoal::runChild() } sandboxProfile += ")\n"; + sandboxProfile += additionalSandboxProfile; + debug("Generated sandbox profile:"); debug(sandboxProfile); + char *tmpProfile = strdup((format("%1%/nix-sandboxXXXXXX.sb") % globalTmpDir).str().c_str()); + int profileFd = mkstemps(tmpProfile, 3); + closeOnExec(profileFd); + writeFull(profileFd, sandboxProfile); + builder = "/usr/bin/sandbox-exec"; args.push_back("sandbox-exec"); - args.push_back("-p"); - args.push_back(sandboxProfile); + args.push_back("-f"); + args.push_back(tmpProfile); args.push_back("-D"); args.push_back((format("_GLOBAL_TMP_DIR=%1%") % globalTmpDir).str()); args.push_back(drv->builder); From 22dfd023fafc5951619072d3031e3198f9538e45 Mon Sep 17 00:00:00 2001 From: Jude Taylor Date: Thu, 12 Nov 2015 22:51:52 -0800 Subject: [PATCH 2/9] update sandbox profiles within nix --- corepkgs/buildenv.nix | 18 ++++++++++++++---- release.nix | 5 +++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/corepkgs/buildenv.nix b/corepkgs/buildenv.nix index b4946457f..ab1ce13f2 100644 --- a/corepkgs/buildenv.nix +++ b/corepkgs/buildenv.nix @@ -23,10 +23,20 @@ derivation { # network traffic, so don't do that. preferLocalBuild = true; - __impureHostDeps = if builtins.currentSystem == "x86_64-darwin" then [ - "/usr/lib/libSystem.dylib" - "/usr/lib/system" - ] else null; + __sandboxProfile = '' + (allow sysctl-read) + (allow file-read* + (literal "/usr/lib/libSystem.dylib") + (literal "/usr/lib/libSystem.B.dylib") + (literal "/usr/lib/libobjc.A.dylib") + (literal "/usr/lib/libobjc.dylib") + (literal "/usr/lib/libauto.dylib") + (literal "/usr/lib/libc++abi.dylib") + (literal "/usr/lib/libc++.1.dylib") + (literal "/usr/lib/libDiagnosticMessagesClient.dylib") + (subpath "/usr/lib/system") + (subpath "/dev")) + ''; inherit chrootDeps; } diff --git a/release.nix b/release.nix index 4269a3f76..cb391d0ff 100644 --- a/release.nix +++ b/release.nix @@ -97,6 +97,11 @@ let enableParallelBuilding = true; + __sandboxProfile = lib.sandbox.allowNetwork + + lib.sandbox.allowFileRead { + literal = [ "/etc" "/etc/nix/nix.conf" "/private/etc/nix/nix.conf" ]; + }; + makeFlags = "profiledir=$(out)/etc/profile.d"; preBuild = "unset NIX_INDENT_MAKE"; From d760c2638c9e1f4b8cd9b4ec90d68bf0c76a800b Mon Sep 17 00:00:00 2001 From: Jude Taylor Date: Thu, 12 Nov 2015 22:54:46 -0800 Subject: [PATCH 3/9] remove sandbox-defaults.sb --- src/libstore/local.mk | 1 - src/libstore/sandbox-defaults.sb.in | 63 ----------------------------- 2 files changed, 64 deletions(-) delete mode 100644 src/libstore/sandbox-defaults.sb.in diff --git a/src/libstore/local.mk b/src/libstore/local.mk index f10981ad4..bf5c256c9 100644 --- a/src/libstore/local.mk +++ b/src/libstore/local.mk @@ -33,4 +33,3 @@ $(d)/local-store.cc: $(d)/schema.sql.hh clean-files += $(d)/schema.sql.hh $(eval $(call install-file-in, $(d)/nix-store.pc, $(prefix)/lib/pkgconfig, 0644)) -$(eval $(call install-file-in, $(d)/sandbox-defaults.sb, $(datadir)/nix, 0644)) diff --git a/src/libstore/sandbox-defaults.sb.in b/src/libstore/sandbox-defaults.sb.in deleted file mode 100644 index b5e80085f..000000000 --- a/src/libstore/sandbox-defaults.sb.in +++ /dev/null @@ -1,63 +0,0 @@ -(allow file-read* file-write-data (literal "/dev/null")) -(allow ipc-posix*) -(allow mach-lookup (global-name "com.apple.SecurityServer")) - -(allow file-read* - (literal "/dev/dtracehelper") - (literal "/dev/tty") - (literal "/dev/autofs_nowait") - (literal "/System/Library/CoreServices/SystemVersion.plist") - (literal "/private/var/run/systemkeychaincheck.done") - (literal "/private/etc/protocols") - (literal "/private/var/tmp") - (literal "/private/var/db") - (subpath "/private/var/db/mds")) - -(allow file-read* - (subpath "/usr/share/icu") - (subpath "/usr/share/locale") - (subpath "/usr/share/zoneinfo")) - -(allow file-write* - (literal "/dev/tty") - (literal "/dev/dtracehelper") - (literal "/mds")) - -(allow file-ioctl (literal "/dev/dtracehelper")) - -(allow file-read-metadata - (literal "/var") - (literal "/tmp") - ; symlinks - (literal "@sysconfdir@") - (literal "@sysconfdir@/nix") - (literal "@sysconfdir@/nix/nix.conf") - (literal "/etc/resolv.conf") - (literal "/private/etc/resolv.conf")) - -(allow file-read* - (literal "/private@sysconfdir@/nix/nix.conf") - (literal "/private/var/run/resolv.conf")) - -; some builders use filehandles other than stdin/stdout -(allow file* - (subpath "/dev/fd") - (literal "/dev/ptmx") - (regex #"^/dev/[pt]ty.*$")) - -; allow everything inside TMP -(allow file* process-exec - (subpath (param "_GLOBAL_TMP_DIR")) - (subpath "/private/tmp")) - -(allow process-fork) -(allow sysctl-read) -(allow signal (target same-sandbox)) - -; allow getpwuid (for git and other packages) -(allow mach-lookup - (global-name "com.apple.system.notification_center") - (global-name "com.apple.system.opendirectoryd.libinfo")) - -; allow local networking -(allow network* (local ip) (remote unix-socket)) From 4876bb012e78e6e397f34fd7fb91f67520cbd744 Mon Sep 17 00:00:00 2001 From: Jude Taylor Date: Sat, 14 Nov 2015 13:52:33 -0800 Subject: [PATCH 4/9] simplify build permissions --- release.nix | 7 +++---- src/libstore/build.cc | 16 +++------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/release.nix b/release.nix index cb391d0ff..8935cfa19 100644 --- a/release.nix +++ b/release.nix @@ -97,10 +97,9 @@ let enableParallelBuilding = true; - __sandboxProfile = lib.sandbox.allowNetwork - + lib.sandbox.allowFileRead { - literal = [ "/etc" "/etc/nix/nix.conf" "/private/etc/nix/nix.conf" ]; - }; + __sandboxProfile = lib.sandbox.allowFileRead [ + "/etc" "/etc/nix/nix.conf" "/private/etc/nix/nix.conf" + ]; makeFlags = "profiledir=$(out)/etc/profile.d"; diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 5cd695b1c..32325a6b1 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2498,13 +2498,8 @@ void DerivationGoal::runChild() } sandboxProfile += ")\n"; - /* Our inputs (transitive dependencies and any impurities computed above) - Note that the sandbox profile allows file-write* even though it isn't seemingly necessary. First of all, nix's standard user permissioning - mechanism still prevents builders from writing to input directories, so no security/purity is lost. The reason we allow file-write* is that - denying it means the `access` syscall will return EPERM instead of EACCESS, which confuses a few programs that assume (understandably, since - it appears to be a violation of the POSIX spec) that `access` won't do that, and don't deal with it nicely if it does. The most notable of - these is the entire GHC Haskell ecosystem. */ - sandboxProfile += "(allow file-read* file-write* process-exec mach-priv-task-port\n"; + /* Our inputs (transitive dependencies and any impurities computed above) */ + sandboxProfile += "(allow file-read* process-exec\n"; for (auto & i : dirsInChroot) { if (i.first != i.second) throw SysError(format("can't map '%1%' to '%2%': mismatched impure paths not supported on darwin")); @@ -2520,12 +2515,7 @@ void DerivationGoal::runChild() } sandboxProfile += ")\n"; - /* Our ancestry. N.B: this uses literal on folders, instead of subpath. Without that, - you open up the entire filesystem because you end up with (subpath "/") - Note: file-read-metadata* is not sufficiently permissive for GHC. file-read* is but may - be a security hazard. - TODO: figure out a more appropriate directive. - */ + /* Allow file-read* on full directory hierarchy to self. Allows realpath() */ sandboxProfile += "(allow file-read*\n"; for (auto & i : ancestry) { sandboxProfile += (format("\t(literal \"%1%\")\n") % i.c_str()).str(); From bd09a4c96799275d105b5ffe9a6fcb60200deb5f Mon Sep 17 00:00:00 2001 From: Jude Taylor Date: Sat, 14 Nov 2015 14:09:52 -0800 Subject: [PATCH 5/9] simplify build.cc using modern C++ features --- src/libstore/build.cc | 58 +++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 32325a6b1..1dee1ca2c 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1921,35 +1921,35 @@ void DerivationGoal::startBuilder() for (auto & i : closure) dirsInChroot[i] = i; - if(!SANDBOX_ENABLED) { - string allowed = settings.get("allowed-impure-host-deps", string(DEFAULT_ALLOWED_IMPURE_PREFIXES)); - PathSet allowedPaths = tokenizeString(allowed); +#if SANDBOX_ENABLED + additionalSandboxProfile = get(drv->env, "__sandboxProfile"); +#else + string allowed = settings.get("allowed-impure-host-deps", string(DEFAULT_ALLOWED_IMPURE_PREFIXES)); + PathSet allowedPaths = tokenizeString(allowed); - /* This works like the above, except on a per-derivation level */ - Strings impurePaths = tokenizeString(get(drv->env, "__impureHostDeps")); + /* This works like the above, except on a per-derivation level */ + Strings impurePaths = tokenizeString(get(drv->env, "__impureHostDeps")); - for (auto & i : impurePaths) { - bool found = false; - /* Note: we're not resolving symlinks here to prevent - giving a non-root user info about inaccessible - files. */ - Path canonI = canonPath(i); - /* If only we had a trie to do this more efficiently :) luckily, these are generally going to be pretty small */ - for (auto & a : allowedPaths) { - Path canonA = canonPath(a); - if (canonI == canonA || isInDir(canonI, canonA)) { - found = true; - break; - } - } - if (!found) - throw Error(format("derivation '%1%' requested impure path ‘%2%’, but it was not in allowed-impure-host-deps (‘%3%’)") % drvPath % i % allowed); + for (auto & i : impurePaths) { + bool found = false; + /* Note: we're not resolving symlinks here to prevent + giving a non-root user info about inaccessible + files. */ + Path canonI = canonPath(i); + /* If only we had a trie to do this more efficiently :) luckily, these are generally going to be pretty small */ + for (auto & a : allowedPaths) { + Path canonA = canonPath(a); + if (canonI == canonA || isInDir(canonI, canonA)) { + found = true; + break; + } + } + if (!found) + throw Error(format("derivation '%1%' requested impure path ‘%2%’, but it was not in allowed-impure-host-deps (‘%3%’)") % drvPath % i % allowed); - dirsInChroot[i] = i; - } - } else { - additionalSandboxProfile = get(drv->env, "__sandboxProfile"); + dirsInChroot[i] = i; } +#endif #if CHROOT_ENABLED /* Create a temporary directory in which we set up the chroot @@ -2527,17 +2527,15 @@ void DerivationGoal::runChild() debug("Generated sandbox profile:"); debug(sandboxProfile); - char *tmpProfile = strdup((format("%1%/nix-sandboxXXXXXX.sb") % globalTmpDir).str().c_str()); - int profileFd = mkstemps(tmpProfile, 3); - closeOnExec(profileFd); - writeFull(profileFd, sandboxProfile); + Path tmpProfile = createTempDir() + "/profile.sb"; + writeFile(tmpProfile, sandboxProfile); builder = "/usr/bin/sandbox-exec"; args.push_back("sandbox-exec"); args.push_back("-f"); args.push_back(tmpProfile); args.push_back("-D"); - args.push_back((format("_GLOBAL_TMP_DIR=%1%") % globalTmpDir).str()); + args.push_back("_GLOBAL_TMP_DIR=" + globalTmpDir); args.push_back(drv->builder); } else { builder = drv->builder.c_str(); From 4390142315a0d6ed0f67712061498c68389ea3b7 Mon Sep 17 00:00:00 2001 From: Shea Levy Date: Sun, 15 Nov 2015 06:08:50 -0500 Subject: [PATCH 6/9] Use AutoDelete for sandbox profile file --- src/libstore/build.cc | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 1dee1ca2c..6f662f81d 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -778,9 +778,13 @@ private: DirsInChroot dirsInChroot; typedef map Environment; Environment env; +#if SANDBOX_ENABLED typedef string SandboxProfile; SandboxProfile additionalSandboxProfile; + AutoDelete autoDelSandbox; +#endif + /* Hash rewriting. */ HashRewrites rewritesToTmp, rewritesFromTmp; typedef map RedirectedOutputs; @@ -2445,9 +2449,10 @@ void DerivationGoal::runChild() const char *builder = "invalid"; string sandboxProfile; - if (isBuiltin(*drv)) + if (isBuiltin(*drv)) { ; - else if (useChroot && SANDBOX_ENABLED) { +#if SANDBOX_ENABLED + } else if (useChroot) { /* Lots and lots and lots of file functions freak out if they can't stat their full ancestry */ PathSet ancestry; @@ -2527,16 +2532,20 @@ void DerivationGoal::runChild() debug("Generated sandbox profile:"); debug(sandboxProfile); - Path tmpProfile = createTempDir() + "/profile.sb"; - writeFile(tmpProfile, sandboxProfile); + Path sandboxFile = drvPath + ".sb"; + if (pathExists(sandboxFile)) deletePath(sandboxFile); + autoDelSandbox = AutoDelete(sandboxFile); + + writeFile(sandboxFile, sandboxProfile); builder = "/usr/bin/sandbox-exec"; args.push_back("sandbox-exec"); args.push_back("-f"); - args.push_back(tmpProfile); + args.push_back(sandboxFile); args.push_back("-D"); args.push_back("_GLOBAL_TMP_DIR=" + globalTmpDir); args.push_back(drv->builder); +#endif } else { builder = drv->builder.c_str(); string builderBasename = baseNameOf(drv->builder); From 58d2fac91d0da7312e3ef147b6b290ea16031da8 Mon Sep 17 00:00:00 2001 From: Shea Levy Date: Mon, 16 Nov 2015 05:53:10 -0500 Subject: [PATCH 7/9] AutoDelete: Add default constructor with deletion disabled --- src/libstore/build.cc | 2 +- src/libutil/util.cc | 8 ++++++++ src/libutil/util.hh | 2 ++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 6f662f81d..6112d528c 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -2534,7 +2534,7 @@ void DerivationGoal::runChild() Path sandboxFile = drvPath + ".sb"; if (pathExists(sandboxFile)) deletePath(sandboxFile); - autoDelSandbox = AutoDelete(sandboxFile); + autoDelSandbox.reset(sandboxFile, false); writeFile(sandboxFile, sandboxProfile); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 27116fd18..84f578eec 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -599,6 +599,8 @@ string drainFD(int fd) ////////////////////////////////////////////////////////////////////// +AutoDelete::AutoDelete() : del{false} {} + AutoDelete::AutoDelete(const string & p, bool recursive) : path(p) { del = true; @@ -626,6 +628,12 @@ void AutoDelete::cancel() del = false; } +void AutoDelete::reset(const Path & p, bool recursive = true) { + this-> p = p; + this->recursive = recursive; + del = true; +} + ////////////////////////////////////////////////////////////////////// diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 23d01e9a6..f4026a0a8 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -199,9 +199,11 @@ class AutoDelete bool del; bool recursive; public: + AutoDelete(); AutoDelete(const Path & p, bool recursive = true); ~AutoDelete(); void cancel(); + void reset(const Path & p, bool recursive = true); operator Path() const { return path; } }; From 9b4cd20752886d2e5447297d5fd00dd83b1ce547 Mon Sep 17 00:00:00 2001 From: Shea Levy Date: Mon, 16 Nov 2015 05:54:34 -0500 Subject: [PATCH 8/9] Fix copy-paste error --- src/libutil/util.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 84f578eec..0a19e79bc 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -629,7 +629,7 @@ void AutoDelete::cancel() } void AutoDelete::reset(const Path & p, bool recursive = true) { - this-> p = p; + path = p; this->recursive = recursive; del = true; } From 1d3529e93a449622987f259e6449a63fff62a1b2 Mon Sep 17 00:00:00 2001 From: Shea Levy Date: Mon, 16 Nov 2015 05:55:55 -0500 Subject: [PATCH 9/9] Default arguments belong at declaration, not definition --- src/libutil/util.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 0a19e79bc..75032bf90 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -628,7 +628,7 @@ void AutoDelete::cancel() del = false; } -void AutoDelete::reset(const Path & p, bool recursive = true) { +void AutoDelete::reset(const Path & p, bool recursive) { path = p; this->recursive = recursive; del = true;