Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functionality for pseudoterminals in linux sandbox #14072

Conversation

crydell-ericsson
Copy link
Contributor

@crydell-ericsson crydell-ericsson commented Oct 4, 2021

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.

@google-cla google-cla bot added the cla: yes label Oct 4, 2021
@aiuto aiuto requested a review from philwo October 7, 2021 20:42
@aiuto aiuto added the team-Local-Exec Issues and PRs for the Execution (Local) team label Oct 7, 2021
Copy link
Contributor

@aiuto aiuto left a comment

Choose a reason for hiding this comment

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

This review is just general comments. I don't know enough about the sandbox to determine if this is a general purpose patch, or if we'll be back for a new design the second someone wants it for BSD or NIX. Also, it is not clear to me from the comments in #5373 that we should even do this.

"Explicitly enable the creation of pseudoterminals for sandboxed actions."
+ " Some linux distributions require setting the group of the user 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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not look like there is any option to use other gid values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intended meaning is that setting the flag would override anything that would otherwise alter the gid within the user namespace, e.g. if --sandbox_fake_username is set or if REQUIRES_FAKEROOT evaluates to true. I could rephrase this if it's unclear.

@aiuto
Copy link
Contributor

aiuto commented Oct 7, 2021

We'll need to have someone who owns sandboxing review this. I added @philwo because of comments in the original bug.

@philwo
Copy link
Member

philwo commented Oct 19, 2021

Thank you @crydell-ericsson! Ideally I'd like to avoid adding yet another flag... is there a way to automatically detect what the right thing to do is?

Could we just always mount /dev/pts and somehow map or make the tty group usable inside the sandbox without explicitly changing to it?

@crydell-ericsson
Copy link
Contributor Author

Could we just always mount /dev/pts

I think we should be able to always have /dev/pts mounted as writable, yes. I don't know of any situations where this would reasonably cause issues, it was mostly included with the flag as a precaution.

and somehow map or make the tty group usable inside the sandbox without explicitly changing to it?

I'm not sure how this could be done in a way that would still let this functionality be used by an unprivileged user. From the man page of user_namespaces(7):

   In order for a process to write to the /proc/[pid]/uid_map
   (/proc/[pid]/gid_map) file, all of the following permission
   requirements must be met:
   
   [...]

   5. One of the following two cases applies:

      *  Either the writing process has the CAP_SETUID (CAP_SETGID)
         capability in the parent user namespace.

         +  No further restrictions apply: the process can make
            mappings to arbitrary user IDs (group IDs) in the parent
            user namespace.

      *  Or otherwise all of the following restrictions apply:

         +  The data written to uid_map (gid_map) must consist of a
            single line that maps the writing process's effective
            user ID (group ID) in the parent user namespace to a
            user ID (group ID) in the user namespace.

         +  The writing process must have the same effective user ID
            as the process that created the user namespace.

         +  In the case of gid_map, use of the setgroups(2) system
            call must first be denied by writing "deny" to the
            /proc/[pid]/setgroups file (see below) before writing to
            gid_map.

Ideally you would be able to make two gid mappings, one tty->tty mapping and one arbitrary mapping for the gid in the parent namespace. This would not be possible if we want to run bazel as an unprivileged user, since only a single mapping can be done without granting the CAP_SETGID capability to the process in the parent namespace.

@crydell-ericsson
Copy link
Contributor Author

Just thought I'd check in on the progress of this pull request; is there any additional information needed for this?

@philwo
Copy link
Member

philwo commented Nov 24, 2021

Thank you for the information, @crydell-ericsson. It's too bad that the best solution requires CAP_SETGID. 😞 I agree with your thoughts.

@larsrc-google Shall we merge this as is? Looks clean to me and all changed behavior is behind a flag.

@larsrc-google
Copy link
Contributor

@crydell-ericsson This PR is out of date, could you update it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author cla: yes team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants