Skip to content
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

Self patching Mac application #1528

Merged
merged 32 commits into from
Jul 28, 2024
Merged

Conversation

colincornaby
Copy link
Contributor

@colincornaby colincornaby commented Nov 19, 2023

This adds self patching to the Mac application. Servers will need to offer a Mac specific manifest, and encode the Mac client app with the bundle flag. The Mac client should be encoded in a tar file before it is gzipped. In the current implementation, the root of the inside of the bundle is the top level of the tar file.

The hash of the Mac client should be derived from the executable inside. (Note: this differs from the current implementation of Hoikas/UruManifest#12.) The executable should be code signed. Because the executable is code signed, it contains hashes as part of its loader flags that validate the entire bundle. This guarantees a unique executable for any version of the bundle - even if only resources have changed.

On macOS, CFBundle is used to decode the bundle and find the executable within the bundle. The name of the executable is encoded in the info.plist of the bundle, but non Apple platforms could either include Swift or CoreFoundation to decode the bundle, or make a reasonable guess. This could be useful for encoding manifests on non-Mac platforms.

Ventura has protection against apps being able to modify installed applications. I noticed this while implementing this feature, but am yet to test the bounds of this security or research further. It seems that macOS will permit us to install a new application on top of our existing application. But we may be prevented from upgrading any other bundle on the system. There may be ways around this using code signing.

This change also disables use of the Windows manifest files meaning after this change the Mac client will no longer work with Windows servers. A reasonable change could be an "emulate Windows" launch flag that causes the Mac client to go back to it's previous behavior of using the Windows manifests and filtering out executables.

Draft notes:

  • Getting this in front of people for early review.
  • Error handling likely needs to be improved.
  • Not all the patcher flags from Windows are implemented yet.

@dpogue
Copy link
Member

dpogue commented Nov 20, 2023

My first question, which is sort of a meta question: Is there a reason to do this in plClient rather than plUruLauncher, to match Windows?

On Windows, plUruLauncher handles the patching (and self-patching of just the launcher) and then launches plClient.

If we can do it all as a single binary, that seems better for end users, but I'm not sure what the original pros/cons were of having plUruLauncher be separate.

@colincornaby
Copy link
Contributor Author

I'm not sure. On the Mac it would be more complicated in since there are no start menu links - so we'd have to stash plClient someplace it wouldn't confuse the user.

I don't know that the launcher really simplifies things easier. Self patching is a pain point but the launcher has to self patch itself too.

@colincornaby
Copy link
Contributor Author

Here's a post talking about the new app update restrictions in Ventura:
https://lapcatsoftware.com/articles/AppManagement.html

It sounds like - as long as updates continue to be signed by the same signing organization - updates should work. But apps that are signed by one signing entity are not allowed to update apps signed by other entities. This could cause problems if an Uru server suddenly changed who was signing the application.

@colincornaby
Copy link
Contributor Author

Reading more - the only thing I'm concerned about is the insinuation that feature should only apply to applications that attempt to alter the bundle of another application - and not replacing the entire bundle. Odd.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

This isn't a complete review, just a few quick thoughts:

  • Because the bundle updating logic is macOS-specific anyway, could the archive handling be done using a macOS system library/framework/tool instead of pulling in our own copy of libarchive?
  • Using the main executable hash as a proxy for the entire bundle is a clever idea. It only works reliably if the app is code-signed though - are we sure that this will always be the case? I'm specifically thinking of testing shards like TrollLand and Minkata, where the shard operators don't pay Apple and so can't properly code-sign their client builds. Or can this be worked around by using an ad-hoc signature or something instead of completely unsigned builds?

Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
cmake/Findlibarchive.cmake Outdated Show resolved Hide resolved
@dpogue
Copy link
Member

dpogue commented Nov 20, 2023

  • Because the bundle updating logic is macOS-specific anyway, could the archive handling be done using a macOS system library/framework/tool instead of pulling in our own copy of libarchive?

No, because while macOS includes libarchive as a system library, it is not part of their public API and they don't provide headers for it and you're not allowed to link against it.

We talked initially about using the Apple-specific XAR archive format, but that posed issues for generating the bundles on Linux/Windows machines (where the servers are probably running).

Apple has a new AppleArchive format that they're wanting people to use, but it's only supported on macOS 11.0 and newer.

@dgelessus
Copy link
Contributor

I was thinking of the Apple-specific frameworks and not libarchive, but the Apple Archive framework is only supported since macOS 11 (and is apparently Swift-only) and the Compression framework only does single files and not archives. So none of those are an option.

I suppose you could call Archive Utility or the command-line tar tool to unpack the .tar.gz, but that's probably a lot more work and not worth it just to avoid the libarchive dependency.

@colincornaby
Copy link
Contributor Author

On signing specifically - unsigned applications won't run on macOS anymore without some serious tinkering. On Apple Silicon I believe it requires a firmware level override.

How everyone signs is still a complicated question with no good answer yet. But it's very likely everything distributed for macOS will need to be signed. Unfortunately - if not done right - that could cause some servers to not roll out a Mac client. If H'uru could control app builds and distribution for all shards (and we figure out signing) we could allow other shards to distribute our signed client.

@dgelessus
Copy link
Contributor

Hmm, that requirement is only true for native ARM code on Apple Silicon machines, I think. I have a couple of x86_64 applications that I can run on my M1 MBP even though they are (according to codesign -dv) "not signed at all".

In any case, I think ad-hoc signatures will be enough for testing purposes. They don't require any kind of code signing identity, so anybody can ad-hoc sign an app using the standard Apple dev tools. (I think this happens automatically for ARM code, which is how you can run self-compiled ARM binaries even if you never explicitly sign them.) Although ad-hoc signatures are only valid on the machine that created them, you can bypass that check with just right-click > Open on an ad-hoc signed app. I think ad-hoc signed binaries also contain all the resource hashes that a real signed binary would, so they should be enough to make the hash of the main binary representative for the entire bundle.

TLDR: if you ad-hoc-sign your client, everything should be fine.

@colincornaby
Copy link
Contributor Author

I don't know that I'd encourage ad hoc signing because of notarization - which is the other requirement looming over the Mac client. I don't think notarization works with ad hoc app.

Of course notarization can also be bypassed by lowering Gatekeeper settings and right click -> opening the app. But I don't know if that's a good experience. I'm also unsure of our support path for Intel binaries. Apple may stop producing tools and may stop producing Rosetta (at least for Cocoa apps) at some point in the future. So I weigh the experience with an Apple Silicon binary a lot more strongly than an Intel binary.

(How we handle dropping support for an architecture or OS revision may be it's own question with the self patcher.)

Regardless - distributing unsigned should be an edge case, and it would only be an issue if somehow a release was done without the executable changing at all. Which is possible, but an edge case within an edge case.

@dgelessus
Copy link
Contributor

To clarify, I was talking about ad-hoc signing as a better alternative than completely unsigned binaries, for cases where real signing isn't an option (mainly for people who don't pay Apple 100 $ a year). Of course, if you're able to, you should build a properly signed binary and notarize it. I just wanted to point out that if you don't have that ability, you can still produce a client that works with the bundle patching logic and can be run by normal people without too much hassle.

I don't think we need to worry about dropping support for x86_64 for now. The latest version of macOS still supports it, and as long as that's the case, the dev tools will be able to target it too, I assume.

Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

The executable should be code signed. Because the executable is code signed, it contains hashes as part of its loader flags that validate the entire bundle. This guarantees a unique executable for any version of the bundle - even if only resources have changed.

This is a good idea, I think, and it allows us to side-step problems with hashing the bundle. I think, however, we shouldn't require code signing. Instead, we could ensure uniqueness by forcing the build time to be embedded. This is currently an optional step on Windows, but it could be forced on Apple platforms. That way, we aren't requiring folks to pay the Apple tax.

This change also disables use of the Windows manifest files meaning after this change the Mac client will no longer work with Windows servers. A reasonable change could be an "emulate Windows" launch flag that causes the Mac client to go back to it's previous behavior of using the Windows manifests and filtering out executables.

This doesn't sound right. The macThinExternal manifest should contain not just the app bundle but also all of the core game files required to start the game. macExternal should contain the app bundle and all of the game files.

Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/pfPatcher.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/pfPatcher.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
cmake/Findlibarchive.cmake Outdated Show resolved Hide resolved
@dpogue
Copy link
Member

dpogue commented Nov 25, 2023

That way, we aren't requiring folks to pay the Apple tax.

FWIW, I'm pretty sure you can do local code signing with a free Apple Developer account, you only need to paid if you want to set up a team with multiple people having access to the signing certificates (and possibly also for notarization?)

That said, trying to do code signing on a build system where you're not logged in to the Xcode GUI is a pain, and I have spent literally 12 years of my life fighting that battle for CI builds of iOS apps at work.

@colincornaby
Copy link
Contributor Author

colincornaby commented Nov 25, 2023

That way, we aren't requiring folks to pay the Apple tax.

FWIW, I'm pretty sure you can do local code signing with a free Apple Developer account, you only need to paid if you want to set up a team with multiple people having access to the signing certificates (and possibly also for notarization?)

That said, trying to do code signing on a build system where you're not logged in to the Xcode GUI is a pain, and I have spent literally 12 years of my life fighting that battle for CI builds of iOS apps at work.

The free program won't let you sign or notarize - only run locally (or technically self sign for local runs.) The $99 individual program will let you sign, or the $299 group program.

At a PR conversation level - I still remain unconvinced that groups would be able to realistically avoid code signing. Code signing and notarization are both required on Apple Silicon. I think Intel only builds being grandfathered in is a loophole that will only work for a limited period of time. Rosetta will eventually go away, our ability to build for Intel will go away over the long term, and I wouldn't really want to encourage Intel only builds.

By the same token - developer/self sign/adhoc builds cannot be distributed to Apple Silicon users because they cannot be notarized.

That way, we aren't requiring folks to pay the Apple tax.

I don't think we're doing anything. Unfortunately it's Apple requiring people to pay the Apple tax. Whatever our feeling are, Apple at an operating system level is increasingly forcing the matter.

We can't give away the signing keys - but one service we could provide is distribution of H'uru signed clients.

@Hoikas
Copy link
Member

Hoikas commented Nov 25, 2023

Apple will do as Apple will do. I remain unconvinced that we should acquiesce to their money and/or time grab. I understand that Mac on ARM will require paying Bill GatesTim Cook, but I don't see why we should simply capitulate to that requirement when there is a suitable solution for this problem in the code already in the repo. Not to say we shouldn't encourage signed builds nor should we not provide signed builds (I think it would be great if we did), I think that providing something that works easily and inexpensively out of the box is important. Intel Macs aren't going away any time soon - I know that Apple likes for people to throw their computer away every 4 years or so, but, realistically, anything mid-range or better from the last decade is still perfectly serviceable (or better) hardware. Just because Apple will stop caring doesn't mean we should, IMO.

@dpogue
Copy link
Member

dpogue commented Nov 25, 2023

The free program won't let you sign or notarize - only run locally (or technically self sign for local runs.)

Ahh right, and I guess when we're talking about patching it's unlikely being are doing local builds. So builds meant to be distributed via the patcher would always need to be signed/notarized, yeah.

@dgelessus
Copy link
Contributor

By the same token - developer/self sign/adhoc builds cannot be distributed to Apple Silicon users because they cannot be notarized.

Notarization isn't specific to ARM. It's been "required" since macOS 10.15, but it's relatively easy for the user to bypass that. AFAIK, this behavior is always the same, regardless of the architecture of the binary and the host.

An unnotarized ad-hoc signed build definitely can be distributed, even if it's native ARM running on Apple Silicon. AFAIK, the only hurdle is that the end user needs a couple of extra clicks the first time they run the app (the user does not need to run scary commands or disable important system security features). As mentioned above, I think that's acceptable for testing purposes.

Because ad-hoc signing is done with the standard dev tools and doesn't require a code signing certificate or other special permission, it should be okay to assume that anybody can do ad-hoc signing, so we shouldn't need other workarounds like force-embedding the build timestamp. (Please also keep in mind that not every client build will go onto a file server. 🙂)

@colincornaby
Copy link
Contributor Author

Apple will do as Apple will do. I remain unconvinced that we should acquiesce to their money and/or time grab. I understand that Mac on ARM will require paying Bill GatesTim Cook, but I don't see why we should simply capitulate to that requirement when there is a suitable solution for this problem in the code already in the repo. Not to say we shouldn't encourage signed builds nor should we not provide signed builds (I think it would be great if we did), I think that providing something that works easily and inexpensively out of the box is important. Intel Macs aren't going away any time soon - I know that Apple likes for people to throw their computer away every 4 years or so, but, realistically, anything mid-range or better from the last decade is still perfectly serviceable (or better) hardware. Just because Apple will stop caring doesn't mean we should, IMO.

I think I'd like to see this in a place where solutions are more productive for working with the ecosystem that exists. Providing signed H'uru builds for other servers doesn't seem infeasible. UruManifest already pulls our GitHub builds - it wouldn't be a stretch for it to pull signed releases that get sent downstream to other shards.

Intel support is going to be beyond our control. Eventually Apple will stop supplying Intel SDKs, we won't be able to run the old ones on current hardware/operating systems, and the old SDKs will drop off of Github Actions. It may be some time (probably 3-4 years from now?) But it's going to be out of our control. Same thing happened with PowerPC. You couldn't actually build for PowerPC anymore unless you kept specific hardware/software configs around to do it. It's not a stance on the longevity of old hardware. It's just me assuming that there will come a day when we cannot build against the Intel SDK. Apple does not support building against old SDKs long term like Microsoft does and actively blocks it in their software.

To be frank - there is already considerable overhead in our support range. I have an Nvidia Macbook Pro that has it's GPU power management controller wedged into place with a rubber pad because it's become partially desoldered - and it's my only Nvidia Mac. Uru is an older game, and older hardware may be capable. But the actual overhead of supporting older Macs in development is and will be a problem. I have to maintain the Macs - and I have to have an OS install with the matching SDK for each OS version we support (which the Metal debugger gets really picky about.) And because Apple doesn't distribute free standing GPU driver updates, when we support a macOS version, we support all the GPU driver bugs on that macOS version. Which has been a particular problem with the Nvidia and Intel GPU drivers.

I guess what I'm saying is I agree in principle supporting older hardware is great - but the technical overhead of doing that on macOS is very different than Windows.

@colincornaby
Copy link
Contributor Author

colincornaby commented Nov 25, 2023

By the same token - developer/self sign/adhoc builds cannot be distributed to Apple Silicon users because they cannot be notarized.

Notarization isn't specific to ARM. It's been "required" since macOS 10.15, but it's relatively easy for the user to bypass that. AFAIK, this behavior is always the same, regardless of the architecture of the binary and the host.

An unnotarized ad-hoc signed build definitely can be distributed, even if it's native ARM running on Apple Silicon. AFAIK, the only hurdle is that the end user needs a couple of extra clicks the first time they run the app (the user does not need to run scary commands or disable important system security features). As mentioned above, I think that's acceptable for testing purposes.

Because ad-hoc signing is done with the standard dev tools and doesn't require a code signing certificate or other special permission, it should be okay to assume that anybody can do ad-hoc signing, so we shouldn't need other workarounds like force-embedding the build timestamp. (Please also keep in mind that not every client build will go onto a file server. 🙂)

There is one angle to this I probably still need to do more digging on, but it could be relevant here.

Ventura added the requirement that when an application modifies another application - the code signing team needs to match. In testing, it did not seem like my developer signed build trying to do any sort of self patching. It's not clear what an unsigned build would do or if an unsigned build would be allowed to self patch (my guess is no - but thats just a guess.)

I need to contact Apple DTS to get more clarity on the restrictions. My big concern has been if a server changed its code signing - self patching may begin to fail. But it might cause unsigned applications to not be able to patch at all.

(The app modification policy can be changed in security - but it's another toggle for the user to go through... And the OS will just keep blocking self patching quietly until the user toggles that setting. The user will only be notified on the first attempt.)

@dgelessus
Copy link
Contributor

Ventura added the requirement that when an application modifies another application - the code signing team needs to match.

I read about this a few days ago, but unfortunately can't find the link anymore... but if I remember correctly, this restriction is only relevant if you modify individual files in an existing app bundle. If you replace the entire bundle with another one (which your patching code does here I think), then the teams don't need to match.

@Deledrius
Copy link
Member

That makes sense from an internal-consistency point of view.

@colincornaby
Copy link
Contributor Author

I read about this a few days ago, but unfortunately can't find the link anymore... but if I remember correctly, this restriction is only relevant if you modify individual files in an existing app bundle. If you replace the entire bundle with another one (which your patching code does here I think), then the teams don't need to match.

That is what I thought as well, but it's getting triggered on full bundle replacements that do not access the internals of the bundles, or do partial replacements. This PR triggers it even though it does a swap at a file system level.

The documentation is kind of ambiguous - but the WWDC sessions specifically mentions self update systems. Sparkle also had a thread on this issue where they determined that they should be ok because they inherit the parent apps signing. It's one reason I want to follow up with Apple. Most the security oriented documentation seems concerned about replacement of bundle internals. But there is some documentation out there implying this also affects self update systems.

@colincornaby
Copy link
Contributor Author

I've reached out to Apple to inquire why we're seeing the App Management security feature block self updating in unsigned situations (where at least one bundle is unsigned) and if that is intended behavior. It may be a bit, I'll update when I hear back.

@colincornaby
Copy link
Contributor Author

I'm going to resume working on this. I haven't seen any response from Apple yet - and I still need to investigate what is going on with self patching when signing changes on one of my Macs (prompting an application modification block.) But I've built other standalone samples that don't seem to cause blocking issues.

@colincornaby
Copy link
Contributor Author

I've resolved the questions about the App Modification security block - and have moved the Mac bundle code into the Mac client itself. I'm going to promote this to a full PR.

Note that UruManifest behavior still does not match the current implementation. I'm testing with a hand modified manifest - I'll need to update UruManifest separately.

Deleting entire temporary directory. Also removing redundant unlink - it was causing problems because the unlinked app was getting swapped into the temporary directory.
Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

  • PLSPatcher.mm looks like it needs to have clang-format run on it.
  • There are changes to PLSLoginWindowController.xib that appear to be unrelated to the goal of self patching.

cmake/Findlibarchive.cmake Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/CoreLib/plFileSystem.cpp Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/CoreLib/plFileSystem.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
Sources/Plasma/FeatureLib/pfPatcher/plManifests.cpp Outdated Show resolved Hide resolved
cmake/Findlibarchive.cmake Outdated Show resolved Hide resolved
@colincornaby colincornaby force-pushed the mac-self-patching branch 3 times, most recently from 3534291 to c5071bf Compare July 9, 2024 04:06
@colincornaby
Copy link
Contributor Author

Changes are made and tested against local Docker server.

Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/main.mm Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/main.mm Outdated Show resolved Hide resolved
Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

Looks like just two more minor things.

Sources/Plasma/Apps/plClient/CMakeLists.txt Outdated Show resolved Hide resolved
Sources/Plasma/Apps/plClient/Mac-Cocoa/PLSPatcher.mm Outdated Show resolved Hide resolved
@colincornaby
Copy link
Contributor Author

@dpogue - Would like to see a final ok from you before merging.

@colincornaby
Copy link
Contributor Author

@dpogue - Checking in again if you have any input on this PR. No rush - I just don't want to merge if you still have any outstanding concerns.

Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

This looks good to me

@colincornaby colincornaby merged commit 2a60d41 into H-uru:master Jul 28, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants