Sandbox blocks output to default plugin output directory when it's under <pkgdir>/.build

abertelrud · abertelrud · commit 0b631ed886e4 · 2022-01-11T19:14:17.000-08:00
The sandbox rules introduced in #3996 made that the entire package directory read-only, but that isn't appropriate when `.build` is inside the package directory. This changes the order of the operations so that the `allow` comes after the `deny`, but still after the temporary directories. In order to make this more flexible this coalesces the lists of writable and read-only directories into a single `pathRules` list of a new enum type that allows writing or read-only paths, and allows the caller to control the order. Also, writability of temporary directories is pulled into an option on the `default` strictness. rdar://87417780
diff --git a/Sources/Basics/Sandbox.swift b/Sources/Basics/Sandbox.swift
@@ -20,32 +20,34 @@ public enum Sandbox {
     ///
     /// - Parameters:
     ///   - command: The command line to sandbox (including executable as first argument)
-    ///   - strictness: The basic strictness level of the standbox.
-    ///   - writableDirectories: Paths under which writing should be allowed.
-    ///   - 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`.
+    ///   - strictness: The basic strictness level of the sandbox.
+    ///   - fileSystemRules: Ordered set of rules that allow reading or writing to particular paths in the file system (last one wins for a particular path).
     public static func apply(
         command: [String],
-        strictness: Strictness = .default,
-        writableDirectories: [AbsolutePath] = [],
-        readOnlyDirectories: [AbsolutePath] = []
+        strictness: Strictness = .default(writableTemporaryDirectory: false),
+        pathRules: [PathRule] = []
     ) -> [String] {
         #if os(macOS)
-        let profile = macOSSandboxProfile(strictness: strictness, writableDirectories: writableDirectories, readOnlyDirectories: readOnlyDirectories)
+        let profile = macOSSandboxProfile(strictness: strictness, pathRules: pathRules)
         return ["/usr/bin/sandbox-exec", "-p", profile] + command
         #else
         // rdar://40235432, rdar://75636874 tracks implementing sandboxes for other platforms.
         return command
         #endif
     }
 
+    /// Represents a readonly/writable rule for a path and everything under it.
+    public enum PathRule: Equatable {
+        case readonly(path: AbsolutePath)
+        case writable(path: AbsolutePath)
+    }
+
     /// Basic strictness level of a sandbox applied to a command line.
     public enum Strictness: Equatable {
-        /// Blocks network access and all file system modifications.
-        case `default`
+        /// Blocks network access and all file system modifications except, optionally, temporary locations.
+        case `default`(writableTemporaryDirectory: Bool)
         /// More lenient restrictions than the default, for compatibility with SwiftPM manifests using a tools version older than 5.3.
         case manifest_pre_53 // backwards compatibility for manifests
-        /// Like `default`, but also makes temporary-files directories (such as `/tmp`) on the platform writable.
-        case writableTemporaryDirectory
     }
 }
 
@@ -54,8 +56,7 @@ public enum Sandbox {
 #if os(macOS)
 fileprivate func macOSSandboxProfile(
     strictness: Sandbox.Strictness,
-    writableDirectories: [AbsolutePath],
-    readOnlyDirectories: [AbsolutePath]
+    pathRules: [Sandbox.PathRule]
 ) -> String {
     var contents = "(version 1)\n"
 
@@ -78,40 +79,34 @@ fileprivate func macOSSandboxProfile(
         contents += "(allow sysctl*)\n"
     }
 
-    // Allow writing only to certain directories.
-    var writableDirectoriesExpression = writableDirectories.map {
-        "(subpath \(resolveSymlinks($0).quotedAsSubpathForSandboxProfile))"
-    }
     // The following accesses are only needed when interpreting the manifest (versus running a compiled version).
-    if strictness == .manifest_pre_53 {
-        writableDirectoriesExpression += Platform.threadSafeDarwinCacheDirectories.get().map {
-            ##"(regex #"^\##($0.pathString)/org\.llvm\.clang.*")"##
-        }
-    }
-    // Optionally allow writing to temporary directories (a lot of use of Foundation requires this).
-    else if strictness == .writableTemporaryDirectory {
-        // Add `subpath` expressions for the regular and the Foundation temporary directories.
-        for tmpDir in ["/tmp", NSTemporaryDirectory()] {
-            writableDirectoriesExpression += ["(subpath \(resolveSymlinks(AbsolutePath(tmpDir)).quotedAsSubpathForSandboxProfile))"]
-        }
-    }
-
-    // Emit rules for paths under which writing is allowed. Some of these expressions may be regular expressions and others literal subpaths.
-    if writableDirectoriesExpression.count > 0 {
+    switch strictness {
+    case .manifest_pre_53:
+        // SwiftPM 5.3 and older allowed writing to some other cache directories.
         contents += "(allow file-write*\n"
-        for expression in writableDirectoriesExpression {
-            contents += "    \(expression)\n"
+        for path in Platform.threadSafeDarwinCacheDirectories.get() {
+            contents += "    (regex #\"^\(resolveSymlinks(path).pathString)/org\\.llvm\\.clang.*\")\n"
         }
         contents += ")\n"
+    case .default(let writableTemporaryDirectory):
+        // Optionally allow writing to the platform-specific temporary directories.
+        if writableTemporaryDirectory {
+            contents += "(allow file-write*\n"
+            for path in ["/tmp", NSTemporaryDirectory()] {
+                contents += "    (subpath \(resolveSymlinks(AbsolutePath(path)).quotedAsSubpathForSandboxProfile))"
+            }
+            contents += ")\n"
+        }
     }
 
-    // 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.
-    if readOnlyDirectories.count > 0 {
-        contents += "(deny file-write*\n"
-        for path in readOnlyDirectories {
-            contents += "    (subpath \(resolveSymlinks(path).quotedAsSubpathForSandboxProfile))\n"
+    // Emit rules for specific file system locations. Everything is readonly by default, so we just allow or deny writing, in order.
+    for rule in pathRules {
+        switch rule {
+        case .readonly(let path):
+            contents += "(deny file-write* (subpath \(resolveSymlinks(path).quotedAsSubpathForSandboxProfile)))\n"
+        case .writable(let path):
+            contents += "(allow file-write* (subpath \(resolveSymlinks(path).quotedAsSubpathForSandboxProfile)))\n"
         }
-        contents += ")\n"
     }
 
     return contents
diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift
@@ -344,7 +344,10 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
                 // TODO: We need to also use any working directory, but that support isn't yet available on all platforms at a lower level.
                 var commandLine = [command.configuration.executable.pathString] + command.configuration.arguments
                 if !self.disableSandboxForPluginCommands {
-                    commandLine = Sandbox.apply(command: commandLine, strictness: .writableTemporaryDirectory, writableDirectories: [pluginResult.pluginOutputDirectory])
+                    commandLine = Sandbox.apply(
+                        command: commandLine,
+                        strictness: .default(writableTemporaryDirectory: true),
+                        pathRules: [.writable(path: pluginResult.pluginOutputDirectory)])
                 }
                 let processResult = try Process.popen(arguments: commandLine, environment: command.configuration.environment)
                 let output = try processResult.utf8Output() + processResult.utf8stderrOutput()
diff --git a/Sources/Build/LLBuildManifestBuilder.swift b/Sources/Build/LLBuildManifestBuilder.swift
@@ -618,7 +618,10 @@ extension LLBuildManifestBuilder {
                 let displayName = command.configuration.displayName ?? execPath.basename
                 var commandLine = [execPath.pathString] + command.configuration.arguments
                 if !self.disableSandboxForPluginCommands {
-                    commandLine = Sandbox.apply(command: commandLine, strictness: .writableTemporaryDirectory, writableDirectories: [result.pluginOutputDirectory])
+                    commandLine = Sandbox.apply(
+                        command: commandLine,
+                        strictness: .default(writableTemporaryDirectory: true),
+                        pathRules: [.writable(path: result.pluginOutputDirectory)])
                 }
                 manifest.addShellCmd(
                     name: displayName + "-" + ByteString(encodingAsUTF8: uniquedName).sha256Checksum,
diff --git a/Sources/PackageLoading/ManifestLoader.swift b/Sources/PackageLoading/ManifestLoader.swift
@@ -899,8 +899,8 @@ public final class ManifestLoader: ManifestLoaderProtocol {
                     // We only allow the permissions which are absolutely necessary.
                     if self.isManifestSandboxEnabled {
                         let cacheDirectories = [self.databaseCacheDir, moduleCachePath].compactMap{ $0 }
-                        let strictness: Sandbox.Strictness = toolsVersion < .v5_3 ? .manifest_pre_53 : .default
-                        cmd = Sandbox.apply(command: cmd, strictness: strictness, writableDirectories: cacheDirectories)
+                        let strictness: Sandbox.Strictness = toolsVersion < .v5_3 ? .manifest_pre_53 : .default(writableTemporaryDirectory: false)
+                        cmd = Sandbox.apply(command: cmd, strictness: strictness, pathRules: cacheDirectories.map{ .writable(path: $0) })
                     }
 
                     // Run the compiled manifest.
diff --git a/Sources/Workspace/DefaultPluginScriptRunner.swift b/Sources/Workspace/DefaultPluginScriptRunner.swift
@@ -361,7 +361,10 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
 
         // 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.
         if self.enableSandbox {
-            command = Sandbox.apply(command: command, strictness: .writableTemporaryDirectory, writableDirectories: writableDirectories + [self.cacheDir], readOnlyDirectories: readOnlyDirectories)
+            command = Sandbox.apply(
+                command: command,
+                strictness: .default(writableTemporaryDirectory: true),
+                pathRules: readOnlyDirectories.map{ .readonly(path: $0) } + (writableDirectories + [self.cacheDir]).map{ .writable(path: $0) })
         }
 
         // Create and configure a Process. We set the working directory to the cache directory, so that relative paths end up there.
diff --git a/Tests/BasicsTests/SandboxTests.swift b/Tests/BasicsTests/SandboxTests.swift
@@ -17,7 +17,9 @@ import XCTest
 final class SandboxTest: XCTestCase {
     func testSandboxOnAllPlatforms() throws {
         try withTemporaryDirectory { path in
-            let command = Sandbox.apply(command: ["echo", "0"], strictness: .default, writableDirectories: [])
+            let command = Sandbox.apply(
+                command: ["echo", "0"],
+                strictness: .default(writableTemporaryDirectory: false))
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
         }
     }
@@ -27,7 +29,9 @@ final class SandboxTest: XCTestCase {
         try XCTSkipIf(true, "test is only supported on macOS")
         #endif
 
-        let command = Sandbox.apply(command: ["ping", "-t", "1", "localhost"], strictness: .default, writableDirectories: [])
+        let command = Sandbox.apply(
+            command: ["ping", "-t", "1", "localhost"],
+            strictness: .default(writableTemporaryDirectory: false))
 
         XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
             guard case ProcessResult.Error.nonZeroExit(let result) = error else {
@@ -43,7 +47,10 @@ final class SandboxTest: XCTestCase {
         #endif
 
         try withTemporaryDirectory { path in
-            let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], strictness: .default, writableDirectories: [path])
+            let command = Sandbox.apply(
+                command: ["touch", path.appending(component: UUID().uuidString).pathString],
+                strictness: .default(writableTemporaryDirectory: false),
+                pathRules: [.writable(path: path)])
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
         }
     }
@@ -54,7 +61,9 @@ final class SandboxTest: XCTestCase {
         #endif
 
         try withTemporaryDirectory { path in
-            let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], strictness: .default, writableDirectories: [])
+            let command = Sandbox.apply(
+                command: ["touch", path.appending(component: UUID().uuidString).pathString],
+                strictness: .default(writableTemporaryDirectory: false))
             XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
                 guard case ProcessResult.Error.nonZeroExit(let result) = error else {
                     return XCTFail("invalid error \(error)")
@@ -73,7 +82,9 @@ final class SandboxTest: XCTestCase {
             let file = path.appending(component: UUID().uuidString)
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["touch", file.pathString]))
 
-            let command = Sandbox.apply(command: ["rm", file.pathString], strictness: .default, writableDirectories: [])
+            let command = Sandbox.apply(
+                command: ["rm", file.pathString],
+                strictness: .default(writableTemporaryDirectory: false))
             XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
                 guard case ProcessResult.Error.nonZeroExit(let result) = error else {
                     return XCTFail("invalid error \(error)")
@@ -93,7 +104,9 @@ final class SandboxTest: XCTestCase {
             let file = path.appending(component: UUID().uuidString)
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["touch", file.pathString]))
 
-            let command = Sandbox.apply(command: ["cat", file.pathString], strictness: .default, writableDirectories: [])
+            let command = Sandbox.apply(
+                command: ["cat", file.pathString],
+                strictness: .default(writableTemporaryDirectory: false))
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
         }
     }
@@ -109,7 +122,9 @@ final class SandboxTest: XCTestCase {
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["touch", file.pathString]))
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: ["chmod", "+x", file.pathString]))
 
-            let command = Sandbox.apply(command: [file.pathString], strictness: .default, writableDirectories: [])
+            let command = Sandbox.apply(
+                command: [file.pathString],
+                strictness: .default(writableTemporaryDirectory: false))
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
         }
     }
@@ -121,12 +136,16 @@ final class SandboxTest: XCTestCase {
 
         // Try writing to the per-user temporary directory, which is under /var/folders/.../TemporaryItems.
         let tmpFile1 = NSTemporaryDirectory() + "/" + UUID().uuidString
-        let command1 = Sandbox.apply(command: ["touch", tmpFile1], strictness: .writableTemporaryDirectory)
+        let command1 = Sandbox.apply(
+            command: ["touch", tmpFile1],
+            strictness: .default(writableTemporaryDirectory: true))
         XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command1))
         try? FileManager.default.removeItem(atPath: tmpFile1)
 
         let tmpFile2 = "/tmp" + "/" + UUID().uuidString
-        let command2 = Sandbox.apply(command: ["touch", tmpFile2], strictness: .writableTemporaryDirectory)
+        let command2 = Sandbox.apply(
+            command: ["touch", tmpFile2],
+            strictness: .default(writableTemporaryDirectory: true))
         XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command2))
         try? FileManager.default.removeItem(atPath: tmpFile2)
     }
@@ -139,13 +158,19 @@ final class SandboxTest: XCTestCase {
         try withTemporaryDirectory { tmpDir in
             // Check that we can write into it, but not to a read-only directory underneath it.
             let writableDir = tmpDir.appending(component: "ShouldBeWritable")
-            let allowedCommand = Sandbox.apply(command: ["mkdir", writableDir.pathString], strictness: .default, writableDirectories: [tmpDir])
+            let allowedCommand = Sandbox.apply(
+                command: ["mkdir", writableDir.pathString],
+                strictness: .default(writableTemporaryDirectory: false),
+                pathRules: [.writable(path: tmpDir)])
             XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: allowedCommand))
 
             // Check that we cannot write into a read-only directory inside it.
             let readOnlyDir = writableDir.appending(component: "ShouldBeReadOnly")
             try localFileSystem.createDirectory(readOnlyDir)
-            let deniedCommand = Sandbox.apply(command: ["touch", readOnlyDir.pathString], strictness: .default, writableDirectories: [tmpDir], readOnlyDirectories: [readOnlyDir])
+            let deniedCommand = Sandbox.apply(
+                command: ["touch", readOnlyDir.pathString],
+                strictness: .default(writableTemporaryDirectory: false),
+                pathRules: [.writable(path: tmpDir), .readonly(path: readOnlyDir)])
             XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: deniedCommand)) { error in
                 guard case ProcessResult.Error.nonZeroExit(let result) = error else {
                     return XCTFail("invalid error \(error)")
diff --git a/Tests/CommandsTests/PackageToolTests.swift b/Tests/CommandsTests/PackageToolTests.swift
