Make sure that NSTemporaryDirectory() is included in the sandbox when writableTemporaryDirectory is set
#3918
Conversation
…raryDirectory` is set. It seems that `TSCBasic.determineTempDirectory()` lets environment variables override the temporary directory, which might be alright for the process itself but not necessarily for spawned subprocesses. If the subprocess uses NSTemporaryDirectory(), then that still won't be covered by the sandbox. This change makes sure that it is covered by the sandbox, and uses a set to avoid duplicates (which wouldn't actually hurt but is inelegant). rdar://86150592
abertelrud
added
the
ready
|
@swift-ci please smoke test |
| writableDirectoriesExpression.append("(subpath \"/private/tmp\")") | ||
| if let tmpDir = try? TSCBasic.determineTempDirectory() { | ||
| // Add the standard and Foundation temporary directories, and the one determined by TSC (which also taked into account environment variables). | ||
| var temporaryDirectories = Set([AbsolutePath("/tmp"), AbsolutePath(NSTemporaryDirectory())]) |
There was a problem hiding this comment.
Note that this will get resolved to /private/tmp since all the paths now go through the resolveSymlinks below. So this is not a material change from before. But it is semantically more correct, since the fact that /tmp is a symlink to /private/tmp is an implementation detail.
| if let tmpDir = try? TSCBasic.determineTempDirectory() { | ||
| // Add the standard and Foundation temporary directories, and the one determined by TSC (which also taked into account environment variables). | ||
| var temporaryDirectories = Set([AbsolutePath("/tmp"), AbsolutePath(NSTemporaryDirectory())]) | ||
| if let tscTmpDir = try? TSCBasic.determineTempDirectory() { |
There was a problem hiding this comment.
It's not clear that we should still be doing this instead of just the standard paths. But it seems better to start more lenient and then tighten it over time.
There was a problem hiding this comment.
Since the sandbox always applies to a subprocess, the most correct thing to do here is probably for the code in Process that applies the sandbox to look in the environment of the process it is about to launch (however it gets that environment, whether by inheritance or direct assignment, and if TMPDIR or any of the other usual suspects is set, then it should add that as a writable directory when it applies the sandbox. The environment of the parent doesn't necessarily get inherited and so should ideally not affect the sandbox.
There was a problem hiding this comment.
Can the sandbox of a subprocess be more lenient than the one of the parent?
There was a problem hiding this comment.
Nested sandboxing is actually completely unsupported right now (on Darwin). What I meant by lenient is to allow both the NSTemporaryDirectory and also any path provided by TMPDIR. But I think we should probably remove the TMPDIR part later and just allow /tmp and NSTemporaryDirectory. I'll do that in separate PR but want to unblock CI.
There was a problem hiding this comment.
Adjusted it here #3920
…eTemporaryDirectory` is set. (swiftlang#3918) It seems that `TSCBasic.determineTempDirectory()` lets environment variables override the temporary directory, which might be alright for the process itself but not necessarily for spawned subprocesses. If the subprocess uses NSTemporaryDirectory(), then that still won't be covered by the sandbox. This change makes the sandbox cover the regular "/tmp" and NSTemporaryDirectory() paths. If environment entries are used to set other temporary directories in the subprocess, then the calling code should pass those to the `writableDirectories` parameter. rdar://86150592 (cherry picked from commit 5546a97 and 3ba3305)
abertelrud
deleted the
eng/make-sure-sandbox-profile-includes-nstemporarydirectory
branch
It seems that
TSCBasic.determineTempDirectory()lets environment variables override the temporary directory, which might be alright for the process itself but not necessarily for spawned subprocesses. If the subprocess uses NSTemporaryDirectory(), then that still won't be covered by the sandbox.This change makes sure that it is covered by the sandbox, and uses a set to avoid duplicates (which wouldn't actually hurt but is inelegant).
This fixes a CI problem such as seen in https://ci.swift.org/job/oss-swift-package-macos/6399.
Modifications:
It's interesting to note that the PR build passed. Perhaps the set of environment overrides is different for PR builds.
Since 5.6 was branched today this will also need to be merged there.
rdar://86150592