From ab31af2b4b0c43bf62bbe8720096b7d3861bf4d9 Mon Sep 17 00:00:00 2001 From: "L. E. Segovia" <13498015+amyspark@users.noreply.github.com> Date: Fri, 7 Sep 2018 15:37:31 +0000 Subject: [PATCH 1/4] Cask: fixes for quarantining Gatekeeper's Path Randomization is currently making automated installation a nightmare. Let's manually toggle the (undocumented) app translocation bit in the `com.apple.quarantine` extended attribute. While we're at it, let's also toss in some fixes: - zip downloads with improper permissions that prevent us from quarantining - quarantine/release/skip downloads as requested by the user --- Library/Homebrew/cask/download.rb | 7 +- Library/Homebrew/cask/exceptions.rb | 14 ++++ Library/Homebrew/cask/quarantine.rb | 64 ++++++++++++++----- Library/Homebrew/test/cask/cmd/fetch_spec.rb | 4 +- .../test/cask/{cmd => }/quarantine_spec.rb | 48 ++++++++++---- 5 files changed, 104 insertions(+), 33 deletions(-) rename Library/Homebrew/test/cask/{cmd => }/quarantine_spec.rb (85%) diff --git a/Library/Homebrew/cask/download.rb b/Library/Homebrew/cask/download.rb index 5b7a63d9bc..8bb97466f6 100644 --- a/Library/Homebrew/cask/download.rb +++ b/Library/Homebrew/cask/download.rb @@ -7,7 +7,7 @@ module Cask class Download attr_reader :cask - def initialize(cask, force: false, quarantine: true) + def initialize(cask, force: false, quarantine: nil) @cask = cask @force = force @quarantine = quarantine @@ -46,11 +46,10 @@ module Cask end def quarantine - return unless @quarantine + return if @quarantine.nil? return unless Quarantine.available? - return if Quarantine.detect(@downloaded_path) - Quarantine.cask(cask: @cask, download_path: @downloaded_path) + Quarantine.cask!(cask: @cask, download_path: @downloaded_path, action: @quarantine) end end end diff --git a/Library/Homebrew/cask/exceptions.rb b/Library/Homebrew/cask/exceptions.rb index b9389374cc..939e9e1292 100644 --- a/Library/Homebrew/cask/exceptions.rb +++ b/Library/Homebrew/cask/exceptions.rb @@ -184,4 +184,18 @@ module Cask s end end + + class CaskQuarantineReleaseError < CaskQuarantineError + def to_s + s = "Failed to release #{path} from quarantine." + + unless reason.empty? + s << " Here's the reason:\n" + s << Formatter.error(reason) + s << "\n" unless reason.end_with?("\n") + end + + s + end + end end diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index ead40af1ec..80749cd15c 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -55,26 +55,58 @@ module Cask print_stderr: false).stdout.rstrip end - def cask(cask: nil, download_path: nil) + def disable_translocation!(xattr) + fields = xattr.split(";") + + # Fields: status, epoch, download agent, event ID + # Let's toggle the app translocation bit, bit 8 + # http://openradar.me/radar?id=5022734169931776 + + fields[0] = (fields[0].to_i(16) | 0x0100).to_s(16).rjust(4, "0") + + fields.join(";") + end + + def cask!(cask: nil, download_path: nil, action: true) return if cask.nil? || download_path.nil? - odebug "Quarantining #{download_path}" + if action + return if detect(download_path) - quarantiner = system_command(swift, - args: [ - QUARANTINE_SCRIPT, - download_path, - cask.url.to_s, - cask.homepage.to_s, - ]) + odebug "Quarantining #{download_path}" - return if quarantiner.success? + quarantiner = system_command(swift, + args: [ + QUARANTINE_SCRIPT, + download_path, + cask.url.to_s, + cask.homepage.to_s, + ]) - case quarantiner.exit_status - when 2 - raise CaskQuarantineError.new(download_path, "Insufficient parameters") + return if quarantiner.success? + + case quarantiner.exit_status + when 2 + raise CaskQuarantineError.new(download_path, "Insufficient parameters") + else + raise CaskQuarantineError.new(download_path, quarantiner.stderr) + end else - raise CaskQuarantineError.new(download_path, quarantiner.stderr) + return unless detect(download_path) + + odebug "Releasing #{download_path} from quarantine" + + quarantiner = system_command("/usr/bin/xattr", + args: [ + "-d", + QUARANTINE_ATTRIBUTE, + download_path, + ], + print_stderr: false) + + return if quarantiner.success? + + raise CaskQuarantineReleaseError.new(download_path, quarantiner.stderr) end end @@ -85,10 +117,12 @@ module Cask odebug "Propagating quarantine from #{from} to #{to}" - quarantine_status = status(from) + quarantine_status = disable_translocation!(status(from)) resolved_paths = Pathname.glob(to/"**/*", File::FNM_DOTMATCH) + FileUtils.chmod "u+w", resolved_paths + quarantiner = system_command("/usr/bin/xargs", args: [ "-0", diff --git a/Library/Homebrew/test/cask/cmd/fetch_spec.rb b/Library/Homebrew/test/cask/cmd/fetch_spec.rb index 449de9d8ba..153f5bb42a 100644 --- a/Library/Homebrew/test/cask/cmd/fetch_spec.rb +++ b/Library/Homebrew/test/cask/cmd/fetch_spec.rb @@ -37,7 +37,7 @@ describe Cask::Cmd::Fetch, :cask do old_ctime = File.stat(cached_location).ctime - described_class.run("local-transmission") + described_class.run("local-transmission", "--no-quarantine") new_ctime = File.stat(cached_location).ctime expect(old_ctime.to_i).to eq(new_ctime.to_i) @@ -49,7 +49,7 @@ describe Cask::Cmd::Fetch, :cask do old_ctime = File.stat(cached_location).ctime sleep(1) - described_class.run("local-transmission", "--force") + described_class.run("local-transmission", "--force", "--no-quarantine") new_ctime = File.stat(cached_location).ctime expect(new_ctime.to_i).to be > old_ctime.to_i diff --git a/Library/Homebrew/test/cask/cmd/quarantine_spec.rb b/Library/Homebrew/test/cask/quarantine_spec.rb similarity index 85% rename from Library/Homebrew/test/cask/cmd/quarantine_spec.rb rename to Library/Homebrew/test/cask/quarantine_spec.rb index 87b3bcdffc..f4f937e26d 100644 --- a/Library/Homebrew/test/cask/cmd/quarantine_spec.rb +++ b/Library/Homebrew/test/cask/quarantine_spec.rb @@ -23,7 +23,7 @@ describe Cask::Quarantine, :cask do it "quarantines Cask fetches" do Cask::Cmd::Fetch.run("local-transmission") local_transmission = Cask::CaskLoader.load(cask_path("local-transmission")) - cached_location = Cask::Download.new(local_transmission, force: false, quarantine: false).perform + cached_location = Cask::Download.new(local_transmission).perform expect(cached_location).to be_quarantined end @@ -32,11 +32,23 @@ describe Cask::Quarantine, :cask do Cask::Cmd::Audit.run("local-transmission", "--download") local_transmission = Cask::CaskLoader.load(cask_path("local-transmission")) - cached_location = Cask::Download.new(local_transmission, force: false, quarantine: false).perform + cached_location = Cask::Download.new(local_transmission).perform expect(cached_location).to be_quarantined end + it "quarantines Cask installs even if the fetch was not" do + Cask::Cmd::Fetch.run("local-transmission", "--no-quarantine") + + Cask::Cmd::Install.run("local-transmission") + + expect( + Cask::CaskLoader.load(cask_path("local-transmission")), + ).to be_installed + + expect(Cask::Config.global.appdir.join("Transmission.app")).to be_quarantined + end + it "quarantines dmg-based Casks" do Cask::Cmd::Install.run("container-dmg") @@ -124,11 +136,32 @@ describe Cask::Quarantine, :cask do it "does not quarantine Cask fetches" do Cask::Cmd::Fetch.run("local-transmission", "--no-quarantine") local_transmission = Cask::CaskLoader.load(cask_path("local-transmission")) - cached_location = Cask::Download.new(local_transmission, force: false, quarantine: false).perform + cached_location = Cask::Download.new(local_transmission).perform expect(cached_location).to_not be_quarantined end + it "does not quarantine Cask audits" do + Cask::Cmd::Audit.run("local-transmission", "--download", "--no-quarantine") + + local_transmission = Cask::CaskLoader.load(cask_path("local-transmission")) + cached_location = Cask::Download.new(local_transmission).perform + + expect(cached_location).to_not be_quarantined + end + + it "does not quarantine Cask installs even if the fetch was" do + Cask::Cmd::Fetch.run("local-transmission") + + Cask::Cmd::Install.run("local-transmission", "--no-quarantine") + + expect( + Cask::CaskLoader.load(cask_path("local-transmission")), + ).to be_installed + + expect(Cask::Config.global.appdir.join("Transmission.app")).to_not be_quarantined + end + it "does not quarantine dmg-based Casks" do Cask::Cmd::Install.run("container-dmg", "--no-quarantine") @@ -200,14 +233,5 @@ describe Cask::Quarantine, :cask do expect(Cask::Config.global.appdir.join("MyNestedApp.app")).to_not be_quarantined end - - it "does not quarantine Cask audits" do - Cask::Cmd::Audit.run("local-transmission", "--download", "--no-quarantine") - - local_transmission = Cask::CaskLoader.load(cask_path("local-transmission")) - cached_location = Cask::Download.new(local_transmission, force: false, quarantine: false).perform - - expect(cached_location).to_not be_quarantined - end end end From 53b95c62604fb902425c93aa01d68dc873067635 Mon Sep 17 00:00:00 2001 From: "L. E. Segovia" <13498015+amyspark@users.noreply.github.com> Date: Fri, 7 Sep 2018 16:57:00 +0000 Subject: [PATCH 2/4] Cask: use native chmod to set write permissions Ruby chmod follows symlinks, which can point to non-existent files. This should fix quarantining Casks e.g. disk-inventory-x. --- Library/Homebrew/cask/quarantine.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index 80749cd15c..c51702171c 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -121,7 +121,7 @@ module Cask resolved_paths = Pathname.glob(to/"**/*", File::FNM_DOTMATCH) - FileUtils.chmod "u+w", resolved_paths + system_command!("/bin/chmod", args: ["-R", "u+w", to]) quarantiner = system_command("/usr/bin/xargs", args: [ From c7c14e1333aaa379a08014156c44abcb945dcd80 Mon Sep 17 00:00:00 2001 From: "L. E. Segovia" <13498015+amyspark@users.noreply.github.com> Date: Sat, 8 Sep 2018 14:00:44 +0000 Subject: [PATCH 3/4] Cask: split quarantine/release functions --- Library/Homebrew/cask/download.rb | 6 ++- Library/Homebrew/cask/quarantine.rb | 64 ++++++++++++++--------------- 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/Library/Homebrew/cask/download.rb b/Library/Homebrew/cask/download.rb index 8bb97466f6..0a5b2c3578 100644 --- a/Library/Homebrew/cask/download.rb +++ b/Library/Homebrew/cask/download.rb @@ -49,7 +49,11 @@ module Cask return if @quarantine.nil? return unless Quarantine.available? - Quarantine.cask!(cask: @cask, download_path: @downloaded_path, action: @quarantine) + if @quarantine + Quarantine.cask!(cask: @cask, download_path: @downloaded_path) + else + Quarantine.release!(download_path: @downloaded_path) + end end end end diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index c51702171c..35888b6749 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -67,46 +67,46 @@ module Cask fields.join(";") end + def release!(download_path: nil) + return unless detect(download_path) + + odebug "Releasing #{download_path} from quarantine" + + quarantiner = system_command("/usr/bin/xattr", + args: [ + "-d", + QUARANTINE_ATTRIBUTE, + download_path, + ], + print_stderr: false) + + return if quarantiner.success? + + raise CaskQuarantineReleaseError.new(download_path, quarantiner.stderr) + end + def cask!(cask: nil, download_path: nil, action: true) return if cask.nil? || download_path.nil? - if action - return if detect(download_path) + return if detect(download_path) - odebug "Quarantining #{download_path}" + odebug "Quarantining #{download_path}" - quarantiner = system_command(swift, - args: [ - QUARANTINE_SCRIPT, - download_path, - cask.url.to_s, - cask.homepage.to_s, - ]) + quarantiner = system_command(swift, + args: [ + QUARANTINE_SCRIPT, + download_path, + cask.url.to_s, + cask.homepage.to_s, + ]) - return if quarantiner.success? + return if quarantiner.success? - case quarantiner.exit_status - when 2 - raise CaskQuarantineError.new(download_path, "Insufficient parameters") - else - raise CaskQuarantineError.new(download_path, quarantiner.stderr) - end + case quarantiner.exit_status + when 2 + raise CaskQuarantineError.new(download_path, "Insufficient parameters") else - return unless detect(download_path) - - odebug "Releasing #{download_path} from quarantine" - - quarantiner = system_command("/usr/bin/xattr", - args: [ - "-d", - QUARANTINE_ATTRIBUTE, - download_path, - ], - print_stderr: false) - - return if quarantiner.success? - - raise CaskQuarantineReleaseError.new(download_path, quarantiner.stderr) + raise CaskQuarantineError.new(download_path, quarantiner.stderr) end end From 124a8109ce18411a108f715a605faae1135d2483 Mon Sep 17 00:00:00 2001 From: "L. E. Segovia" <13498015+amyspark@users.noreply.github.com> Date: Sat, 8 Sep 2018 20:20:25 +0000 Subject: [PATCH 4/4] Cask: rename no-translocation-bit changer function --- Library/Homebrew/cask/quarantine.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/cask/quarantine.rb b/Library/Homebrew/cask/quarantine.rb index 35888b6749..1e65cc2195 100644 --- a/Library/Homebrew/cask/quarantine.rb +++ b/Library/Homebrew/cask/quarantine.rb @@ -55,7 +55,7 @@ module Cask print_stderr: false).stdout.rstrip end - def disable_translocation!(xattr) + def toggle_no_translocation_bit(xattr) fields = xattr.split(";") # Fields: status, epoch, download agent, event ID @@ -117,7 +117,7 @@ module Cask odebug "Propagating quarantine from #{from} to #{to}" - quarantine_status = disable_translocation!(status(from)) + quarantine_status = toggle_no_translocation_bit(status(from)) resolved_paths = Pathname.glob(to/"**/*", File::FNM_DOTMATCH)