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

extend/os/mac/keg: codesign on Intel if invalid signature #15903

Merged
merged 1 commit into from
Aug 27, 2023
Merged

extend/os/mac/keg: codesign on Intel if invalid signature #15903

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Library/Homebrew/extend/os/mac/keg.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ def binary_executable_or_library_files

def codesign_patched_binary(file)
return if MacOS.version < :big_sur
Copy link
Member

@Bo98 Bo98 Aug 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This OS conditional can probably be removed, arm64 can't be < Big Sur anyway.

Though we should check if the commands below haven't changed over the years.

return unless Hardware::CPU.arm?

unless Hardware::CPU.arm?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be attempted regardless to CPU?

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, I was wondering if there would be any downside to unconditionally code-signing patched binaries?

Vaguely relatedly: I wonder if it would ever be possible to:

  • avoid patching/rewriting binaries installed into the default prefix (particularly cellar :any or cellar :any_no_relocation
  • codesign binaries at CI time

CC @woodruffw as we've talked about this recently too.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see no reason not to attempt signature verification on other archs 🙂

And yep, I think it'd be possible to do things "right" here and push codesigning into CI (with the assumption that 99% of users wouldn't need to perform any relocations and could use the CI-supplied signature.) The main challenges that I can think of these are mostly logistical ones (e.g. around ensuring that the signing step remains secure.)

Copy link
Member

Choose a reason for hiding this comment

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

Yeh, I was wondering if there would be any downside to unconditionally code-signing patched binaries?

Intel doesn't actually require dev tools installed, so we could be breaking existing setups there. If we implement codesigning in Ruby or something, this downside will disappear.

codesign binaries at CI time

This only works if there is no clientside modifications (e.g. relocation). The code here already does apply to CI time too, so if there's only CI-time modifications then codesigning will only apply there.

The number of formula we ship that's already codesigned on Intel is very small. The only known case is Qemu, which only recently broke as it didn't have any binary modifications until #15571 (and that change only applies on the CI side, so only took affect in a recent formula update).

With that said, I think it would be good idea to ship with the smallest necessary scope now (i.e. only codesign on Intel if there's an existing code signature that's been broken) so we can rebuild Qemu. Then we can have a bigger discussion on if we want anything more in the short/medium/long-term.

result = system_command("codesign", args: ["--verify", file], print_stderr: false)
return unless result.stderr.match?(/invalid signature/i)
end

odebug "Codesigning #{file}"
prepare_codesign_writable_files(file) do
Expand Down