Skip to content

Make sure that NSTemporaryDirectory() is included in the sandbox when writableTemporaryDirectory is set #3918

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 8, 2021
Merged

Make sure that NSTemporaryDirectory() is included in the sandbox when writableTemporaryDirectory is set #3918

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Sources/Basics/Sandbox.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import Foundation
import TSCBasic
import TSCUtility

Expand Down Expand Up @@ -73,8 +74,13 @@ fileprivate func macOSSandboxProfile(
}
// Optionally allow writing to temporary directories (a lot of use of Foundation requires this).
else if strictness == .writableTemporaryDirectory {
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())])
Copy link
Contributor Author

@abertelrud abertelrud Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 tscTmpDir = try? TSCBasic.determineTempDirectory() {
Copy link
Contributor Author

@abertelrud abertelrud Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the sandbox of a subprocess be more lenient than the one of the parent?

Copy link
Contributor Author

@abertelrud abertelrud Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted it here #3920

temporaryDirectories.insert(tscTmpDir)
}
// Add `subpath` expressions for all of them.
for tmpDir in temporaryDirectories.sorted() {
writableDirectoriesExpression += ["(subpath \(resolveSymlinks(tmpDir).quotedAsSubpathForSandboxProfile))"]
}
}
Expand Down