-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Improve flexibility of sandbox path rules, and make sandbox profile a struct that's easier to manage #5857
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Improve flexibility of sandbox path rules, and make sandbox profile a struct that's easier to manage #5857
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
This source file is part of the Swift.org open source project | ||
|
||
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 | ||
See http://swift.org/CONTRIBUTORS.txt for Swift project authors | ||
*/ | ||
|
||
import Foundation | ||
import TSCBasic | ||
|
||
/// A sandbox profile representing the desired restrictions. The implementation can vary between platforms. Currently | ||
/// the only control a client has is in the path rules, but in the future there should also be options for controlling | ||
/// blocking of network access and process launching. | ||
public struct SandboxProfile: Equatable { | ||
/// An ordered list of path rules, where the last rule to cover a particular path "wins". These will be resolved | ||
/// to absolute paths at the time the profile is applied. They are applied after any of the implicit directories | ||
/// referenced by other sandbox profile settings. | ||
public var pathAccessRules: [PathAccessRule] | ||
|
||
/// Represents a rule for access to a path and everything under it. | ||
public enum PathAccessRule: Equatable { | ||
case noaccess(AbsolutePath) | ||
case readonly(AbsolutePath) | ||
case writable(AbsolutePath) | ||
} | ||
|
||
/// Configures a SandboxProfile for blocking network access and writing to the file system except to specifically | ||
/// permitted locations. | ||
public init(_ pathAccessRules: [PathAccessRule] = []) { | ||
self.pathAccessRules = pathAccessRules | ||
} | ||
} | ||
|
||
extension SandboxProfile { | ||
/// Applies the sandbox profile to the given command line (if the platform supports it), and returns the modified | ||
/// command line. On platforms that don't support sandboxing, the unmodified command line is returned. | ||
public func apply(to command: [String]) throws -> [String] { | ||
#if os(macOS) | ||
return ["/usr/bin/sandbox-exec", "-p", try self.generateMacOSSandboxProfileString()] + command | ||
#else | ||
// rdar://40235432, rdar://75636874 tracks implementing sandboxes for other platforms. | ||
return command | ||
#endif | ||
} | ||
} | ||
|
||
// MARK: - macOS | ||
|
||
#if os(macOS) | ||
fileprivate extension SandboxProfile { | ||
/// Private function that generates a Darwin sandbox profile suitable for passing to `sandbox-exec(1)`. | ||
func generateMacOSSandboxProfileString() throws -> String { | ||
var contents = "(version 1)\n" | ||
|
||
// Deny everything by default. | ||
contents += "(deny default)\n" | ||
|
||
// Import the system sandbox profile. | ||
contents += "(import \"system.sb\")\n" | ||
|
||
// Allow operations on subprocesses. | ||
contents += "(allow process*)\n" | ||
|
||
// Allow reading any file that isn't protected by TCC or permissions (ideally we'd only allow a specific set | ||
// of readable locations, and can maybe tighten this in the future). | ||
contents += "(allow file-read*)\n" | ||
|
||
// Apply customized rules for specific file system locations. Everything is readonly by default, so we just | ||
// either allow or deny writing, in order. Later rules have precedence over earlier rules. | ||
for rule in pathAccessRules { | ||
switch rule { | ||
case .noaccess(let path): | ||
contents += "(deny file-* (subpath \(try resolveSymlinksAndQuotePath(path))))\n" | ||
case .readonly(let path): | ||
contents += "(deny file-write* (subpath \(try resolveSymlinksAndQuotePath(path))))\n" | ||
case .writable(let path): | ||
contents += "(allow file-write* (subpath \(try resolveSymlinksAndQuotePath(path))))\n" | ||
} | ||
} | ||
return contents | ||
} | ||
|
||
/// Private helper function that resolves an AbsolutePath and returns it as a string quoted for use as a subpath | ||
/// in a .sb sandbox profile. | ||
func resolveSymlinksAndQuotePath(_ path: AbsolutePath) throws -> String { | ||
return try "\"" + resolveSymlinks(path).pathString | ||
.replacingOccurrences(of: "\\", with: "\\\\") | ||
.replacingOccurrences(of: "\"", with: "\\\"") | ||
+ "\"" | ||
} | ||
} | ||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -708,10 +708,29 @@ public final class ManifestLoader: ManifestLoaderProtocol { | |
// This provides some safety against arbitrary code execution when parsing manifest files. | ||
// 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 | ||
do { | ||
cmd = try Sandbox.apply(command: cmd, strictness: strictness, writableDirectories: cacheDirectories) | ||
var sandbox = SandboxProfile() | ||
|
||
// Allow writing inside the temporary directories. | ||
sandbox.pathAccessRules.append(.writable(try AbsolutePath(validating: "/tmp"))) | ||
sandbox.pathAccessRules.append(.writable(try localFileSystem.tempDirectory)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we consider changing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I'd want to change the I agree that we'd want to avoid the duplication. In this case, I think the hope is that we can remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about something like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having discussed this further offline, I think a improvement for this PR would be to follow Tom's suggestion of encapsulating the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having thought some more about this, maybe the right thing to do here is to reintroduce the option for whether or not "temporary directories are writable" as a The construction of these paths could then be pulled into a single place in the sandbox profile, and clients just get to choose whether or not to enable writing to temporary directories. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
|
||
// Allow writing in the database cache directory, if we have one. | ||
if let databaseCacheDir = self.databaseCacheDir { | ||
sandbox.pathAccessRules.append(.writable(databaseCacheDir)) | ||
} | ||
|
||
// Allow writing in the module cache path, if there is one. | ||
if let moduleCachePath = moduleCachePath { | ||
sandbox.pathAccessRules.append(.writable(moduleCachePath)) | ||
} | ||
|
||
// But do not allow writing in the directory that contains the manifest, even if it is | ||
// inside one of the otherwise writable directories. | ||
sandbox.pathAccessRules.append(.readonly(manifestPath.parentDirectory)) | ||
|
||
// Finally apply the sandbox. | ||
cmd = try sandbox.apply(to: cmd) | ||
} catch { | ||
return completion(.failure(error)) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't as material as it looks. The pre-5.3 special case in the Sandbox was to allow the manifest to run interpreted, which is something that no longer happens regardless of the tools version. This controlled writability to
org.llvm.clang
cache directories etc, and is not likely to affect existing manifests.There is actually a question of whether sandbox rule changes should be based on tools version at all, as this makes it very easy to circumvent tightened sandboxes even in new builds of SwiftPM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably double-check the assumption that this doesn't affect existing libraries by running through our various compatibility suites. I agree in principle that the rules shouldn't depend on the tools-version, on the other hand we need to make sure we don't break existing packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point.
The other approach would be to add back the basic knobs-and-switches controlled by that specialized setting and just apply them at the call site. I mainly wanted to get the "special sauce" out of the SandboxProfile type.