-
-
Notifications
You must be signed in to change notification settings - Fork 677
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.
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
Enable App Sandbox on macOS for increased security #3149
Draft
kthchew
wants to merge
50
commits into
PrismLauncher:develop
Choose a base branch
from
kthchew:feat/macos-sandbox
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Also changes the way signing is done. A script is added to code each code item individually. Avoiding `--deep` is required to maintain the entitlements on Sparkle, otherwise they would be overwritten. It is also discouraged in general by Apple - see [`--deep` Considered Harmful](https://forums.developer.apple.com/forums/thread/129980). Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
…cess This issue appears to be a bug between a call to `close()` in `jspawnhelper` and the App Sandbox. This was reported to Apple as FB15992343 and OpenJDK as JDK-8345256. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 4, 2024 06:51
1a2c6ae
to
bc2bc34
Compare
…s on macOS This enables sandboxed apps to maintain access to user-selected items. In addition, for both sandboxed and nonsandboxed apps it can keep track of directories even if they are moved or renamed, and can remember access to directories in "sensitive" locations (such as the Documents folder or external drives). Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 4, 2024 07:26
bc2bc34
to
91aed41
Compare
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
…iginal file Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
…f file Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
This allows the Java downloader to work for Mojang Java installs in spite of the sandbox applying a quarantine. Also apply a sandbox exception to allow execution of Java. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Unlike the default sandbox quarantine, an executable file with a quarantine type that indicates that the file was downloaded is allowed to run. However, it is subject to Gatekeeper checks. Since Adoptium and Zulu are correctly signed and notarized, this isn't a problem and lets us offload the work to verifying their integrity to the OS. Some additional changes were made to how extraction is done to avoid invalidating the code signature. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 8, 2024 04:00
1bc2a91
to
0410f5a
Compare
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
2 times, most recently
from
December 8, 2024 06:46
62e537a
to
9b4a91e
Compare
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 8, 2024 07:09
9b4a91e
to
e62eb76
Compare
Also allow bookmark creation when migrating from an older version without bookmarks Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 9, 2024 06:48
a504b96
to
45b7439
Compare
For convenience, also add reset buttons for these paths Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 11, 2024 19:39
1daeccb
to
65e5c1d
Compare
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 13, 2024 02:21
a3688fd
to
515d005
Compare
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Note: Loading the steam libraries into the launcher itself seems to cause a crash, which is why the `allow-dyld-environment-variables` entitlement was removed. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 14, 2024 02:04
cb5364f
to
b0072aa
Compare
Now a pair of Unix socket file descriptors are made when opening an instance, and one of the descriptors is sent to the instance through an environment variable rather than using a path to a file. This gets around an issue where long usernames may break. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 14, 2024 02:11
b0072aa
to
ceb5356
Compare
Merge in the removal of hardened runtime exceptions, and the split of the ad-hoc and regular entitlements that came with it. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com> # Conflicts: # .github/workflows/build.yml # program_info/App.entitlements
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
This isn't correct, as the instance directory can be arbitrarily set and may lead to natives being written outside the home directory. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Link locations and the existence of directories are now explicitly checked, and most work is done in the enumerator where some details are prefetched. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Explicitly disallow applications and bundles, and skip files that already aren't quarantined. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Most libraries do not have an entrypoint (i.e. a `main` method that runs when they are executed directly) so this is not an issue. Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
Signed-off-by: Kenneth Chew <79120643+kthchew@users.noreply.github.com>
kthchew
force-pushed
the
feat/macos-sandbox
branch
from
December 26, 2024 23:03
4c46fa0
to
c2e468d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
If you want to test this, keep in mind the following:
~/Library/Containers/org.prismlauncher.PrismLauncher/Data/Library/Application Support/PrismLauncher
. To revert, manually move that folder back to~/Library/Application Support/PrismLauncher
.~/Library/Containers/org.prismlauncher.PrismLauncher
and~/Library/Application Scripts/org.prismlauncher.PrismLauncher
after testing. Be sure to move your data out before deleting these folders – see above. If you do not delete these folders and App Sandbox is ever added to Prism Launcher proper, the system will likely show security warnings every time you open the launcher because the identity of the app generated from a PR is different from its identity during a release, and the system cannot tell that they are the same.~/Library/Containers/org.prismlauncher.PrismLauncher/Data
). You'll likely encounter crashes if the Java directory is in the container due to what I believe to be macOS bugs that I'm hoping Apple will fix (FB16070451).Extends off the work in #1190 to enable App Sandbox to reduce potential damage by vulnerabilities in the launcher, game, and mods, as well as outright malicious mods.
On recent macOS versions, it also protects sensitive data in the Prism Launcher container (such as account tokens) from other running processes, even those with root privileges, using System Integrity Protection (unless user consent or Full Disk Access is given).
NOTE: This comment should not be taken to mean that Prism Launcher's data is protected from itself or mods. A malicious mod can still steal Minecraft account tokens, since it is within the sandbox, as is expected to be the case on any platform with a sandbox. This provides protection from other malware external to the game (e.g. a random script someone downloads).
tl;dr, does this work?
The game should work, and be sandboxed, as long as you
use default instance, etc. directories (or at least one within the sandbox container, so not on an external drive for example)you can now choose other directories :) it's possible there'll be problems with instances on other drives though, need to test/Library/Java/JavaVirtualMachines
), or another directory allowed by default by the sandbox, (edit: or by the launcher's java downloader)the launcher’s Java downloader won’t worknow works :)This isn’t fully ready yet for review/merging. See below for more known issues.
The Quarantine Issue™
Based on the discussion in #1190, we found that sandboxed apps quarantine all files they write to. This prevents a class of simple sandbox escapes. However, Minecraft (and some mods) extract native dynamic libraries to the filesystem at runtime, causing the system to deny them and show an error when they are loaded, and the game won't run. Getting around this requires a bit of a hack. When loading the library, the process calls the
dlopen
C function. By injecting a library into the game with theDYLD_INSERT_LIBRARIES
environment variable (similar toLD_PRELOAD
on Linux), we can use a technique called "dyld interposing" to replace anydlopen
function calls to call our own implementation. This is where we can remove the quarantine attribute.Actually removing the quarantine attribute must be done from a non-sandboxed process. (There'd be no point if the sandboxed process could just remove it itself.) Thus, a new non-sandboxed XPC service is added that removes quarantine on a provided file. There are a few checks and measures to attempt to stop malicious code from abusing it for a sandbox escape, though these might be expanded. Note the following principles I used to decide how to design these preventative checks in the XPC service (mostly found through trial and error rather than documentation...):
NSWorkspace.shared.open()
or using/usr/bin/open
) to open a file it has access to in a similar manner to if you double clicked it in Finder. Such file opens occur outside the sandbox (for example, opening a text file will open a text editor with the text editor's sandbox, or lack thereof. Same applies to a regular app. This has to be the case, otherwise these apps often wouldn't work.)chmod +x
). It also declares that it can open.dylib
files, whether they have executable permission or not.chmod +x
), it will run immediately. If it does not, it may still launch Terminal, but will not actually execute..tmp
and.jnilib
files.Essentially, for the file extensions I whitelisted, they are safe to dequarantine as long as you strip away their execute permission and don't allow them to have a customized "Open With" attribute (which leads to us having no idea the properties of whatever app this refers to). Libraries can still be loaded without the execute bit, so this is fine. Note that further attempts by the sandboxed process to modify the file, such as readding execute permission, will lead to the file becoming quarantined once again.
The Java child process unfortunately can't directly talk to the XPC service. The launcher thus uses a Unix socket to talk to the game. The transposed
dlopen
will send a message through the socket, triggering the launcher to ask the XPC service to unquarantine the library, before calling the actualdlopen
.The code is a bit of a mess right now since it's practically a proof-of-concept.
Note
Interposition isn't documented public API by Apple, so it's technically unsupported and only recommended for use in developer tools. The alternative methods have some issues though:
dlopen
to avoid interposition, but then user-installed "regular" JREs won't work.System.loadLibrary
etc.) but I'm not too familiar with such methods and they appear to be more complex. For example, it seems like you can't directly write to Unix sockets from Java.Known issues/TODO
Java downloader doesn't work (edit: fixed for Mojang Java in 653309d and others in 0410f5a)
Java can be downloaded, but it won't appear in the list afterwards (but will be present on disk). If you manually choose the downloaded Java, it won't run.
There are two problems here. One is the same as above - the quarantine applied on the file, but this time by the launcher. The Java executables won't be allowed to run as a result. The fix is, again, to offload this work to an unsandboxed XPC service (e.g. to lift the quarantine), but without any of the complicated interposing or sockets this time, since this is directly from the launcher itself.
The other problem is that for some reason, the sandbox denies the app from executing any executables located in the sandbox container directory, even without quarantine. (You can see in Console that a process-exec on Java is denied, even after you remove the quarantine.) I believe this is a macOS bug, as the documentation claims the app “can run programs located [in the container].” I filed a bug with Apple about this (FB15963761).
Note that the launcher seems to be able to run Java that is installed system-wide just fine.
Crash due to narrator when using Java downloader
There is an issue where the game crashes when attempting to load the narrator, for some reason. The error in the logs is of the form
This occurs when the Java installation used is contained in the sandbox directory. If that condition is true, it occurs regardless of whether or not the game is sandboxed. The current workarounds are to
~/Library/Containers/org.prismlauncher.PrismLauncher/Data
, which can be downloaded by the launcher's Java downloader, orFurther testing: This appears to be a macOS issue if the executable trying to use text-to-speech APIs is contained in a sandbox container on macOS Sequoia. The issue does not appear to affect macOS Sonoma. I made a minimally reproducible case and reported it to Apple (FB16070451).
Migrating data (default locations) (edit: added in 32c0d85)
Sandboxed apps have a different "home directory." Normally
~
is/Users/USER
, but now it's/Users/USER/Library/Containers/org.prismlauncher.PrismLauncher/Data
(at least this is the implementation detail in current macOS versions), so data must be migrated.This is fairly trivial for users who don't change their data directories from the default, as macOS lets you use a
container-migration.plist
to indicate what should be moved.Accommodating non-default data directories (edit: implemented in 503c702)
The situation is more complicated if someone customizes where data, such as instances, are stored. The container migration system described above is declarative and can't accommodate this situation (and arguably the launcher shouldn't move that data if the location isn't standard, anyway). macOS grants sandboxed apps access to directories if they select it in a native file picker.
Therefore, the current state is that if you go to Settings > Launcher and use the "Browse" buttons to choose some directory for instances, etc. outside of the sandbox container, Prism will be allowed to access that directory for that session. But if quit and reopened, access will be lost and you'll need to do this again. The solution to maintaining this access is to store security-scoped bookmarks rather than just a path, which will likely require some more native code (I'm not sure if Qt has anything to help with this).
Picking alternate directories will thus require the use of the file browser rather than allowing users to type in a path. From a simple test in Xcode, it looks like a non-sandboxed app can save security-scoped bookmarks in advance, and if it later adopts the App Sandbox, the bookmarks will work, allowing for a seamless update. It would require an intermediate update that saves these bookmarks rather than paths in the config, however. Otherwise, after an update that migrates to the sandbox, users would need to select the relevant locations in the file picker manually to grant access.
Sparkle updater (edit: fixed in e62eb76)
Some changes are needed as described at https://sparkle-project.org/documentation/sandboxing/.
Also, if a non-sandboxed version is built separately, the feeds for each version should likely be separated.
Ability to disable sandbox by instance (?) (edit: separated build in 670d69e)
It's described in #1146 that users should be able to disable the sandbox. This might be doable with an XPC service, though it's risky if you naively do it based on an instance setting, since a malicious mod in a sandboxed instance could just disable the sandbox (as the settings are contained inside the sandbox container). It might be easier and safer to have two versions of Prism Launcher, one sandboxed and one not.
User-defined allowed directories (?) (edit: added in 65e5c1d with a caveat)
#1146 also describes a feature where users can choose what directories are allowed. This could be possible by presenting a file picker and storing the security-scoped bookmarks as described above.
Edit: this is now added
unfortunately, while you can get read/write access to user-selected directories, you don't get permission to execute programs in those directories, so users still need to place their programs and scripts in "typical" locations (e.g.
/usr/local
,/opt/homebrew
, places indicated in the entitlements like~/bin
, etc.)Pre/post launch scripts and wrappers (?)
Not sure how this solution or the sandbox itself interacts with these or how people use them in the wild.
In any case, a wrapper command chosen by a user must make sure to pass down the
DYLD_INSERT_LIBRARIES
andXPC_MIDDLEMAN_SOCKET
environment variables to java.Steam overlay (edit: resolved in 88153e7 – probably)
Steam doesn't support the App Sandbox,
so the overlay is unlikely to ever work with it enabled.Possibly another argument for maintaining a non-sandboxed version. https://partner.steamgames.com/doc/store/application/platformsApparently it works if you allow the IPC, and the browser even works too – some of the names feel a bit "random" but I tested a VM and saw similar prefixes for the names, so I think the entitlements used should work generically.
Mods (mainly Discord RPC - supported in fb0d41d and 5e3fb20)
Most mods seem to work fine. Some won’t work by virtue of what the sandbox does (Discord RPC is an obvious example). We could see if there are entitlements that allow for this.
Long usernames may not work (edit: fixed in ceb5356)
There’s an added reliance on Unix sockets, but the max length of a Unix socket path on macOS is 104 bytes for historical reasons. (It’s around there on other operating systems that support Unix sockets, too.) The sandbox container temporary directory is quite long:
/Users/USER/Library/Containers/org.prismlauncher.PrismLauncher/Data/tmp/
. Right now the socket name isprism-launcher
, but if I shorten the socket name down to just one character, that leaves 34 bytes left for the name of the home folder (subtracting the null terminator in the C string) (edit: done in 3beeb10). I think most users’ usernames are less than 34 characters, though it could still conceivably be a problem as macOS seems to allow home folder names up to 63 characters long in the System Settings GUI.If that’s the case it might be better to use something like a local TCP socket.edit: swapped to using unnamed Unix sockets and passing the file descriptor to the game, which avoids this problem
Other odd things
Recent versions of Minecraft take a long time to load without allowing reads to
/dev/fd/
EDIT: This issue was fixed by OpenJDK and the fix should be available in future releases.
tl;dr Java 8 is OK. Java 17.0.10 or earlier is OK. Java 17.0.11 and later or any version of Java 21 will have long load times without this entitlement because of either a macOS or a Java bug.
The game won’t appear for a long time, but does load eventually (as in, after waiting for several minutes…) The
jspawnhelper
process freezes for a while, and if you continuously force quit it, Minecraft will start up much quicker, though that’s obviously not a good solution…Older versions of MC don’t show this behavior (e.g. 1.8.9, 1.12.2, 1.15.2, 1.16.5 seem to be fine). It seems to be related to Java 8 vs later Java versions. (1.16.5 is on Java 8 and uses LWJGL 3, and works. 1.17.1 is on Java 17, and has problems.)
I used a debugger on
jspawnhelper
and tracked it down to aclose
call it is attempting to do. From looking at the source, I tracked the likely cause of this freeze to the fix to https://bugs.openjdk.org/browse/JDK-8307990. Using versions of Java before that (e.g. 17.0.10) that resolve the error.Luckily, applying a read-only entitlement to
/dev/fd/
resolves the issue. I don’t think this adds much risk, although if this ends up being fixed by either Apple or OpenJDK, it should probably be removed.