Sandbox for manifests and plugins should provide a unique path specified by TMPDIR instead of allowing /tmp
#4307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
abertelrud
commented
Apr 19, 2022
abertelrud
commented
Apr 19, 2022
neonichu
reviewed
Apr 19, 2022
neonichu
approved these changes
Apr 19, 2022
abertelrud
force-pushed
the
eng/91575256-sandbox-improvements
branch
2 times, most recently
from
780bbd9 to
0963c8d
Compare
|
@swift-ci please smoke test |
tomerd
reviewed
Apr 19, 2022
tomerd
reviewed
Apr 19, 2022
tomerd
reviewed
Apr 19, 2022
tomerd
reviewed
Apr 19, 2022
tomerd
reviewed
Apr 19, 2022
tomerd
reviewed
Apr 19, 2022
tomerd
reviewed
Apr 19, 2022
abertelrud
added
the
ready
abertelrud
force-pushed
the
eng/91575256-sandbox-improvements
branch
from
0963c8d to
6440584
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
Putting back to Draft — I'm going to fix the |
abertelrud
removed
the
ready
|
Since this PR has a couple of separate changes, I will be pulling them out into separate PRs. |
I let this one sit for a long time, but it seems like high time to at least incorporate the SandboxProfile cleanup that doesn't change the TMPDIR semantics, since the cleanup seemed uncontroversial. |
|
My word, there are a lot of merge conflicts now... :) |
|
I have rebased onto |
abertelrud
force-pushed
the
eng/91575256-sandbox-improvements
branch
from
fa71720 to
c429e2e
Compare
… struct that's easier to manage Turn the stateless `Sandbox` enum into a `SandboxProfile` struct that's a bit more flexible: - there is a list of path rules allowing a mixed order of `writable`, `readonly`, and `noaccess` rules; this addresses an issue where writable directories were always applied after readable directories, preventing arrangements where, for example, a source directory inside the temporary directory was properly made readonly - the defaults are built into the initializer rather than a separate `strictness` parameter — this means that they can be queried once the SandboxProfile has been created and are expressed in terms of the choices available to all sandbox profiles - it's a bit more ergonomic (as a struct, it can be copied, mutated, and carried around in a property before being used), and an optional `SandboxProfile` can be used to indicate both whether or not to apply a profile and if so what to apply. Having the sandbox profile be a struct that generates the platform specifics as needed also could also provide a place with which to associate any cached/compiled representation for profiles that are largely static, for any platform that supports that. As cleanup, this commit also removes the pre-SwiftPM 5.3 specialities which were specific to running the package manifest in an interpreter rather than compiling and executing it. This functionality is no longer relevant since it isn't possible to run any manifest in the interpreter, no matter what tools version the package specifies. In this commit, the call sites have been adjusted so that they use the modified SandboxProfile API, but they still construct the profiles on-the-fly as before. A future change will make them instead be configured at an early point and then applied later when the sandboxed process is actually launched. Note also that the existing call sites still pass `/tmp` because that it was the sandbox has allowed up until now. A separate PR will deal with whether or not allowing writes to `/tmp` is apprporiate. Another future change could add the sandbox profile to `Process` as a property so that there is no API assumption that applying a sandbox necessarily involves just modifying the command line.
abertelrud
force-pushed
the
eng/91575256-sandbox-improvements
branch
from
c429e2e to
0bd9968
Compare
|
@swift-ci please smoke test |
|
Oh right, the sandboxing is only a Darwin thing, so the test fails on other platforms. I'll fix that. |
…vide a unique path specified by `TMPDIR` The `/tmp` temporary directory is particularly unsafe because it is noisy — various processes with various ownerships write and read files there. So it's better to use the per-user directory returned by `NSTemporaryDirectory()`. We can improve several things here by passing through the result of calling `NSTemporaryDirectory()` in the sandbox instead of `/tmp`, and also `TMPDIR` in the environment (regardless of whether or not it was set in the inherited one). This will cause the inferior's use of Foundation to respect this directory (the implementation of `NSTemporaryDirectory()` looks at `TMPDIR` on all platforms), and that should also be true for any command line tools it calls, as long as it passes through the environment. Note: if `TMPDIR` happens to be set in SwiftPM's own environment, it will control SwiftPM's own uses of `NSTemporaryDirectory()`, so the callers `TMPDIR` will be respected all the way through (down to manifests and plugins). rdar://91575256
abertelrud
force-pushed
the
eng/91575256-sandbox-improvements
branch
from
0bd9968 to
273865b
Compare
|
@swift-ci please smoke test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The sandbox used for manifests and plugins currently allows unrestricted access to
/tmpas well as to the directory returned byNSTemporaryDirectory(). It would be safer to just provide the directory returned byNSTemporaryDirectory(). Since and most tools look atTMPDIR, this should not prevent doing anything that could be done before but provides better control of where temporary files go.Motivation
The
/tmptemporary directory is particularly unsafe because it is noisy — various processes with various ownerships write and read files there. So it's better to use the per-user directory returned byNSTemporaryDirectory().Details
We can improve several things here by passing through the result of calling
NSTemporaryDirectory()in the sandbox instead of/tmpand also settingTMPDIRin the environment (regardless of whether or not it was set in the inherited one). This will cause the inferior's use of Foundation to respect this directory (the implementation ofNSTemporaryDirectory()looks atTMPDIRon all platforms), and that should also be true for any command line tools it calls, as long as it passes through the environment.If
TMPDIRis set in SwiftPM's own environment, it will control SwiftPM's ownNSTemporaryDirectory(), and so the specified directory will be respected all the way through.Remarks
This builds on the improvements made in #5857, so it's better to review only the second commit that removes the writability of
/tmp.Note that we don't gate this check on tools version as we usually do. When dealing with tightening a sandbox, it seems pointless to do so based on the tools version, since that allows any package to circumvent the change in perpetuity. This could indeed lead to some manifests or plugins requiring changes.
Also, I'm not delighted to make very similar modifications to four separate call sites. While I don't think special-casing
/tmpshould be part of the sandbox profile, I do think it would make sense to push this functionality down toProcessas an option to setTMPDIRin the environment of the subprocess (as indeed it would also make sense to extendProcessto allow it to take a Sandbox profile in the first place; enabling the option to setTMPDIRin the subprocess could then also add that as a writable directory in the Sandbox profile before applying it, making everything dealing with the temporary directory nicely packaged inside of theProcesslaunch).Changes
/tmpfrom the Sandbox profiles/tmpisn't writablerdar://91575256