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.

Sign up for GitHub

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

Jump to bottom

cask/audit: update signing checks for app, binary, and pkg #17031

Merged
merged 1 commit into from
May 22, 2024
Merged

cask/audit: update signing checks for app, binary, and pkg #17031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions Library/Homebrew/cask/audit.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,13 +482,25 @@
odebug "Auditing signing"

extract_artifacts do |artifacts, tmpdir|
is_container = artifacts.any? { |a| a.is_a?(Artifact::App) || a.is_a?(Artifact::Pkg) }

Check warning on line 485 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L485 

Added line #L485 was not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a standalone binary (typically a shell script) included in or with an application bundle, we can skip auditing it.

Many times those scripts aren't signed, but if the application is we'll consider it trusted. If the app isn't trusted, it will fail the normal check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!


artifacts.each do |artifact|
next if artifact.is_a?(Artifact::Binary) && is_container == true

artifact_path = artifact.is_a?(Artifact::Pkg) ? artifact.path : artifact.source
path = tmpdir/artifact_path.relative_path_from(cask.staged_path)

next unless path.exist?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was silently skipping over paths not found, which was causing us not to audit direct binary downloads properly.

path = tmpdir/artifact_path.relative_path_from(cask.staged_path)

Check warning on line 492 in Library/Homebrew/cask/audit.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/cask/audit.rb#L492 

Added line #L492 was not covered by tests

result = system_command("spctl", args: ["--assess", "--type", "install", path], print_stderr: false)
result = case artifact
when Artifact::Pkg
system_command("spctl", args: ["--assess", "--type", "install", path], print_stderr: false)
when Artifact::App
system_command("spctl", args: ["--assess", "--type", "execute", path], print_stderr: false)
when Artifact::Binary
system_command("codesign", args: ["--verify", path], print_stderr: false)
else
add_error "Unknown artifact type: #{artifact.class}", location: cask.url.location
end
Comment on lines +495 to +503
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested against a range of applications and binaries, this reports the same as tools like Apparency and Whatsyoursign.

Happy to publish a results report run against applications locally for review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I believe you, great work!


next if result.success?

Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/unpack_strategy/uncompressed.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def extract_nestedly(to: nil, basename: nil, verbose: false, prioritize_extensio

sig { override.params(unpack_dir: Pathname, basename: Pathname, verbose: T::Boolean).returns(T.untyped) }
def extract_to_dir(unpack_dir, basename:, verbose: false)
FileUtils.cp path, unpack_dir/basename, preserve: true, verbose:
FileUtils.cp path, unpack_dir/basename.sub(/^[\da-f]{64}--/, ""), preserve: true, verbose:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed during the audit signing that this caused audit to fail as the paths didn't exist. This applied to direct binary downloads only. Making this adjustment now causes the paths to be found.

end
end
end