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

Sandboxes: Make /tmp writable to restore POSIX compliancy #5634

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Aug 15, 2023

Fixes #5462

This PR still fixes the issue described in #4589 while restoring POSIX compliency as described in #5462 by making /tmp writable and propagate its content regardless of the previous value of TMPDIR.

The value of TMPDIR does not change and still provides a cleared, fast and unshared directory on Linux (the macOS sandbox is not expressive enough to be able to do that but that behaviour does not change either)

Side note for macOS. In macOS /tmp is a symlink so we need to make both the symlink and its target directory writable. I simply copied the behaviour from MacPorts that you can see here: https://github.com/macports/macports-base/blob/96d5581e069463c488cc878a5c7c73fc7117b905/src/port1.0/portsandbox.tcl#L92

@avsm
Copy link
Member

avsm commented Aug 15, 2023

Looks right to me, and I agree that making /tmp writeable according to POSIX is the right move for the sandbox.

@kit-ty-kate kit-ty-kate requested a review from AltGr August 29, 2023 16:28
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

In normal use this seems OK ;
the bit I still have uncertainty about is that it seems to me that this can relax the effectfulness of the sandbox in the case where your opamroot is in /tmp

What could happen (please correct if that's not the case!) is that /tmp is rw, /tmp/opamroot is rw, and only /tmp/opamroot/myswitch is rebound ro. As a consequence, the sandboxed process would be able to alter the opam root (outside of the switch).

I am not sure this is a problem for a few reasons:

  • an opamroot in /tmp is not expected for production use anyway I guess
  • the sandbox is disabled anyway in tests ?

still, if one was to use an opamroot in /tmp to check that the package correctly followed the sandboxing rules, and the package build rules would run say opam switch create, the test wouldn't catch it (or so I presume)

I don't know if this is a real problem, but it should at least be exposed and discussed

@kit-ty-kate
Copy link
Member Author

What could happen (please correct if that's not the case!) is that /tmp is rw, /tmp/opamroot is rw, and only /tmp/opamroot/myswitch is rebound ro. As a consequence, the sandboxed process would be able to alter the opam root (outside of the switch).

fair point. f69b92a takes care of this problem. Whether the opam root is defined through OPAMROOT or --root the OPAMROOT variable will always be present when it is in its non-default place.

@rjbou rjbou merged commit ce386b7 into ocaml:master Sep 11, 2023
26 checks passed
@rjbou
Copy link
Collaborator

rjbou commented Sep 11, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ensure /tmp is writeable from the sandbox
4 participants