From e90371f8abd188c046692ed4998737cba656e697 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Mon, 1 Aug 2022 14:30:04 +0200 Subject: [PATCH 1/2] cask: add audit for incorrect signing --- Library/Homebrew/cask/audit.rb | 41 ++++++++++++++++--- Library/Homebrew/cask/auditor.rb | 5 +++ Library/Homebrew/cask/cmd/audit.rb | 5 +++ Library/Homebrew/test/cask/audit_spec.rb | 52 +++++++++++++++++++++++- 4 files changed, 96 insertions(+), 7 deletions(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index 91bcf2d3ec..d8f703710d 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -20,28 +20,31 @@ module Cask attr_reader :cask, :download - attr_predicate :appcast?, :new_cask?, :strict?, :online?, :token_conflicts? + attr_predicate :appcast?, :new_cask?, :strict?, :signing?, :online?, :token_conflicts? def initialize(cask, appcast: nil, download: nil, quarantine: nil, - token_conflicts: nil, online: nil, strict: nil, + token_conflicts: nil, online: nil, strict: nil, signing: nil, new_cask: nil) - # `new_cask` implies `online` and `strict` + # `new_cask` implies `online`, `token_conflicts`, `strict` and signing online = new_cask if online.nil? strict = new_cask if strict.nil? + signing = new_cask if signing.nil? + token_conflicts = new_cask if token_conflicts.nil? # `online` implies `appcast` and `download` appcast = online if appcast.nil? download = online if download.nil? - # `new_cask` implies `token_conflicts` - token_conflicts = new_cask if token_conflicts.nil? + # `signing` implies `download` + download = signing if download.nil? @cask = cask @appcast = appcast @download = Download.new(cask, quarantine: quarantine) if download @online = online @strict = strict + @signing = signing @new_cask = new_cask @token_conflicts = token_conflicts end @@ -81,6 +84,7 @@ module Cask check_github_repository_archived check_github_prerelease_version check_bitbucket_repository + check_signing self rescue => e odebug e, e.backtrace @@ -550,6 +554,33 @@ module Cask add_error "download not possible: #{e}" end + def check_signing + return if !signing? || download.blank? || cask.url.blank? + + odebug "Auditing signing" + odebug cask.artifacts + artifacts = cask.artifacts.select { |k| k.is_a?(Artifact::Pkg) || k.is_a?(Artifact::App) } + + return if artifacts.empty? + + downloaded_path = download.fetch + primary_container = UnpackStrategy.detect(downloaded_path, type: @cask.container&.type, merge_xattrs: true) + + return if primary_container.nil? + + Dir.mktmpdir do |tmpdir| + tmpdir = Pathname(tmpdir) + primary_container.extract_nestedly(to: tmpdir, basename: downloaded_path.basename, verbose: false) + cask.artifacts.each do |artifact| + result = system_command("codesign", args: [ + "--verify", + tmpdir/artifact.source.basename, + ], print_stderr: false) + add_warning result.merged_output unless result.success? + end + end + end + def check_livecheck_version return unless appcast? diff --git a/Library/Homebrew/cask/auditor.rb b/Library/Homebrew/cask/auditor.rb index 508fb0a379..2b80340860 100644 --- a/Library/Homebrew/cask/auditor.rb +++ b/Library/Homebrew/cask/auditor.rb @@ -15,6 +15,7 @@ module Cask audit_online: nil, audit_new_cask: nil, audit_strict: nil, + audit_signing: nil, audit_token_conflicts: nil, quarantine: nil, any_named_args: nil, @@ -29,6 +30,7 @@ module Cask audit_online: audit_online, audit_new_cask: audit_new_cask, audit_strict: audit_strict, + audit_signing: audit_signing, audit_token_conflicts: audit_token_conflicts, quarantine: quarantine, any_named_args: any_named_args, @@ -46,6 +48,7 @@ module Cask audit_appcast: nil, audit_online: nil, audit_strict: nil, + audit_signing: nil, audit_token_conflicts: nil, audit_new_cask: nil, quarantine: nil, @@ -60,6 +63,7 @@ module Cask @audit_online = audit_online @audit_new_cask = audit_new_cask @audit_strict = audit_strict + @audit_signing = audit_signing @quarantine = quarantine @audit_token_conflicts = audit_token_conflicts @any_named_args = any_named_args @@ -133,6 +137,7 @@ module Cask appcast: @audit_appcast, online: @audit_online, strict: @audit_strict, + signing: @audit_signing, new_cask: @audit_new_cask, token_conflicts: @audit_token_conflicts, download: @audit_download, diff --git a/Library/Homebrew/cask/cmd/audit.rb b/Library/Homebrew/cask/cmd/audit.rb index 516f4199bd..be8a6abeb4 100644 --- a/Library/Homebrew/cask/cmd/audit.rb +++ b/Library/Homebrew/cask/cmd/audit.rb @@ -19,6 +19,8 @@ module Cask description: "Audit the appcast" switch "--[no-]token-conflicts", description: "Audit for token conflicts" + switch "--[no-]signing", + description: "Audit for signed apps, which is required on ARM" switch "--[no-]strict", description: "Run additional, stricter style checks" switch "--[no-]online", @@ -50,6 +52,7 @@ module Cask appcast: args.appcast?, online: args.online?, strict: args.strict?, + signing: args.signing?, new_cask: args.new_cask?, token_conflicts: args.token_conflicts?, quarantine: args.quarantine?, @@ -71,6 +74,7 @@ module Cask appcast: nil, online: nil, strict: nil, + signing: nil, new_cask: nil, token_conflicts: nil, quarantine: nil, @@ -84,6 +88,7 @@ module Cask audit_appcast: appcast, audit_online: online, audit_strict: strict, + audit_signing: signing, audit_new_cask: new_cask, audit_token_conflicts: token_conflicts, quarantine: quarantine, diff --git a/Library/Homebrew/test/cask/audit_spec.rb b/Library/Homebrew/test/cask/audit_spec.rb index 212cca3405..020b8dfe33 100644 --- a/Library/Homebrew/test/cask/audit_spec.rb +++ b/Library/Homebrew/test/cask/audit_spec.rb @@ -411,6 +411,53 @@ describe Cask::Audit, :cask do end end + describe "signing checks" do + let(:download_double) { instance_double(Cask::Download) } + let(:unpack_double) { instance_double(UnpackStrategy::Zip) } + + before do + allow(audit).to receive(:download).and_return(download_double) + allow(audit).to receive(:signing?).and_return(true) + allow(audit).to receive(:check_https_availability) + end + + context "when cask is not using a signed artifact" do + let(:cask) do + tmp_cask "signing-cask-test", <<~RUBY + cask 'signing-cask-test' do + version '1.0' + url "https://brew.sh/index.html" + binary 'Audit.app' + end + RUBY + end + + it "does not fail" do + expect(download_double).not_to receive(:fetch) + expect(UnpackStrategy).not_to receive(:detect) + expect(run).not_to warn_with(/Audit\.app/) + end + end + + context "when cask is using a signed artifact" do + let(:cask) do + tmp_cask "signing-cask-test", <<~RUBY + cask 'signing-cask-test' do + version '1.0' + url "https://brew.sh/" + pkg 'Audit.app' + end + RUBY + end + + it "does not fail since no extract" do + allow(download_double).to receive(:fetch).and_return(Pathname.new("/tmp/test.zip")) + allow(UnpackStrategy).to receive(:detect).and_return(nil) + expect(run).not_to warn_with(/Audit\.app/) + end + end + end + describe "livecheck should be skipped" do let(:online) { true } let(:message) { /Version '[^']*' differs from '[^']*' retrieved by livecheck\./ } @@ -888,7 +935,7 @@ describe Cask::Audit, :cask do end describe "audit of downloads" do - let(:cask_token) { "with-binary" } + let(:cask_token) { "basic-cask" } let(:cask) { Cask::CaskLoader.load(cask_token) } let(:download_double) { instance_double(Cask::Download) } let(:message) { "Download Failed" } @@ -896,10 +943,11 @@ describe Cask::Audit, :cask do before do allow(audit).to receive(:download).and_return(download_double) allow(audit).to receive(:check_https_availability) + allow(UnpackStrategy).to receive(:detect).and_return(nil) end it "when download and verification succeed it does not fail" do - expect(download_double).to receive(:fetch) + expect(download_double).to receive(:fetch).and_return(Pathname.new("/tmp/test.zip")) expect(run).to pass end From ca65777e7053e419432257bbdf08c211dba50635 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Tue, 16 Aug 2022 10:01:35 +0200 Subject: [PATCH 2/2] cask/audit: improve wording Co-authored-by: Mike McQuaid --- Library/Homebrew/cask/audit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/cask/audit.rb b/Library/Homebrew/cask/audit.rb index d8f703710d..98944e506d 100644 --- a/Library/Homebrew/cask/audit.rb +++ b/Library/Homebrew/cask/audit.rb @@ -26,7 +26,7 @@ module Cask token_conflicts: nil, online: nil, strict: nil, signing: nil, new_cask: nil) - # `new_cask` implies `online`, `token_conflicts`, `strict` and signing + # `new_cask` implies `online`, `token_conflicts`, `strict` and `signing` online = new_cask if online.nil? strict = new_cask if strict.nil? signing = new_cask if signing.nil?