Skip to content

Enforce read-only-ness of package directory even when it's inside writable directories #3996

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 3 commits into from
Jan 9, 2022
Merged

Enforce read-only-ness of package directory even when it's inside writable directories #3996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82e773f
Enforce read-only-ness of package directory even when it's inside /tm…
abertelrud Jan 8, 2022
00a636e
Remove a stray line from `Sources/Basics/Sandbox.swift`
abertelrud Jan 8, 2022
d69d1d1
Remove a stray print statement
abertelrud Jan 8, 2022
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
34 changes: 30 additions & 4 deletions Sources/Basics/Sandbox.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
This source file is part of the Swift.org open source project

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

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

public enum Sandbox {

/// Applies a sandbox invocation to the given command line (if the platform supports it),
/// and returns the modified command line. On platforms that don't support sandboxing, the
/// command line is returned unmodified.
///
/// - 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`.
public static func apply(
command: [String],
strictness: Strictness = .default,
writableDirectories: [AbsolutePath] = [],
strictness: Strictness = .default
readOnlyDirectories: [AbsolutePath] = []
) -> [String] {
#if os(macOS)
let profile = macOSSandboxProfile(writableDirectories: writableDirectories, strictness: strictness)
let profile = macOSSandboxProfile(strictness: strictness, writableDirectories: writableDirectories, readOnlyDirectories: readOnlyDirectories)
return ["/usr/bin/sandbox-exec", "-p", profile] + command
#else
// rdar://40235432, rdar://75636874 tracks implementing sandboxes for other platforms.
return command
#endif
}

/// 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`
/// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

would it better to have the call site pass the temp dirs as writable locations instead of defining a new strictness option for that? alternatively, should this be an argument on "default" strictness instead of it's own case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that (passing in the temporary directories from the client) initially actually, but that pushes the platform-specific nature of these paths onto the client. In addition to the paths, there could actually be a varying number of them on different platforms. But I agree that this shouldn't really be part of the strictness, but rather an option on the default strictness as you suggest. I think there are more of those in the future, such as whether or not to allow network, etc.

So yes, in retrospect I think it was a mistake to add the temporary directory handling as a separate strictness level, and it's something to be unwound in the future.

Really what I'd like to do is to change the Sandbox struct to a SandboxProfile or some other name that suggests that it's a set of options, and then that profile could be configured once and passed around so it can be applied to multiple calls later without having to thread through all the individual parameters (and perhaps also cached using operating system calls based on the contents of that profile). Then as we add more parameters, none of the intermediate code has to change. The code that calls the subprocess could just apply the same profile to one command after another. I'm hoping to get to that improvement soonish, in particular since I'm hopeful that it could allow compilation of the profile on macOS just once and the reuse of the compiled profile, saving time when starting sandboxed subprocesses.

At that point it would make sense to get rid of strictness, I think, and just have a set of individually controllable options, and then a convenience initializer to create a SandboxProfile that matches the SwiftPM 5.3 semantics. That special case could then exist outside of the SandboxProfile code itself as an extension, which seems more correct than leaking it into the sandbox API.

All of course for a future PR, but hopefully not too long in the future.

}
}
Expand All @@ -38,8 +53,9 @@ public enum Sandbox {

#if os(macOS)
fileprivate func macOSSandboxProfile(
strictness: Sandbox.Strictness,
writableDirectories: [AbsolutePath],
strictness: Sandbox.Strictness
readOnlyDirectories: [AbsolutePath]
) -> String {
var contents = "(version 1)\n"

Expand Down Expand Up @@ -80,6 +96,7 @@ fileprivate func macOSSandboxProfile(
}
}

// 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 {
contents += "(allow file-write*\n"
for expression in writableDirectoriesExpression {
Expand All @@ -88,6 +105,15 @@ fileprivate func macOSSandboxProfile(
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"
}
contents += ")\n"
}

return contents
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/Build/BuildOperation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ 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, writableDirectories: [pluginResult.pluginOutputDirectory], strictness: .writableTemporaryDirectory)
commandLine = Sandbox.apply(command: commandLine, strictness: .writableTemporaryDirectory, writableDirectories: [pluginResult.pluginOutputDirectory])
}
let processResult = try Process.popen(arguments: commandLine, environment: command.configuration.environment)
let output = try processResult.utf8Output() + processResult.utf8stderrOutput()
Expand Down
2 changes: 1 addition & 1 deletion Sources/Build/LLBuildManifestBuilder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ extension LLBuildManifestBuilder {
let displayName = command.configuration.displayName ?? execPath.basename
var commandLine = [execPath.pathString] + command.configuration.arguments
if !self.disableSandboxForPluginCommands {
commandLine = Sandbox.apply(command: commandLine, writableDirectories: [result.pluginOutputDirectory], strictness: .writableTemporaryDirectory)
commandLine = Sandbox.apply(command: commandLine, strictness: .writableTemporaryDirectory, writableDirectories: [result.pluginOutputDirectory])
}
manifest.addShellCmd(
name: displayName + "-" + ByteString(encodingAsUTF8: uniquedName).sha256Checksum,
Expand Down
15 changes: 4 additions & 11 deletions Sources/Commands/SwiftPackageTool.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,10 +895,6 @@ extension SwiftPackageTool {
help: "Allow the plugin to write to an additional directory")
var additionalAllowedWritableDirectories: [String] = []

@Flag(name: .customLong("block-writing-to-temporary-directory"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flag was just added, so nobody is depending on it yet, and it was mostly for unit tests — it seems much clearer to always make the package directory read-only, even for packages checked out to /tmp or other implicitly writable places.

help: "Block the plugin from writing to the package directory (by default this is allowed)")
var blockWritingToTemporaryDirectory: Bool = false

@Argument(parsing: .unconditionalRemaining,
help: "Name and arguments of the command plugin to invoke")
var pluginCommand: [String] = []
Expand Down Expand Up @@ -979,13 +975,6 @@ extension SwiftPackageTool {
if allowWritingToPackageDirectory {
writableDirectories.append(package.path)
}
if !blockWritingToTemporaryDirectory {
writableDirectories.append(AbsolutePath("/tmp"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The temporary directories are now again handled by the strictness.

writableDirectories.append(AbsolutePath(NSTemporaryDirectory()))
}
if allowWritingToPackageDirectory {
writableDirectories.append(package.path)
}
Comment on lines -986 to -988
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually redundant with lines 975-977 above.

else {
// If the plugin requires write permission but it wasn't provided, we ask the user for approval.
if case .command(_, let permissions) = plugin.capability {
Expand All @@ -999,6 +988,9 @@ extension SwiftPackageTool {
writableDirectories.append(AbsolutePath(pathString, relativeTo: swiftTool.originalWorkingDirectory))
}

// Make sure that the package path is read-only unless it's covered by any of the explicitly writable directories.
let readOnlyDirectories = writableDirectories.contains{ package.path.isDescendantOfOrEqual(to: $0) } ? [] : [package.path]

Comment on lines +991 to +993
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is subject to debate. It avoids marking the package path as readonly if it's covered by any of the directories marked as explicitly writable. Maybe what should be done here in the long term is to have a single list of directory-specific permissions, each of which could be either block or allow and then the caller can craft their sandbox directories as they want to. For example, it might make sense to allow a plugin to modify only a subdirectory of the package. We can extend this in the future — probably better to keep it simple for now.

Copy link
Contributor

@tomerd tomerd Jan 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

starting simple makes sense to me. having the sandbox API accepts a list of allow/deny and having the call site define that make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

// Use the directory containing the compiler as an additional search directory, and add the $PATH.
let toolSearchDirs = [try swiftTool.getToolchain().swiftCompilerPath.parentDirectory]
+ getEnvSearchPaths(pathString: ProcessEnv.path, currentWorkingDirectory: .none)
Expand Down Expand Up @@ -1051,6 +1043,7 @@ extension SwiftPackageTool {
toolSearchDirectories: toolSearchDirs,
toolNamesToPaths: toolNamesToPaths,
writableDirectories: writableDirectories,
readOnlyDirectories: readOnlyDirectories,
fileSystem: localFileSystem,
observabilityScope: swiftTool.observabilityScope,
callbackQueue: delegateQueue,
Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageLoading/ManifestLoader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
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, writableDirectories: cacheDirectories, strictness: strictness)
cmd = Sandbox.apply(command: cmd, strictness: strictness, writableDirectories: cacheDirectories)
}

// Run the compiled manifest.
Expand Down
15 changes: 9 additions & 6 deletions Sources/SPMBuildCore/PluginInvocation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ extension PluginTarget {
toolSearchDirectories: [AbsolutePath],
toolNamesToPaths: [String: AbsolutePath],
writableDirectories: [AbsolutePath],
readOnlyDirectories: [AbsolutePath],
fileSystem: FileSystem,
observabilityScope: ObservabilityScope,
callbackQueue: DispatchQueue,
Expand Down Expand Up @@ -90,6 +91,7 @@ extension PluginTarget {
input: inputStruct,
toolsVersion: self.apiVersion,
writableDirectories: writableDirectories,
readOnlyDirectories: readOnlyDirectories,
fileSystem: fileSystem,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue,
Expand Down Expand Up @@ -178,12 +180,11 @@ extension PackageGraph {
// Assign a plugin working directory based on the package, target, and plugin.
let pluginOutputDir = outputDir.appending(components: package.identity.description, target.name, pluginTarget.name)

// 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.
let writableDirectories = [
outputDir,
AbsolutePath("/tmp"),
AbsolutePath(NSTemporaryDirectory())
]
// 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.
let writableDirectories = [outputDir]

// 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).
let readOnlyDirectories = [package.path]

// 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.
let delegateQueue = DispatchQueue(label: "plugin-invocation")
Expand Down Expand Up @@ -245,6 +246,7 @@ extension PackageGraph {
toolSearchDirectories: toolSearchDirectories,
toolNamesToPaths: toolNamesToPaths,
writableDirectories: writableDirectories,
readOnlyDirectories: readOnlyDirectories,
fileSystem: fileSystem,
observabilityScope: observabilityScope,
callbackQueue: delegateQueue,
Expand Down Expand Up @@ -389,6 +391,7 @@ public protocol PluginScriptRunner {
input: PluginScriptRunnerInput,
toolsVersion: ToolsVersion,
writableDirectories: [AbsolutePath],
readOnlyDirectories: [AbsolutePath],
fileSystem: FileSystem,
observabilityScope: ObservabilityScope,
callbackQueue: DispatchQueue,
Expand Down
7 changes: 5 additions & 2 deletions Sources/Workspace/DefaultPluginScriptRunner.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
input: PluginScriptRunnerInput,
toolsVersion: ToolsVersion,
writableDirectories: [AbsolutePath],
readOnlyDirectories: [AbsolutePath],
fileSystem: FileSystem,
observabilityScope: ObservabilityScope,
callbackQueue: DispatchQueue,
Expand All @@ -93,6 +94,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
self.invoke(
compiledExec: result.compiledExecutable,
writableDirectories: writableDirectories,
readOnlyDirectories: readOnlyDirectories,
input: input,
observabilityScope: observabilityScope,
callbackQueue: callbackQueue,
Expand Down Expand Up @@ -342,6 +344,7 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
fileprivate func invoke(
compiledExec: AbsolutePath,
writableDirectories: [AbsolutePath],
readOnlyDirectories: [AbsolutePath],
input: PluginScriptRunnerInput,
observabilityScope: ObservabilityScope,
callbackQueue: DispatchQueue,
Expand All @@ -356,9 +359,9 @@ public struct DefaultPluginScriptRunner: PluginScriptRunner {
// Construct the command line. Currently we just invoke the executable built from the plugin without any parameters.
var command = [compiledExec.pathString]

// 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.
// 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, writableDirectories: writableDirectories + [self.cacheDir], strictness: .default)
command = Sandbox.apply(command: command, strictness: .writableTemporaryDirectory, writableDirectories: writableDirectories + [self.cacheDir], readOnlyDirectories: readOnlyDirectories)
}

// Create and configure a Process. We set the working directory to the cache directory, so that relative paths end up there.
Expand Down
39 changes: 31 additions & 8 deletions Tests/BasicsTests/SandboxTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
This source file is part of the Swift.org open source project

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

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

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

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

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

try withTemporaryDirectory { path in
let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], writableDirectories: [], strictness: .default)
let command = Sandbox.apply(command: ["touch", path.appending(component: UUID().uuidString).pathString], strictness: .default, writableDirectories: [])
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
return XCTFail("invalid error \(error)")
Expand All @@ -73,7 +73,7 @@ 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], writableDirectories: [], strictness: .default)
let command = Sandbox.apply(command: ["rm", file.pathString], strictness: .default, writableDirectories: [])
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: command)) { error in
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
return XCTFail("invalid error \(error)")
Expand All @@ -93,7 +93,7 @@ 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], writableDirectories: [], strictness: .default)
let command = Sandbox.apply(command: ["cat", file.pathString], strictness: .default, writableDirectories: [])
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
}
}
Expand All @@ -109,7 +109,7 @@ 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], writableDirectories: [], strictness: .default)
let command = Sandbox.apply(command: [file.pathString], strictness: .default, writableDirectories: [])
XCTAssertNoThrow(try Process.checkNonZeroExit(arguments: command))
}
}
Expand All @@ -131,4 +131,27 @@ final class SandboxTest: XCTestCase {
try? FileManager.default.removeItem(atPath: tmpFile2)
}

func testWritingToReadOnlyInsideWritableNotAllowed() throws {
#if !os(macOS)
try XCTSkipIf(true, "test is only supported on macOS")
#endif

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])
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])
XCTAssertThrowsError(try Process.checkNonZeroExit(arguments: deniedCommand)) { error in
guard case ProcessResult.Error.nonZeroExit(let result) = error else {
return XCTFail("invalid error \(error)")
}
XCTAssertMatch(try! result.utf8stderrOutput(), .contains("Operation not permitted"))
}
}
}
}
Loading