Improve flexibility of sandbox path rules, and make sandbox profile a struct that's easier to manage #5857
Conversation
|
@swift-ci please smoke test |
|
Actually, I accidentally left in the part where we block |
abertelrud
force-pushed
the
eng/sandbox-improvements
branch
from
22aa4fd to
9ff90b0
Compare
|
@swift-ci please smoke test |
| @@ -635,10 +635,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 | |||
There was a problem hiding this comment.
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.
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.
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.
|
@swift-ci please test package compatibility |
|
Actually, I guess the SwiftCI package compatibility job doesn't help us because it disables sandboxing entirely? |
It should be enabled on Darwin AFAIK. |
|
|
||
| // 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.
should we consider changing FileSystem::tempDirectory to return an array that includes /tmp instead of adding it manually everywhere? alternatively, maybe a function in this area of the code?
There was a problem hiding this comment.
I'm not sure I'd want to change the tempDirectory function itself, since /tmp is discouraged (in favour of the per-user one that NSTemporaryDirectory() returns), and so making it an array adds complexity without any gain (IMO).
I agree that we'd want to avoid the duplication. In this case, I think the hope is that we can remove the /tmp hardcoding when we set TMPDIR in a follow-on PR, which I would actually want to do as an option on Process (along with adding the sandbox as an option). So the duplication here is fairly temporary, I hope.
There was a problem hiding this comment.
how about something like
// deprecate - for compatibility reason 12/2022
FileSystem._legacyTempDirecotry
There was a problem hiding this comment.
Having discussed this further offline, I think a improvement for this PR would be to follow Tom's suggestion of encapsulating the /tmp part into a FileSystem.legacyTemporaryDirectory or something similar. That way it's clear that it's disfavored compared with the regular temporary directory, but at least the hardcoding is mitigated. Then I also want to open that TSC.Process PR to add the support for the sandbox and for passing through access to the temporary directory there.
There was a problem hiding this comment.
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 SandboxProfile option in its own right (it was removed in a previous change in order to simplify the sandbox profile). I'm not all that keen on making FileSystem have two separate notions of a temporary directory, which seems conceptually confusing to most clients. The inclusion of /tmp here is really just for backward compatibility and so it seems unfortunate to add semi-deprecated API to FileSystem for it.
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.
… struct that's easier to manage Turn the stateless `Sandbox` enum into a `SandboxProfile` struct that's a bit more flexible: - there is a list of path rules allowing a mixed order of `writable`, `readonly`, and `noaccess` rules; this addresses an issue where writable directories were always applied after readable directories, preventing arrangements where, for example, a source directory inside the temporary directory was properly made readonly - the defaults are built into the initializer rather than a separate `strictness` parameter — this means that they can be queried once the SandboxProfile has been created and are expressed in terms of the choices available to all sandbox profiles - it's a bit more ergonomic (as a struct, it can be copied, mutated, and carried around in a property before being used), and an optional `SandboxProfile` can be used to indicate both whether or not to apply a profile and if so what to apply. Having the sandbox profile be a struct that generates the platform specifics as needed also could also provide a place with which to associate any cached/compiled representation for profiles that are largely static, for any platform that supports that. As cleanup, this commit also removes the pre-SwiftPM 5.3 specialities which were specific to running the package manifest in an interpreter rather than compiling and executing it. This functionality is no longer relevant since it isn't possible to run any manifest in the interpreter, no matter what tools version the package specifies. In this commit, the call sites have been adjusted so that they use the modified SandboxProfile API, but they still construct the profiles on-the-fly as before. A future change will make them instead be configured at an early point and then applied later when the sandboxed process is actually launched. Note also that the existing call sites still pass `/tmp` because that it was the sandbox has allowed up until now. A separate PR will deal with whether or not allowing writes to `/tmp` is apprporiate. Another future change could add the sandbox profile to `Process` as a property so that there is no API assumption that applying a sandbox necessarily involves just modifying the command line.
abertelrud
force-pushed
the
eng/sandbox-improvements
branch
from
9ff90b0 to
b5d3eb5
Compare
|
@swift-ci please smoke test |
This is a PR of the part of #4307 that improves the Sandbox API without also changing how
TMPDIRis handled or whether writes to/tmpare allowed. That will be done as a separate PR that builds on top of this one.Motivation
Turn the stateless
Sandboxenum into aSandboxProfilestruct that's a bit more flexible:there is a list of path rules allowing a mixed order of
writable,readonly, andnoaccessrules; this addresses an issue where writable directories were always applied after readable directories, preventing arrangements where, for example, a source directory inside the temporary directory was properly made readonlythe defaults are built into the initializer rather than a separate
strictnessparameter — this means that they can be queried once the SandboxProfile has been created and are expressed in terms of the choices available to all sandbox profilesit's a bit more ergonomic (as a struct, it can be copied, mutated, and carried around in a property before being used), and an optional
SandboxProfilecan be used to indicate both whether or not to apply a profile and if so what to apply.Having the sandbox profile be a struct that generates the platform specifics as needed also could also provide a place with which to associate any cached/compiled representation for profiles that are largely static, for any platform that supports that.
Modifications
writable,readonly, andnoaccessrulesAs cleanup, this commit also removes the pre-SwiftPM 5.3 specialities which were specific to running the package manifest in an interpreter rather than compiling and executing it. This functionality is no longer relevant since it isn't possible to run any manifest in the interpreter, no matter what tools version the package specifies.
In this commit, the call sites have been adjusted so that they use the modified SandboxProfile API, but they still construct the profiles on-the-fly as before. A future change will make them instead be configured at an early point and then applied later when the sandboxed process is actually launched.
Note also that the existing call sites still pass
/tmpbecause that it was the sandbox has allowed up until now. A separate PR will deal with whether or not allowing writes to/tmpis appropriate.Another future change could add the sandbox profile to
Processas a property so that there is no API assumption that applying a sandbox necessarily involves just modifying the command line.Result
In this PR there is no change in semantics, apart from removing the now-irrelevant pre-5.3 affordances for running manifests in an interpreter. Changes that set
TMPDIRand block/tmpcan then easier be built on top of this.