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

Improve security for validating archives #2590

Closed
zorgiepoo opened this issue Jun 22, 2024 · 3 comments · Fixed by #2667
Closed

Improve security for validating archives #2590

zorgiepoo opened this issue Jun 22, 2024 · 3 comments · Fixed by #2667
Milestone

Comments

@zorgiepoo
Copy link
Member

zorgiepoo commented Jun 22, 2024

Sparkle extracts zip, tar, and dmg archives before validating the archive and validating the Apple code signing identity of the app contained inside the archives. This allows key rotation which lets a developer change their EdDSA signing key or their Apple Code Signing identity. Key rotation has been used by developers when losing keys or transferring ownership.

However there can be exploits in extracting archives themselves. We can increase security by always validating archives before extraction. Any updater that validates the downloaded archive ought to really do this (validating the archive provides better security than only validating the app contained inside it after extraction). We will still allow key rotation but only through code signed disk images.

This proposal means that we now check the Apple code signing signature of disk images, and only accept zip or tar archives that have been signed with Sparkle's (Ed)DSA keys. If the Apple code signing identity of the disk image does not match the original app to be updated, we will check its EdDSA signature. Key rotation will not be possible with zip or tar archives anymore.

To support validating the code signing identity of a disk image matches the host app, we will need to write a custom code signing requirement that tests the disk image is Apple issued, Developer ID signed, and has a matching Team ID as the host app. We will need to ignore the code signing identifier of the file.

For external (out-of-app) updaters, we may need to carve out a legacy path to continue allowing updates using the old policy for apps using older versions of Sparkle.

This also means that we will move towards recommending serving code signed disk images, rather than zip files, for updates. Historically disk image files have been hacky and slow to extract. However the next release (2.7) should make disk image extraction efficiency (with modern compression/formatting options) and fine-grained progress reporting on par with zip files. And it turns out there are cases where zip files are actually not well specified and less reliable to extract (e.g. #2544, #1691) -- not to mention they are worse for avoiding issues relating to App translocation which impact updating through Sparkle.

I think we will continue to keep a deprecated / log warning for apps that don't use EdDSA signing. There are several reasons for this: to support local development/testing which often may use a non-Developer ID certificate, support more efficient updates via delta updates, better safety/fallback in case the Apple signing certificate needs to change, avoiding code signing checks in the framework to determine update validity.

This is all agnostic to serving over HTTPS (our strongest line of defense still). Even today, a successful exploit would need to first compromise a developer's server.

Now I will need to test this to see if it can be done.

TLDR; better validation security in exchange for:

  • No longer allow updating zip or tar or non-signed disk image files without using (Ed)DSA signing (this has already been deprecated by Sparkle)
  • No longer allowing key rotation for tar and zip files, but will allow rotation for signed dmg files (in extreme cases where keys are lost or ownership transferred)
@kornelski
Copy link
Member

kornelski commented Jun 22, 2024

Security wise this is the right thing to do.

However, key management and signing are already painful and annoying. For users who have set up tarball based pipeline, switching to DMG may add to the frustration.

Can this be made easier somehow?

  • Maybe allow unverified decompression, but only for ZIP or tgz? It'd embarrassing if the platform couldn't make the oldest simplest decompressor safe.
  • Can decompressor be sandboxed itself?
  • Maybe alert users that something is fishy before allowing key rotation? (This may be a good idea regardless).
  • Maybe allow key rotation only after some quarantine period? (e.g the same update file must be served for 2 weeks). This could stop immediate damage on server takeover or transient network attacks (someone can hijack BGP for a few hours, but not consistently for weeks)

@zorgiepoo
Copy link
Member Author

zorgiepoo commented Jun 23, 2024

However, key management and signing are already painful and annoying. For users who have set up tarball based pipeline, switching to DMG may add to the frustration.

These users who are using tarballs or zip files need to switch if:

  • They need to use key rotation (a feature of last resort that hopefully doesn’t occur but is otherwise infrequent; e.g. losing Sparkle signing keys.)
  • They have not been using Sparkle based signing (which has been logged as deprecated since all of Sparkle 2, because we have not been able to validate the full integrity of the update by not validating the archive. So this should be a small number of developers that adopted Sparkle 2.)

Otherwise they can continue to use the tarball based pipeline.

I'm not familiar with the track record of vulnerabilities in libarchive, ditto/Bom.framework, or disk images but I don't like assuming they're inherently safe. As far as both tar and zip goes, I have seen compatibility issues with them being unable to extract, if not vulnerabilities; ZIP is not a simple format and both formats kind of kludge in support for Apple metadata. In one way I think they are less safe/reliable since they are not Apple-controlled formats. Granted, Sparkle is not the biggest target for these formats, but then again Sparkle can automatically/remotely update code. On that point, it would be a little bit odd to only assume non-code signed DMGs are unsafe while zip/tar.* are safe. If we assume these three formats are safe to decompress, then there's not much to change here since all other formats need to be validated before extracted. Extracting updates before validating the archives when we have validation in place for them is a bit silly, though.

I don't believe the installer/decompressor can be sandboxed feasibly (cannot use AppSandbox, not going to depend on deprecated sandbox_init).

A quarantine period is not feasible here and adds unwanted complexity. I don't see a good user experience here if the archive has to be downloaded first. The installer runs in a “separate” boundary from the framework and is also not in the business of tracking persistent state or trusting the client here.

Key rotation is not supposed to be a security issue from Sparkle’s point of view so I’m not sure about warning for it, but this could be a separate orthogonal issue. In some cases the OS handles this by alerting the user when an app that changes its code signing identity, depending on the functionality used. App sandboxing is one such feature that will definitely provide an alert. This can be tricky to convey.

Code signed / notarized disk images are Apple's preferred way of distributing applications. Distributing apps this way minimizes issues with app translocation (impacts updating) and better integrates with notarization/stapling. In reality, nowadays tarballs and zip files are not any significantly better than disk images for updates and worse in other ways (I plan to improve Sparkle's dmg support in the next release). Some developers have been distributing separate tarballs/zips for updates despite distributing disk images from their websites because of Sparkle, not because they want to (tarballs particularly are rarely distributed to users directly). From that use case, developers will be happy to serve a code signed disk image for both websites and updates.

If I want to make key management simpler while having security validation of archives and aligning to the platform's way of distributing apps, I will re-consider (un-deprecate) the path that allows a developer to distribute their app without using any of Sparkle’s signing keys, and instead by serving signed disk images. This could allow dropping Sparkle’s signing keys too. We have had one such request for this before. Then, for users that want to use serve significantly more efficient updates than zip/tar/dmg or want Sparkle key rotation*, they can use Sparkle's keys.

Appreciate the feedback here by the way. I will ponder on it.

* rotation would still be possible using different Developer ID certificates, assuming they are not revoked/lost

@zorgiepoo
Copy link
Member Author

zorgiepoo commented Nov 25, 2024

I've a change in #2667 that adds an opt-in key for enforcing to verify updates before extracting them to start off with.

  • Opt-in for now without warning so doesn't disrupt existing developers from migrating. Doesn't need to be enabled if developer is happy with current security model. The default behavior could be subject to change one day.
  • Many developers do not use disk images as their main download and may not want to switch for a fallback (if EdDSA keys are lost)
  • Developers still occasionally need to rotate Apple code signing (see Changing code signing identity #2596 for latest example) so we continue to strongly recommend using EdDSA signing for rotation
  • Safer to add disk image code signing verification as a fallback to EdDSA signing, so EdDSA is still required. Both in terms of fallback for developers and implementation where we assume some internals for the designated requirement.
  • Some developers that sign updates may not use developer ID signing specifically, which is what we check for (should be pretty small portion though)
  • Apple archives will require this new option since they were added just recently and are relatively obscure.
  • This could be a requirement if we support signed feeds / release notes one day..

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 a pull request may close this issue.

2 participants