From 9a13051fb73d68221dfd9849b52e0f3cd046c524 Mon Sep 17 00:00:00 2001 From: Christopher Rydell Date: Tue, 18 Oct 2022 05:00:32 -0700 Subject: [PATCH] Functionality for pseudoterminals in linux sandbox As brought up in issue #5373 , the Linux sandbox does not allow processes that run inside it to open pseudoterminals. These changes enable this by addressing the two main underlying issues: - `/dev/pts` can not be read-only if a new pseudoterminal is to be created. These changes make `dev/pts` writable when remounting file systems during sandbox initialization. - The group associated with pseudoterminals is "tty". After creating a new pseudoterminal, its gid has to be changed. If there is no gid mapping in the user namespace that corresponds to "tty", this group will not be known inside the sandbox. This causes issues in some Linux distributions, since they do not allow changing the group of a file to one that is not known inside the current user namespace. These changes map the gid of the user to the one corresponding to "tty" inside the sandbox in order to avoid this issue. These changes introduce the `-P` flag to `linux-sandbox` in order to control whether or not the changes are applied, and the `--sandbox-explicit-pseudoterminal` to `bazel` in order to set this when calling bazel. Closes #14072. PiperOrigin-RevId: 481889631 Change-Id: I5d686769096003a80d4ceffe0ccfcd19c6a7d174 --- .../build/lib/sandbox/LinuxSandboxUtil.java | 14 +++++++++++++ .../sandbox/LinuxSandboxedSpawnRunner.java | 1 + .../build/lib/sandbox/SandboxOptions.java | 12 +++++++++++ src/main/tools/linux-sandbox-options.cc | 6 +++++- src/main/tools/linux-sandbox-options.h | 3 +++ src/main/tools/linux-sandbox-pid1.cc | 18 +++++++++++++++++ src/test/shell/bazel/bazel_sandboxing_test.sh | 20 +++++++++++++++++++ 7 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtil.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtil.java index 659057424996d8..d10ab53b4640a4 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtil.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxUtil.java @@ -72,6 +72,7 @@ public static class CommandLineBuilder { private boolean createNetworkNamespace = false; private boolean useFakeRoot = false; private boolean useFakeUsername = false; + private boolean enablePseudoterminal = false; private boolean useDebugMode = false; private boolean sigintSendsSigterm = false; @@ -188,6 +189,16 @@ public CommandLineBuilder setUseFakeUsername(boolean useFakeUsername) { return this; } + /** + * Sets whether to set group to 'tty' and make /dev/pts writable inside the sandbox in order to + * enable the use of pseudoterminals. + */ + @CanIgnoreReturnValue + public CommandLineBuilder setEnablePseudoterminal(boolean enablePseudoterminal) { + this.enablePseudoterminal = enablePseudoterminal; + return this; + } + /** Sets whether to enable debug mode (e.g. to print debugging messages). */ @CanIgnoreReturnValue public CommandLineBuilder setUseDebugMode(boolean useDebugMode) { @@ -262,6 +273,9 @@ public ImmutableList build() { if (useFakeUsername) { commandLineBuilder.add("-U"); } + if (enablePseudoterminal) { + commandLineBuilder.add("-P"); + } if (useDebugMode) { commandLineBuilder.add("-D"); } diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java index bf7c097036a3e7..1fe431d17a6685 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java @@ -229,6 +229,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context .setTmpfsDirectories(ImmutableSet.copyOf(getSandboxOptions().sandboxTmpfsPath)) .setBindMounts(getBindMounts(blazeDirs, sandboxExecRoot, sandboxTmp)) .setUseFakeHostname(getSandboxOptions().sandboxFakeHostname) + .setEnablePseudoterminal(getSandboxOptions().sandboxExplicitPseudoterminal) .setCreateNetworkNamespace( !(allowNetwork || Spawns.requiresNetwork( diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java index 2d3d05529df175..0a09fa75f77ba4 100644 --- a/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java +++ b/src/main/java/com/google/devtools/build/lib/sandbox/SandboxOptions.java @@ -131,6 +131,18 @@ public String getTypeDescription() { help = "Change the current username to 'nobody' for sandboxed actions.") public boolean sandboxFakeUsername; + @Option( + name = "sandbox_explicit_pseudoterminal", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, + effectTags = {OptionEffectTag.EXECUTION}, + help = + "Explicitly enable the creation of pseudoterminals for sandboxed actions." + + " Some linux distributions require setting the group id of the process to 'tty'" + + " inside the sandbox in order for pseudoterminals to function. If this is" + + " causing issues, this flag can be disabled to enable other groups to be used.") + public boolean sandboxExplicitPseudoterminal; + @Option( name = "sandbox_block_path", allowMultiple = true, diff --git a/src/main/tools/linux-sandbox-options.cc b/src/main/tools/linux-sandbox-options.cc index a9a7ac552dc8b1..50ca4f79d7da45 100644 --- a/src/main/tools/linux-sandbox-options.cc +++ b/src/main/tools/linux-sandbox-options.cc @@ -72,6 +72,7 @@ static void Usage(char *program_name, const char *fmt, ...) { " -N if set, a new network namespace will be created\n" " -R if set, make the uid/gid be root\n" " -U if set, make the uid/gid be nobody\n" + " -P if set, make the gid be tty and make /dev/pts writable\n" " -D if set, debug info will be printed\n" " -h if set, chroot to sandbox-dir and only " " mount whats been specified with -M/-m for improved hermeticity. " @@ -97,7 +98,7 @@ static void ParseCommandLine(unique_ptr> args) { bool source_specified = false; while ((c = getopt(args->size(), args->data(), - ":W:T:t:il:L:w:e:M:m:S:h:HNRUD")) != -1) { + ":W:T:t:il:L:w:e:M:m:S:h:HNRUPD")) != -1) { if (c != 'M' && c != 'm') source_specified = false; switch (c) { case 'W': @@ -215,6 +216,9 @@ static void ParseCommandLine(unique_ptr> args) { } opt.fake_username = true; break; + case 'P': + opt.enable_pty = true; + break; case 'D': opt.debug = true; break; diff --git a/src/main/tools/linux-sandbox-options.h b/src/main/tools/linux-sandbox-options.h index 2843e9f8e3aa90..1766f463f5ba3f 100644 --- a/src/main/tools/linux-sandbox-options.h +++ b/src/main/tools/linux-sandbox-options.h @@ -52,6 +52,9 @@ struct Options { bool fake_root; // Set the username inside the sandbox to 'nobody' (-U) bool fake_username; + // Enable writing to /dev/pts and map the user's gid to tty to enable + // pseudoterminals (-P) + bool enable_pty; // Print debugging messages (-D) bool debug; // Improved hermetic build using whitelisting strategy (-h) diff --git a/src/main/tools/linux-sandbox-pid1.cc b/src/main/tools/linux-sandbox-pid1.cc index 703b8c32a53f64..cc7e465863c3f9 100644 --- a/src/main/tools/linux-sandbox-pid1.cc +++ b/src/main/tools/linux-sandbox-pid1.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -242,6 +243,19 @@ static void SetupUserNamespace() { inner_uid = global_outer_uid; inner_gid = global_outer_gid; } + if (opt.enable_pty) { + // Change the group to "tty" regardless of what was previously set + struct group grp; + char buf[256]; + size_t buflen = sizeof(buf); + struct group *result; + getgrnam_r("tty", &grp, buf, buflen, &result); + if (result == nullptr) { + DIE("getgrnam_r"); + } + inner_gid = grp.gr_gid; + } + WriteFile("/proc/self/uid_map", "%u %u 1\n", inner_uid, global_outer_uid); WriteFile("/proc/self/gid_map", "%u %u 1\n", inner_gid, global_outer_gid); } @@ -325,6 +339,10 @@ static bool ShouldBeWritable(const std::string &mnt_dir) { return true; } + if (opt.enable_pty && mnt_dir == "/dev/pts") { + return true; + } + for (const std::string &writable_file : opt.writable_files) { if (mnt_dir == writable_file) { return true; diff --git a/src/test/shell/bazel/bazel_sandboxing_test.sh b/src/test/shell/bazel/bazel_sandboxing_test.sh index 44dc2a6cab0c54..d64f029bb11da8 100755 --- a/src/test/shell/bazel/bazel_sandboxing_test.sh +++ b/src/test/shell/bazel/bazel_sandboxing_test.sh @@ -645,6 +645,26 @@ EOF bazel test --nocache_test_results --sandbox_fake_username --test_output=errors :test || fail "test did not pass" } +# Tests that a pseudoterminal can be opened in linux when --sandbox_explicit_pseudoterminal is active +function test_can_enable_pseudoterminals() { + if [[ "$(uname -s)" != Linux ]]; then + echo "Skipping test: flag intended for linux systems" + return 0 + fi + + cat > test.py <<'EOF' +import pty +pty.openpty() +EOF + cat > BUILD <<'EOF' +py_test( + name = "test", + srcs = ["test.py"], +) +EOF + bazel test --sandbox_explicit_pseudoterminal :test || fail "test did not pass" +} + # Tests that /proc/self == /proc/$$. This should always be true unless the PID namespace is active without /proc being remounted correctly. function test_sandbox_proc_self() { if [[ ! -d /proc/self ]]; then