Skip to content

Commit f7aa5f8

Browse files
authored
Enforce read-only-ness of package directory even when it's inside writable directories (#3996) (#3999)
* Enforce read-only-ness of package directory even when it's inside /tmp and other writable directories The sandbox supports a list of `writableDirectories`, which are treated as path component prefixes after package resolution. The are applied to the sandbox rules after the default-deny and everything-is-read-only rules, carving out cones of writability in specific areas of the file system. But if the package directory is under one of these directories that is allowed by default, then it is implicitly writable even in the absence `--allow-writing-to-package-directory`. It also makes unit tests awkward, since they usually create temporary test fixtures inside the temporary directory. This adds a set of `readOnlyDirectories` in addition to the writable ones, so that the package directory remains unwritable regardless of where it is located. It also adds some documentation in the Sandbox source file, and it removes a somewhat confusing `--block-writing-to-package-directory` flag that was added mostly for unit tests but that is no longer needed. (cherry picked from commit c117c86)
1 parent 975f06a commit f7aa5f8

File tree

11 files changed

+105
-42
lines changed

11 files changed

+105
-42
lines changed

Sources/Basics/Sandbox.swift

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*
22
This source file is part of the Swift.org open source project
33

4-
Copyright (c) 2021 Apple Inc. and the Swift project authors
4+
Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
55
Licensed under Apache License v2.0 with Runtime Library Exception
66

77
See http://swift.org/LICENSE.txt for license information
@@ -13,23 +13,38 @@ import TSCBasic
1313
import TSCUtility
1414

1515
public enum Sandbox {
16+
17+
/// Applies a sandbox invocation to the given command line (if the platform supports it),
18+
/// and returns the modified command line. On platforms that don't support sandboxing, the
19+
/// command line is returned unmodified.
20+
///
21+
/// - Parameters:
22+
/// - command: The command line to sandbox (including executable as first argument)
23+
/// - strictness: The basic strictness level of the standbox.
24+
/// - writableDirectories: Paths under which writing should be allowed.
25+
/// - readOnlyDirectories: Paths under which writing should be denied, even if they would have otherwise been allowed by either the strictness level or paths in `writableDirectories`.
1626
public static func apply(
1727
command: [String],
28+
strictness: Strictness = .default,
1829
writableDirectories: [AbsolutePath] = [],
19-
strictness: Strictness = .default
30+
readOnlyDirectories: [AbsolutePath] = []
2031
) -> [String] {
2132
#if os(macOS)
22-
let profile = macOSSandboxProfile(writableDirectories: writableDirectories, strictness: strictness)
33+
let profile = macOSSandboxProfile(strictness: strictness, writableDirectories: writableDirectories, readOnlyDirectories: readOnlyDirectories)
2334
return ["/usr/bin/sandbox-exec", "-p", profile] + command
2435
#else
2536
// rdar://40235432, rdar://75636874 tracks implementing sandboxes for other platforms.
2637
return command
2738
#endif
2839
}
2940

41+
/// Basic strictness level of a sandbox applied to a command line.
3042
public enum Strictness: Equatable {
43+
/// Blocks network access and all file system modifications.
3144
case `default`
45+
/// More lenient restrictions than the default, for compatibility with SwiftPM manifests using a tools version older than 5.3.
3246
case manifest_pre_53 // backwards compatibility for manifests
47+
/// Like `default`, but also makes temporary-files directories (such as `/tmp`) on the platform writable.
3348
case writableTemporaryDirectory
3449
}
3550
}
@@ -38,8 +53,9 @@ public enum Sandbox {
3853

3954
#if os(macOS)
4055
fileprivate func macOSSandboxProfile(
56+
strictness: Sandbox.Strictness,
4157
writableDirectories: [AbsolutePath],
42-
strictness: Sandbox.Strictness
58+
readOnlyDirectories: [AbsolutePath]
4359
) -> String {
4460
var contents = "(version 1)\n"
4561

@@ -80,6 +96,7 @@ fileprivate func macOSSandboxProfile(
8096
}
8197
}
8298

99+
// Emit rules for paths under which writing is allowed. Some of these expressions may be regular expressions and others literal subpaths.
83100
if writableDirectoriesExpression.count > 0 {
84101
contents += "(allow file-write*\n"
85102
for expression in writableDirectoriesExpression {
@@ -88,6 +105,15 @@ fileprivate func macOSSandboxProfile(
88105
contents += ")\n"
89106
}
90107

108+
// Emit rules for paths under which writing should be disallowed, even if they would be covered by a previous rule to allow writing to them. A classic case is a package which is located under the temporary directory, which should be read-only even though the temporary directory as a whole is writable.
109+
if readOnlyDirectories.count > 0 {
110+
contents += "(deny file-write*\n"
111+
for path in readOnlyDirectories {
112+
contents += " (subpath \(resolveSymlinks(path).quotedAsSubpathForSandboxProfile))\n"
113+
}
114+
contents += ")\n"
115+
}
116+
91117
return contents
92118
}
93119

Sources/Build/BuildOperation.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,7 +344,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
344344
// TODO: We need to also use any working directory, but that support isn't yet available on all platforms at a lower level.
345345
var commandLine = [command.configuration.executable.pathString] + command.configuration.arguments
346346
if !self.disableSandboxForPluginCommands {
347-
commandLine = Sandbox.apply(command: commandLine, writableDirectories: [pluginResult.pluginOutputDirectory], strictness: .writableTemporaryDirectory)
347+
commandLine = Sandbox.apply(command: commandLine, strictness: .writableTemporaryDirectory, writableDirectories: [pluginResult.pluginOutputDirectory])
348348
}
349349
let processResult = try Process.popen(arguments: commandLine, environment: command.configuration.environment)
350350
let output = try processResult.utf8Output() + processResult.utf8stderrOutput()

Sources/Build/LLBuildManifestBuilder.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -618,7 +618,7 @@ extension LLBuildManifestBuilder {
618618
let displayName = command.configuration.displayName ?? execPath.basename
619619
var commandLine = [execPath.pathString] + command.configuration.arguments
620620
if !self.disableSandboxForPluginCommands {
621-
commandLine = Sandbox.apply(command: commandLine, writableDirectories: [result.pluginOutputDirectory], strictness: .writableTemporaryDirectory)
621+
commandLine = Sandbox.apply(command: commandLine, strictness: .writableTemporaryDirectory, writableDirectories: [result.pluginOutputDirectory])
622622
}
623623
manifest.addShellCmd(
624624
name: displayName + "-" + ByteString(encodingAsUTF8: uniquedName).sha256Checksum,

Sources/Commands/SwiftPackageTool.swift

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -906,10 +906,6 @@ extension SwiftPackageTool {
906906
help: "Allow the plugin to write to an additional directory")
907907
var additionalAllowedWritableDirectories: [String] = []
908908

909-
@Flag(name: .customLong("block-writing-to-temporary-directory"),
910-
help: "Block the plugin from writing to the package directory (by default this is allowed)")
911-
var blockWritingToTemporaryDirectory: Bool = false
912-
913909
@Argument(parsing: .unconditionalRemaining,
914910
help: "Name and arguments of the command plugin to invoke")
915911
var pluginCommand: [String] = []
@@ -990,13 +986,6 @@ extension SwiftPackageTool {
990986
if allowWritingToPackageDirectory {
991987
writableDirectories.append(package.path)
992988
}
993-
if !blockWritingToTemporaryDirectory {
994-
writableDirectories.append(AbsolutePath("/tmp"))
995-
writableDirectories.append(AbsolutePath(NSTemporaryDirectory()))
996-
}
997-
if allowWritingToPackageDirectory {
998-
writableDirectories.append(package.path)
999-
}
1000989
else {
1001990
// If the plugin requires write permission but it wasn't provided, we ask the user for approval.
1002991
if case .command(_, let permissions) = plugin.capability {
@@ -1010,6 +999,9 @@ extension SwiftPackageTool {
1010999
writableDirectories.append(AbsolutePath(pathString, relativeTo: swiftTool.originalWorkingDirectory))
10111000
}
10121001

1002+
// Make sure that the package path is read-only unless it's covered by any of the explicitly writable directories.
1003+
let readOnlyDirectories = writableDirectories.contains{ package.path.isDescendantOfOrEqual(to: $0) } ? [] : [package.path]
1004+
10131005
// Use the directory containing the compiler as an additional search directory, and add the $PATH.
10141006
let toolSearchDirs = [try swiftTool.getToolchain().swiftCompilerPath.parentDirectory]
10151007
+ getEnvSearchPaths(pathString: ProcessEnv.path, currentWorkingDirectory: .none)
@@ -1062,6 +1054,7 @@ extension SwiftPackageTool {
10621054
toolSearchDirectories: toolSearchDirs,
10631055
toolNamesToPaths: toolNamesToPaths,
10641056
writableDirectories: writableDirectories,
1057+
readOnlyDirectories: readOnlyDirectories,
10651058
fileSystem: localFileSystem,
10661059
observabilityScope: swiftTool.observabilityScope,
10671060
callbackQueue: delegateQueue,

Sources/PackageLoading/ManifestLoader.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -900,7 +900,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
900900
if self.isManifestSandboxEnabled {
901901
let cacheDirectories = [self.databaseCacheDir, moduleCachePath].compactMap{ $0 }
902902
let strictness: Sandbox.Strictness = toolsVersion < .v5_3 ? .manifest_pre_53 : .default
903-
cmd = Sandbox.apply(command: cmd, writableDirectories: cacheDirectories, strictness: strictness)
903+
cmd = Sandbox.apply(command: cmd, strictness: strictness, writableDirectories: cacheDirectories)
904904
}
905905

906906
// Run the compiled manifest.

Sources/SPMBuildCore/PluginInvocation.swift

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ extension PluginTarget {
5454
toolSearchDirectories: [AbsolutePath],
5555
toolNamesToPaths: [String: AbsolutePath],
5656
writableDirectories: [AbsolutePath],
57+
readOnlyDirectories: [AbsolutePath],
5758
fileSystem: FileSystem,
5859
observabilityScope: ObservabilityScope,
5960
callbackQueue: DispatchQueue,
@@ -90,6 +91,7 @@ extension PluginTarget {
9091
input: inputStruct,
9192
toolsVersion: self.apiVersion,
9293
writableDirectories: writableDirectories,
94+
readOnlyDirectories: readOnlyDirectories,
9395
fileSystem: fileSystem,
9496
observabilityScope: observabilityScope,
9597
callbackQueue: callbackQueue,
@@ -178,12 +180,11 @@ extension PackageGraph {
178180
// Assign a plugin working directory based on the package, target, and plugin.
179181
let pluginOutputDir = outputDir.appending(components: package.identity.description, target.name, pluginTarget.name)
180182

181-
// Determine the set of directories under which plugins are allowed to write. We always include the output directory and temporary directories, and for now there is no possibility of opting into others.
182-
let writableDirectories = [
183-
outputDir,
184-
AbsolutePath("/tmp"),
185-
AbsolutePath(NSTemporaryDirectory())
186-
]
183+
// Determine the set of directories under which plugins are allowed to write. We always include just the output directory, and for now there is no possibility of opting into others.
184+
let writableDirectories = [outputDir]
185+
186+
// Determine a set of further directories under which plugins are never allowed to write, even if they are covered by other rules (such as being able to write to the temporary directory).
187+
let readOnlyDirectories = [package.path]
187188

188189
// Set up a delegate to handle callbacks from the build tool plugin. We'll capture free-form text output as well as defined commands and diagnostics.
189190
let delegateQueue = DispatchQueue(label: "plugin-invocation")
@@ -245,6 +246,7 @@ extension PackageGraph {
245246
toolSearchDirectories: toolSearchDirectories,
246247
toolNamesToPaths: toolNamesToPaths,
247248
writableDirectories: writableDirectories,
249+
readOnlyDirectories: readOnlyDirectories,
248250
fileSystem: fileSystem,
249251
observabilityScope: observabilityScope,
250252
callbackQueue: delegateQueue,
@@ -389,6 +391,7 @@ public protocol PluginScriptRunner {
389391
input: PluginScriptRunnerInput,
390392
toolsVersion: ToolsVersion,
391393
writableDirectories: [AbsolutePath],
394+
readOnlyDirectories: [AbsolutePath],
392395
fileSystem: FileSystem,
393396
observabilityScope: ObservabilityScope,
394397
callbackQueue: DispatchQueue,

Sources/Workspace/DefaultPluginScriptRunner.swift

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
7171
input: PluginScriptRunnerInput,
7272
toolsVersion: ToolsVersion,
7373
writableDirectories: [AbsolutePath],
74+
readOnlyDirectories: [AbsolutePath],
7475
fileSystem: FileSystem,
7576
observabilityScope: ObservabilityScope,
7677
callbackQueue: DispatchQueue,
@@ -93,6 +94,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
9394
self.invoke(
9495
compiledExec: result.compiledExecutable,
9596
writableDirectories: writableDirectories,
97+
readOnlyDirectories: readOnlyDirectories,
9698
input: input,
9799
observabilityScope: observabilityScope,
98100
callbackQueue: callbackQueue,
@@ -342,6 +344,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
342344
fileprivate func invoke(
343345
compiledExec: AbsolutePath,
344346
writableDirectories: [AbsolutePath],
347+
readOnlyDirectories: [AbsolutePath],
345348
input: PluginScriptRunnerInput,
346349
observabilityScope: ObservabilityScope,
347350
callbackQueue: DispatchQueue,
@@ -356,9 +359,9 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
356359
// Construct the command line. Currently we just invoke the executable built from the plugin without any parameters.
357360
var command = [compiledExec.pathString]
358361

359-
// Optionally wrap the command in a sandbox, which places some limits on what it can do. In particular, it blocks network access and restricts the paths to which the plugin can make file system changes.
362+
// Optionally wrap the command in a sandbox, which places some limits on what it can do. In particular, it blocks network access and restricts the paths to which the plugin can make file system changes. It does allow writing to temporary directories.
360363
if self.enableSandbox {
361-
command = Sandbox.apply(command: command, writableDirectories: writableDirectories + [self.cacheDir], strictness: .default)
364+
command = Sandbox.apply(command: command, strictness: .writableTemporaryDirectory, writableDirectories: writableDirectories + [self.cacheDir], readOnlyDirectories: readOnlyDirectories)
362365
}
363366

364367
// Create and configure a Process. We set the working directory to the cache directory, so that relative paths end up there.

Tests/BasicsTests/SandboxTests.swift

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/*
22
This source file is part of the Swift.org open source project
33

4-
Copyright (c) 2021 Apple Inc. and the Swift project authors
4+
Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
55
Licensed under Apache License v2.0 with Runtime Library Exception
66

77
See http://swift.org/LICENSE.txt for license information
@@ -17,7 +17,7 @@ import XCTest
1717
final class SandboxTest: XCTestCase {
1818
func testSandboxOnAllPlatforms() throws {
1919
try withTemporaryDirectory { path in
20-
let command = Sandbox.apply(command: ["echo", "0"], writableDirectories: [], strictness: .default)
20+
let command = Sandbox.apply(command: ["echo", "0"], strictness: .default, writableDirectories: [])
2121
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
2222
}
2323
}
@@ -27,7 +27,7 @@ final class SandboxTest: XCTestCase {
2727
try XCTSkipIf(true, "test is only supported on macOS")
2828
#endif
2929

30-
let command = Sandbox.apply(command: ["ping", "-t", "1", "localhost"], writableDirectories: [], strictness: .default)
30+
let command = Sandbox.apply(command: ["ping", "-t", "1", "localhost"], strictness: .default, writableDirectories: [])
3131

3232
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
3333
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
@@ -43,7 +43,7 @@ final class SandboxTest: XCTestCase {
4343
#endif
4444

4545
try withTemporaryDirectory { path in
46-
let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], writableDirectories: [path], strictness: .default)
46+
let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], strictness: .default, writableDirectories: [path])
4747
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
4848
}
4949
}
@@ -54,7 +54,7 @@ final class SandboxTest: XCTestCase {
5454
#endif
5555

5656
try withTemporaryDirectory { path in
57-
let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], writableDirectories: [], strictness: .default)
57+
let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], strictness: .default, writableDirectories: [])
5858
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
5959
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
6060
return XCTFail("invalid error \(error)")
@@ -73,7 +73,7 @@ final class SandboxTest: XCTestCase {
7373
let file = path.appending(component: UUID().uuidString)
7474
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["touch", file.pathString]))
7575

76-
let command = Sandbox.apply(command: ["rm", file.pathString], writableDirectories: [], strictness: .default)
76+
let command = Sandbox.apply(command: ["rm", file.pathString], strictness: .default, writableDirectories: [])
7777
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
7878
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
7979
return XCTFail("invalid error \(error)")
@@ -93,7 +93,7 @@ final class SandboxTest: XCTestCase {
9393
let file = path.appending(component: UUID().uuidString)
9494
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["touch", file.pathString]))
9595

96-
let command = Sandbox.apply(command: ["cat", file.pathString], writableDirectories: [], strictness: .default)
96+
let command = Sandbox.apply(command: ["cat", file.pathString], strictness: .default, writableDirectories: [])
9797
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
9898
}
9999
}
@@ -109,7 +109,7 @@ final class SandboxTest: XCTestCase {
109109
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["touch", file.pathString]))
110110
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["chmod", "+x", file.pathString]))
111111

112-
let command = Sandbox.apply(command: [file.pathString], writableDirectories: [], strictness: .default)
112+
let command = Sandbox.apply(command: [file.pathString], strictness: .default, writableDirectories: [])
113113
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
114114
}
115115
}
@@ -131,4 +131,27 @@ final class SandboxTest: XCTestCase {
131131
try? FileManager.default.removeItem(atPath: tmpFile2)
132132
}
133133

134+
func testWritingToReadOnlyInsideWritableNotAllowed() throws {
135+
#if !os(macOS)
136+
try XCTSkipIf(true, "test is only supported on macOS")
137+
#endif
138+
139+
try withTemporaryDirectory { tmpDir in
140+
// Check that we can write into it, but not to a read-only directory underneath it.
141+
let writableDir = tmpDir.appending(component: "ShouldBeWritable")
142+
let allowedCommand = Sandbox.apply(command: ["mkdir", writableDir.pathString], strictness: .default, writableDirectories: [tmpDir])
143+
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: allowedCommand))
144+
145+
// Check that we cannot write into a read-only directory inside it.
146+
let readOnlyDir = writableDir.appending(component: "ShouldBeReadOnly")
147+
try localFileSystem.createDirectory(readOnlyDir)
148+
let deniedCommand = Sandbox.apply(command: ["touch", readOnlyDir.pathString], strictness: .default, writableDirectories: [tmpDir], readOnlyDirectories: [readOnlyDir])
149+
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: deniedCommand)) { error in
150+
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
151+
return XCTFail("invalid error \(error)")
152+
}
153+
XCTAssertMatch(try! result.utf8stderrOutput(), .contains("Operation not permitted"))
154+
}
155+
}
156+
}
134157
}

0 commit comments

Comments
 (0)