Secure architecture for updating in a sandboxed environment

[kornelski]

🔥 Here's the new implementation that will be Sparkle 2.0: https://github.com/sparkle-project/Sparkle/tree/ui-separation-and-xpc 🔥 It implements the changes discussed in the issue below.

---

There has been a very old branch with a sandbox support, but Andy has rejected it due to unresolved security issues.

So I'm opening an issue here to start a discussion how we can support sandboxed applications in Sparkle while keeping the sandbox as tight as possible.

I think minimal requirements are:

• The XPC helper must not trust the host app at all, so it must verify integrity of downloaded update and unpack it by itself. The current approach of telling XPC helper "hey, install this path, I've unpacked it and checked it's ok" can't be secure.
• the XPC can't trust the host to supply a valid DSA key (a compromised host could give a valid and signed update, but signed by the wrong person). The XPC helper must find the trusted DSA key by itself (i.e. find the app's path and read Info.plist from there) and refuse to update any other path.
• Apple Code Signing verification code needs to be refactored (or not supported in sandboxed environment), because the host app checks against its own signature, but the host app is not supposed to be doing the checking.

[fernandomorgan]

Hi, updating my status - so, I do have a fork working since May or so, but I am still waiting the legal department ok to publish it.

I don't think it is in finished in terms of having completely addressing Andy's requirements BUT it does at least code signing validation in the XPC copy process.

I am mailing legal today - I do need to write a small doc about the changes, so I share the blame of the lateness - but I probably won't get an answer until tomorrow (California time), and then I will update this item.

I have distributed around 10 updates or so to around 1000 users, mostly in 10.9. There is a very small percentage in 10.8 and only test machines in 10.7. For a smaller group of 20-30 people I send around 5+ upgrades every day (our dev team, etc).

[SevenBits]

@fernandomorgan That's great! Look forward to hearing more on this!

[fernandomorgan]

Ok, here is the first release - I used a fork from your repo, Pornel

https://github.com/yahoo/Sparkle

[SevenBits]

@fernandomorgan Could you perhaps include documentation on how to set it up? Are any special steps required to get it to work with sandboxing?

[fernandomorgan]

actually you don't need to do anything. If your app isn't sandboxed, it uses the old copy. If the app is sandboxed, it will use a XPC helper - com.yahoo.Sparkle.SandboxService.xpc

I will add a note on distributing this helper

[jakepetroules]

@fernandomorgan Can you rebase on the latest master and send us a pull request please?

[fernandomorgan]

Somewhere, somehow, I guess some git info from the old Pornells fork was lost.

[kornelski]

I'll rebase it, don't worry

[fernandomorgan]

great! thanks

[kornelski]

The yahooxpc branch is ready for review.

Due to recent massive reorganization of the repository (sigh) it wasn't easy to merge and there are some messy merge conflicts/regressions, but at least we can start reviewing security of it and test it on top of the codebase.

[jakepetroules]

Are you sure you pushed? It looks like it's still 32 commits behind master.

[kornelski]

ok, 32 more

[jakepetroules]

OK, I've fixed this up enough that it actually builds. Some things might have been lost during your merging, Xcode complains of a missing implementation for a method in SUUpdater. Probably should compare diffs manually to look things over.

[kornelski]

Ah, yes. I've been brutally skipping over conflicting bits that didn't seem worth pulling out, e.g. log statements, 10.4 support.

One notable thing I've skipped was support for JSON appcasts. While JSON is fashionable, I don't think it solves enough problems, also xkcd-927.

[SevenBits]

I'd be happy to test things tomorrow; I should have some free time. I've been waiting for this feature for quite awhile so it is nice to see this actually happening.

On Jul 3, 2014, at 7:46 PM, Jake Petroules [email protected] wrote:

OK, I've fixed this up enough that it actually builds. Some things might have been lost during your merging, Xcode complains of a missing implementation for a method in SUUpdater. Probably should compare diffs manually to look things over.

—
Reply to this email directly or view it on GitHub.

[jakepetroules]

JSON does not have XML schema validation either. :)

[fernandomorgan]

Yeah. The thing with JSON is that it's pretty easy for the build process to generate a JSON file automatically. That's how I use it

The fork is 10.7+ only because of JSON. Originally used sbjson for 10.6+ later support but we dropped it. Currently only testing from 10.7 to 10.10 but we use both sandboxed and non-sb versions.

Sent from my iPad

On Jul 3, 2014, at 5:01 PM, Jake Petroules [email protected] wrote:

JSON does not have XML schema validation either. :)

—
Reply to this email directly or view it on GitHub.

[jtbandes]

Any updates on this review? Any way others can help?

[SevenBits]

I’ve been trying it out and will write a full report when I’m done. Others may certainly help.

On Jul 7, 2014, at 3:43 PM, Jacob Bandes-Storch [email protected] wrote:

Any updates on this review? Any way others can help?

—
Reply to this email directly or view it on GitHub.

[fernandomorgan]

Thanks! I am hoping to continue improving it too

On Monday, July 7, 2014 12:44 PM, SevenBits [email protected] wrote:

I’ve been trying it out and will write a full report when I’m done. Others may certainly help.

On Jul 7, 2014, at 3:43 PM, Jacob Bandes-Storch [email protected] wrote:

Any updates on this review? Any way others can help?

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub.

[SevenBits]

Just to confirm, is the XPC support present in the master branch of this repository and not just in Fernando's fork?

[fernandomorgan]

AFAIK, it's only in the yahooxpc branch/  https://github.com/yahoo/Sparkle )

it needs the XPC service - check the sandbox_service target)  then SUBasicUpdateDriver's 

• (void)installWithToolAndRelaunch:(BOOL)relaunch displayingUserInterface:(BOOL)showUI

has this to determine if we are in a sandbox environment or not 

   NSDictionary* environ = [[NSProcessInfoprocessInfo] environment];
    BOOL inSandbox = (nil != [environ objectForKey:@"APP_SANDBOX_CONTAINER_ID"]);
BOOLrunning10_7 = floor(NSAppKitVersionNumber) > NSAppKitVersionNumber10_6;
BOOL useXPC = running10_7 && inSandbox &&
    [[NSFileManagerdefaultManager] fileExistsAtPath: [[hostbundlePath] stringByAppendingPathComponent:@"Contents/XPCServices/com.yahoo.Sparkle.SandboxService.xpc"]];
    SULog(@"installWithToolAndRelaunch - using xpc=%d", useXPC);

  
... and in a sandbox, we us the XPC service instead

if( useXPC )
[SUXPC launchTaskWithLaunchPath: relaunchToolPath arguments:arguments];
else
        [NSTask launchedTaskWithLaunchPath: relaunchToolPath arguments: arguments];

the XPC service has to include the code signing verifier to, at a minimum (there's room to improve on this), to validate the signature before installing.

On Monday, July 7, 2014 1:00 PM, SevenBits [email protected] wrote:

Just to confirm, is the XPC support present in the master branch of this repository and not just in Fernando's fork?
—
Reply to this email directly or view it on GitHub.

[jakepetroules]

@fernandomorgan This is a better way of checking if you're sandboxed:

- (BOOL)isSandboxed
{
    BOOL sandboxed = NO;

    SecStaticCodeRef code = NULL;
    SecStaticCodeCreateWithPath((__bridge CFURLRef)[self bundleURL], kSecCSDefaultFlags, &code);

    SecRequirementRef requirement = NULL;
    SecRequirementCreateWithString(CFSTR("entitlement[\"com.apple.security.app-sandbox\"] exists"), kSecCSDefaultFlags, &requirement);

    if (code && requirement)
        sandboxed = (SecStaticCodeCheckValidity(code, kSecCSBasicValidateOnly, requirement) == errSecSuccess);
    if (requirement)
        CFRelease(requirement);
    if (code)
        CFRelease(code);

    return sandboxed;
}

@SevenBits It's in the yahooxpc branch for now, we've not yet merged it into master.

[grouchal]

I have been trying the yahooxpc branch and have tried to configure the Test App to work with the xpc code for an update. It seems the branch is not working at the moment for a few reasons:

• The sandbox_service target doesn't include the main.m file for the sandbox_service.
• The sandbox_service doesn't include the sparkle.framework
• The SANDBOX_SERVICE_NAME_STR seems to be empty
• The Test App doesn't have the sandbox_service included in the build phases and installed in the XPCServices folder
• The current code has the xpc check commented out and the check uses the empty SANDBOX_SERVICE_NAME_STR which causes it not to work.
• The update that is downloaded from the default service is not signed properly and causes update erros.

I have worked around all of these issues and can get the service to work, but think before the xpc changes are merged into the master these issues should be addressed to make it more obvious for others.

[SevenBits]

I found those same issues as well. They definitely need to be addressed.

On Jul 16, 2014, at 5:22 AM, Al Briggs [email protected] wrote:

I have been trying the yahooxpc branch and have tried to configure the Test App to work with the xpc code for an update. It seems the branch is not working at the moment for a few reasons:

The sandbox_service target doesn't include the main.m file for the sandbox_service.
The sandbox_service doesn't include the sparkle.framework
The SANDBOX_SERVICE_NAME_STR seems to be empty
The Test App doesn't have the sandbox_service included in the build phases and installed in the XPCServices folder
The current code has the xpc check commented out and the check uses the empty SANDBOX_SERVICE_NAME_STR which causes it not to work.
The update that is downloaded from the default service is not signed properly and causes update erros.
I have worked around all of these issues and can get the service to work, but think before the xpc changes are merged into the master these issues should be addressed to make it more obvious for others.

—
Reply to this email directly or view it on GitHub.

[jtbandes]

Testing this stuff out, it seems like the problem experienced by the current master branch, when the host is sandboxed, is that +[NSTask launchedTaskWithLaunchPath:arguments:] fails, raising the exception Couldn't posix_spawn: error 1 and logging the following to the console:

kernel[0]: exec of ~/Library/Containers/.../.Sparkle/Autoupdate.app/Contents/MacOS/Autoupdate denied since it was quarantined by ... and created without user consent, qtn-flags was 0x00000006`

However, it seems the same error occurs on this branch (only in the XPC service, since it's the one doing the +launch...). Is nobody else experiencing this? Setting up LSFileQuarantineExcludedPathPatterns seems like it would be the wrong solution, especially since the non-XPC version has the same issue.

[fernandomorgan]

There's some code specifically to remove the quarantine bit. Might be missing

Sent from my iPad

On Jul 17, 2014, at 5:21 PM, Jacob Bandes-Storch [email protected] wrote:

Testing this stuff out, it seems like the problem experienced by the current master branch, when the host is sandboxed, is that +[NSTask launchedTaskWithLaunchPath:arguments:] fails, raising the exception Couldn't posix_spawn: error 1 and logging the following to the console:

kernel[0]: exec of ~/Library/Containers/.../.Sparkle/Autoupdate.app/Contents/MacOS/Autoupdate denied since it was quarantined by ... and created without user consent, qtn-flags was 0x00000006`

However, it seems the same error occurs on this branch (only in the XPC service, since it's the one doing the +launch...). Is nobody else experiencing this? Setting up LSFileQuarantineExcludedPathPatterns seems like it would be the wrong solution, especially since the non-XPC version has the same issue.

—
Reply to this email directly or view it on GitHub.

[jtbandes]

It seems the removexattr call is failing; errno is set to EPERM. This kind of makes sense. But I'm not sure why it's working for anyone else.

[kornelski]

Yes, unification will be useful. Unifying EdDSA is a priority, so it'd be best to start with areas it touches.

[tonyarnold]

Oh, I was suggesting that we merge the changes piece-by-piece from this XPC separation branch back onto master. The goal should be for this to supersede Sparkle 1.x, right?

[kornelski]

If having XPC code on a branch called master is the goal, then no merging needs to be done, it's as easy as renaming the existing branches.

The issue is bigger than this, as it's about supporting the ecosystem.

Even if Sparkle 2.x becomes the Sparkle today, there will be apps that use Sparkle 1.x. We know that telling people to upgrade doesn't work, especially when it requires re-doing the integration. Many will just never upgrade.

So we'll need to keep around a Sparkle 1.x-compatible branch in working condition, for years, in case we need to release fixes or catch up with macOS APIs.

The more similar Sparkle 1.x and 2.x are, the easier it will be to keep them both updated (e.g. adding stuff like Dark Mode, or using new HTTP APIs, localization fixes, etc.).

[tkrajacic]

Maybe a naïve question but can't we hide the new XPC stuff behind new API and have both variants in the main branch at the same time?

Sure, it would break the workflow for the XPC variant if people use this branch now, but it's not the main branch and not really released, so I think it would be fine?

[tonyarnold]

The issue is bigger than this, as it's about supporting the ecosystem.

I absolutely understand that, but at a certain point it becomes untenable to do both - especially when it's due to users who won't upgrade even if you planted a bomb under their butts. Lump in limited time from contributors, and you have a v2 that's been living to one side for years.

I say this with nothing but respect and understanding of what you're trying to do here: Other people's laziness really shouldn't be your problem, @kornelski.

[michelf]

I could be mistaken, but I was under the impression that the XPC branch could be used without the XPC stuff in a non-sandboxed app, similar to Sparkle 1.x. Is there something else that would force developers to rework the way they embed the framework in a non-sandboxed app?

I too agree the current situation can't work for very long. Each accepted pull request to one or the other side adds more work to the pile of things to port to the other one. That can't be good.

[ksuther]

Even if Sparkle 1.x support doesn't end for a while, would it make sense to make the XPC branch master so that it is the new default? Right now Sparkle 2 is considered the secondary branch, and as long as it stays that way that's going to delay adoption of it.

[aonez]

I’m with @ksuther, making the XPC the default branch and keep maintaining the 1.x branch

[sweetppro]

why even bother maintaining the 1.x branch?
2.x supports macOS 10.9+ which was released in 2013!
10.15 comes out this year.
you should be looking to the future, not the past.

it seems like wasted resources supporting the 1.x branch.
Ive been using 2.x without issue for 2-3 years now...

why not just make a clean break?
do devs really even still need to support apps written for Mountain Lion

[digitalmoksha]

I've also been using the XPC branch for 3 years. I second putting 1.x into maintenance mode. Let's get the 2.x branch as the mainline development. Because it's really needed in order to keep moving forward with macOS. Critical fixes could continue to go into 1.x some time, but I don't think any new development/features should go into it.

[jakob]

PLEASE PLEASE PLEASE make the ui-separation-and-xpc the master branch. Or, if you don't want to continue supporting the old Sparkle for a long time, why not just move "ui-separation-and-xpc" to a new repo, for example. sparkle-project/Sparkle2?

Knowing that you have to check out a separate branch to get to the "good" Sparkle really doesn't help adoption of the new version...

[Iomegan]

Someone from the core team, please respond. 😊

[tkrajacic]

Sry to bump this, but is there any progress being made on the decision of how to proceed?

Is the core team even actively thinking about this - is there even a core team at that point?

I mean it has been months and no matter how thorough you are, the decision should have been made a long time ago. It feels just like nobody feels responsible for making it.

[Iomegan]

@kornelski It‘s not that there aren’t enough people to help. We just need a decision to be made. Drop 1.x support, it is not our responsibility if people don’t update.

[kornelski]

I don't have enough time to do the work required to make 2.x production quality.

If you want to help:

• backport changes from 1.x
• make PRs for the website to add proper documentation for 2.x. It's absolutely a blocker and won't be released unless the website has proper polished docs for 2.x integration.

[jakob]

If deprecation of 1.x is not not going to happen soon, what do you think about moving Sparkle 2 to a separate repo? Then at least the folks who use the new version would have a place to discuss the new version instead of all discussion being limited to this giant thread.

[digitalmoksha]

I'm not sure about opening another repo. I think that will make it even more unlikely that a fix on 1.x will get fixed on 2.x. We should be able open issues that begin with [2.x] or something. Or a template is created for use with 2.x issues and auto tag it.

I do think that there should be a real push that says if a fix is going into 1.x, it must also go into 2.x, or be shown that it's already fixed. Same with any enhancements - if it goes into 1.x, it's the submitters responsibility to get it onto 2.x as well.

We might also want a dedicated issue that talks about what is needed to get to 2.x, identify any PRs that haven't been ported over and open issues on them so the community can contribute. Same with the docs, break it into some smaller issues that is easier for someone to pick up and do.

[danmessing]

Is there a reason any new work is going into 1.x at this point? To me that seems like a waste of energy.

2.x is already being used in plenty of production code, so holding up its release based on getting it ready for that doesn't make a lot of sense to me. Similarily, great documentation would be awesome, but any documentation would be more useful to newcomers than having to dig through this thread.

Sorry to sound so negative, but the current state of affairs is frustrating for a lot of people. I very much appreciate the work everyone involved has put and continues to put into this project!

[spearway]

Could we start by something trivial rename the branch 2.x

[kornelski]

Sure, renaming the branch is fine. Adding bug templates for 2.x is fine.

The documentation is really important. It's absolutely unacceptable to switch the repo to 2.x and just keep website as-is with docs for 1.x. That would cause a lot of confusion and grief.

[spearway]

I agree with you we need to address the documentation, although as English is not my original language you don't want me anywhere near it.

At the memento we have a monolithic web site that does not reference any version how do you see that evolving? We need to put the plumbing in place and then put meat on the bones. The difficulty I can see is that most of us switched long ago and at least for me I have forgotten the pit falls…

[michelf]

It's worth mentioning there is already some sort of guide for the XPC branch by @DivineDominion:
https://christiantietze.de/posts/2019/06/sparkle-xpc-branch/
https://christiantietze.de/posts/2019/06/sparkle-xpc-or-no/
https://christiantietze.de/posts/2019/06/sparkle-xpc-setup/

[DivineDominion]

I'd be happy to help on the project docs as well! Does anyone have a vision or checklist I could work against? :) Like @danmessing, what would you have needed info on?

[kornelski]

#1523