Skip to content

Commit

Permalink
Functionality for pseudoterminals in linux sandbox
Browse files Browse the repository at this point in the history
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
  • Loading branch information
crydell-ericsson authored and copybara-github committed Oct 18, 2022
1 parent 3e83fbe commit 9a13051
Show file tree
Hide file tree
Showing 7 changed files with 73 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -262,6 +273,9 @@ public ImmutableList<String> build() {
if (useFakeUsername) {
commandLineBuilder.add("-U");
}
if (enablePseudoterminal) {
commandLineBuilder.add("-P");
}
if (useDebugMode) {
commandLineBuilder.add("-D");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 5 additions & 1 deletion src/main/tools/linux-sandbox-options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 <sandbox-dir> if set, chroot to sandbox-dir and only "
" mount whats been specified with -M/-m for improved hermeticity. "
Expand All @@ -97,7 +98,7 @@ static void ParseCommandLine(unique_ptr<vector<char *>> 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':
Expand Down Expand Up @@ -215,6 +216,9 @@ static void ParseCommandLine(unique_ptr<vector<char *>> args) {
}
opt.fake_username = true;
break;
case 'P':
opt.enable_pty = true;
break;
case 'D':
opt.debug = true;
break;
Expand Down
3 changes: 3 additions & 0 deletions src/main/tools/linux-sandbox-options.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 18 additions & 0 deletions src/main/tools/linux-sandbox-pid1.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#include <errno.h>
#include <fcntl.h>
#include <grp.h>
#include <libgen.h>
#include <math.h>
#include <mntent.h>
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 20 additions & 0 deletions src/test/shell/bazel/bazel_sandboxing_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 9a13051

Please sign in to comment.