Homebrew / homebrew-cask

🍻 A CLI workflow for the administration of macOS applications distributed as binaries
https://brew.sh
BSD 2-Clause "Simplified" License
20.93k stars 10.72k forks source link

Scraping shasum-verification policy #48862

Closed vitorgalvao closed 6 years ago

vitorgalvao commented 6 years ago

So far, I’d call the policy of requiring verification for sha256-only changes a monumental failure. It places a burden on maintainers and contributors that beared no fruit. Everyone is annoyed by it and no one wants to do it.

It’s also easily circumventable, as an attacker that replaces a binary needs only change its version on the download page. Version bumps not initiated by a developer might be easy to catch in an open-source world, but with the tools we deal with a simple change to the HTML of the download page could go unnoticed indefinitely.

By now it looks little less than a rushed policy taken out of proportion because of a single occurrence. It’s not like shasums are a meaningful way of providing security, anyway. At best they provide a check for verifying a correct download.

So how do we fix this?

Codesigning by itself doesn’t save a user from being infected (see Transmission’s case in 2016), but it can prevent the infection from spreading too far (same case, after Apple revoked the certificate). It’s also currently our preferred method of verification — we ask users to upload the file to VirusTotal but what we care about is the codesigning verification it provides. While not a perfect method, it’s the best we have.

It’s impossible for us to know when an app was tampered with and we need to set expectations accordingly. Just like we have --require-sha for security-minded users, we also need to make it their decision the risk of accepting or refusing non-codesingned apps.

macOS already provides an acceptable way of warning users the application they downloaded may not be trustworthy, by means of quarantining. The problem with that method is quarantining needs to be set by the downloading app. Since curl doesn’t do it, no apps we download are quarantined and users never get a warning.

Quarantining is a desirable feature, but unfortunately Apple does not provide a simple way to set a download as quarantined. @reitermarkus started work on this, though I’m unsure it it’s already usable.

So my proposal is to scrape the policy and finish what needs to be done to allow us to quarantine downloads (ideally releasing it as a stand-alone CLI tool that other projects can benefit from as well). We may also add a --no-quaratine option for users that don’t care

Pinging @Homebrew/cask, but everyone is welcome to comment.

amyspark commented 6 years ago

In re quarantining - I believe implementing it in Ruby may be too prone to breakage (manually handling the SQLite database and the extended attribute led me to a broken policy db when I dealt with @reitermarkus's patch).

If we can add a native extension to handle this from within Objective-C++, it'd be a boon. For reference, these lines in Transmission do the trick.

commitay commented 6 years ago

Agreed with everything in the top post.

ideally releasing it as a stand-alone CLI tool that other projects can benefit from as well

How do you see this being integrated into Homebrew?

implementing it in Ruby may be too prone to breakage

If we can add a native extension to handle this from within Objective-C++ ...

Understandably, there had been objections to to adding non-ruby code to Homebrew for a feature previously but personally I think the benefit of this would justify non-ruby code it was needed.

vitorgalvao commented 6 years ago

ideally releasing it as a stand-alone CLI tool that other projects can benefit from as well

How do you see this being integrated into Homebrew?

I don’t mean releasing exclusively as a stand-alone CLI tool. I’m perfectly fine with it being integrated directly into HB and also having a stand-alone version.

In case direct integration isn’t possible (due to ruby breakage) we can just stick the binary somewhere in the tap, or make a formula for it that’ll be installed at the same time HBC is tapped.

amyspark commented 6 years ago

In case direct integration isn’t possible (due to ruby breakage) we can just stick the binary somewhere in the tap, or make a formula for it that’ll be installed at the same time HBC is tapped.

I guess some people may not be comfortable with bottled binaries being automatically downloaded. Personally, I'd like the second option, and that the Formula is compiled in the user's folder, for the sake of transparency with the source code.

brianbrownton commented 6 years ago

I would support using native macOS quarantining, through whichever workflow works for the community.

My alternative suggestions which I have mentioned elsewhere, are: • Let there be a window of time after submissions for the checksum to be updated without requiring additional work • Let the person who subbed the PR do the checksum update without requiring additional work (the logic being if they were trusted initially they are trusted again).

vitorgalvao commented 6 years ago

logic being if they were trusted initially they are trusted again

I don’t expect the person that breaks the server to be the same that submits to HBC. I see it as more likely they would be duped just like any other user. That’d also be extremely easy to circumvent if you’re the malicious user: find the breach, submit the cask update, replace the app, update the cask again.

Let there be a window of time after submissions for the checksum to be updated without requiring additional work

That seems more reasonable if we know the first update has been made right after it was updated upstream.

I would support using native macOS quarantining, through whichever workflow works for the community.

Trouble at this point is we have no working solution for quarantining. Maybe we need a Cocoa developer to shed some light on it?

brianbrownton commented 6 years ago

That’d also be extremely easy to circumvent if you’re the malicious user: find the breach, submit the cask update, replace the app, update the cask again.

My thinking is that this leaves no more of a security hole than already exists - if a cask is breached, you can just release an update and bump the version, sub a PR.

Edit: maybe only trust github users of a certain pedigree, as malicious users would likely use a throwaway account (or perhaps another compromised account...) This is the problem with security - at some point it always breaks down ;)

amyspark commented 6 years ago

Trouble at this point is we have no working solution for quarantining. Maybe we need a Cocoa developer to shed some light on it?

I'm definitely not a Cocoa dev, but I think I may be able to do the job.

First approach is to implement quarantining using a native library. How do we use it?

  1. Formula + binary. Upon tapping Cask, install a binary, say, quarantine, that when fed a path it quarantines the file with the given metadata. If we need extra fields, this one is not exactly extensible.
  2. Gem + native extension. Same as above, but this one would use FFI to call directly the quarantine function. This one allows us to expose all the available fields, and if one of them is Nil, the library ignores it. Do notice that the function signature exposed by the library should hide the Objective-C stuff!

In any case, the challenge I found is the Makefile for the library. I have never seen one which uses Objective-C or MacOS frameworks.

Second approach could be a Swift script. This one would be far less of a burden in terms of compilation, but I'm not sure if it's available in all MacOS versions that Cask covers.

If you think it would be useful, I have a demo of the binary I spoke about in (1), in a Xcode project. Ping me and I'll upload it to a repo.

vitorgalvao commented 6 years ago

Second approach could be a Swift script. This one would be far less of a burden in terms of compilation, but I'm not sure if it's available in all MacOS versions that Cask covers.

From Mojave on that should not be an issue. Since we officially only support the latest two macOS versions, I’d be fine with that solution as it’ll work better as more time passes.

  1. (…) If we need extra fields, this one is not exactly extensible.

Which fields are we talking about?

amyspark commented 6 years ago

Available metadata fields are listed in the LSQuarantine.h header. Just setting AgentName(= Homebrew-Cask) and TypeKey (= OtherDownload) does the job, though.

vitorgalvao commented 6 years ago

It’d be useful to be able to set at least the AgentName, but if it’s not possible for now, that’s fine.

amyspark commented 6 years ago

I've uploaded a gist here to show how we could implement quarantining via a Swift script. Parameter input can be certainly improved (environment variables?), but I feel it's a reasonable start.

EDIT: Oh, before I forget -- remember once a cask is quarantined, it will only run if it is signed!

vitorgalvao commented 6 years ago

remember once a cask is quarantined, it will only run if it is signed!

Unless we right-click and select “Open”, which I think is acceptable. It makes the user say “yes, I understand the risk”.

I haven’t yet had the chance to try the Swift script. Has anyone?

amyspark commented 6 years ago

Unless we right-click and select “Open”, which I think is acceptable. It makes the user say “yes, I understand the risk”.

This will work provided you set Gatekeeper to "open apps from Anyone", which defeats its purpose. If the app isn't signed, why quarantining it at all?

What I mean is:

vitorgalvao commented 6 years ago

Unless we right-click and select “Open”, which I think is acceptable. It makes the user say “yes, I understand the risk”.

This will work provided you set Gatekeeper to "open apps from Anyone", which defeats its purpose.

That is incorrect. If you set it to only allow opening apps from the app store and identified developers, you can still bypass Gatekeeper by right-clicking and selecting “Open”. It makes perfect sense, as macOS understands you’re making a deliberate decision. This was last tested on 10.12, but I see no reason they would have changed it.

There’s no need for extra stanzas and conditionals. We can quarantine everything and let users deal with it with this method officially sanctioned by Apple. Gif of it in action in a VM: gatekeeper

amyspark commented 6 years ago

If anyone desires to test quarantining, I've just made a Homebrew branch with this feature: https://github.com/amyspark/brew/tree/com-apple-quarantine.

commitay commented 6 years ago

Does it need to have any of the quarantine types apart from WEB_DOWNLOAD?

amyspark commented 6 years ago

Not at all, I can clean it up. I just used the generic version I posted in the gist.

commitay commented 6 years ago

Can you squash it down to one commit please? It's easier to test across multiple installs/VMs with brew pull.

amyspark commented 6 years ago

@commitay, done.

commitay commented 6 years ago

Thanks!

brianbrownton commented 6 years ago

Any status updates on this?

vitorgalvao commented 6 years ago

@brianmorton It’s going along well and hopefully we’ll be able to soon scrape the policy in favour of quarantining. See https://github.com/Homebrew/brew/pull/4656.

commitay commented 6 years ago

hopefully we’ll be able to soon scrape the policy in favour of quarantining

We won't be able to drop the current policy outright as quarantining doesn't help with pkg, installer script, etc.

vitorgalvao commented 6 years ago

We won't be able to drop the current policy outright as quarantining doesn't help with pkg, installer script, etc.

I thought pkgs got quarantined as well.

amyspark commented 6 years ago

Everything that passes through Hbc::Download.perform and Hbc::Installer.extract_primary_container is quarantined.

The first one affects all downloads, the second one containers whose extract_to_dir process may not have preserved the quarantine attribute (if the unpacking used anything other than FileUtils.mv or /usr/bin/cp -pR, it probably didn't).

As pkg's install_phase does not unpack in staged_path, quarantine preservation is left to /usr/sbin/installer. Its man page doesn't say anything about it.

vitorgalvao commented 6 years ago

Closing in favour of https://github.com/Homebrew/homebrew-cask/issues/51491.