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

cho-m · 2023-08-23T19:06:59Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Attempt at fixing issue mentioned in Homebrew/homebrew-core#140244.

Due to enabling HOMEBREW_RELOCATABLE_INSTALL_NAMES=1, RPATH modifications are invalidating code signatures added by upstream installation script.

Possible change here to codesign if we detect invalid signature on Intel binary. Alternative could be on formula-by-formula basis, though harder to catch problems before distribution to users.

Snippet of debug output on qemu:

==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/elf2dmp
  from /usr/local/opt/glib/lib/libglib-2.0.0.dylib
    to @loader_path/../../../../opt/glib/lib/libglib-2.0.0.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/elf2dmp

==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-edid
  from /usr/local/opt/glib/lib/libglib-2.0.0.dylib
    to @loader_path/../../../../opt/glib/lib/libglib-2.0.0.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-edid

...

==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/pixman/lib/libpixman-1.0.dylib
    to @loader_path/../../../../opt/pixman/lib/libpixman-1.0.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning (2nd try) /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --sign - --force --preserve-metadata=entitlements,requirements,flags,runtime /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/capstone/lib/libcapstone.4.dylib
    to @loader_path/../../../../opt/capstone/lib/libcapstone.4.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/libpng/lib/libpng16.16.dylib
    to @loader_path/../../../../opt/libpng/lib/libpng16.16.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/jpeg-turbo/lib/libjpeg.8.dylib
    to @loader_path/../../../../opt/jpeg-turbo/lib/libjpeg.8.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/gnutls/lib/libgnutls.30.dylib
    to @loader_path/../../../../opt/gnutls/lib/libgnutls.30.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/snappy/lib/libsnappy.1.dylib
    to @loader_path/../../../../opt/snappy/lib/libsnappy.1.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/lzo/lib/liblzo2.2.dylib
    to @loader_path/../../../../opt/lzo/lib/liblzo2.2.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/glib/lib/libgio-2.0.0.dylib
    to @loader_path/../../../../opt/glib/lib/libgio-2.0.0.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Changing install name in /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
  from /usr/local/opt/glib/lib/libgobject-2.0.0.dylib
    to @loader_path/../../../../opt/glib/lib/libgobject-2.0.0.dylib
/usr/bin/env codesign --verify /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
==> Codesigning /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
/usr/bin/env codesign --display --file-list - /usr/local/Cellar/qemu/8.1.0/bin/qemu-system-x86_64
...

AkihiroSuda · 2023-08-23T19:15:53Z

Library/Homebrew/extend/os/mac/keg.rb

@@ -28,7 +28,11 @@ def binary_executable_or_library_files

  def codesign_patched_binary(file)
    return if MacOS.version < :big_sur
-    return unless Hardware::CPU.arm?
+
+    unless Hardware::CPU.arm?


Maybe this should be attempted regardless to CPU?

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.

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.)

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.

MikeMcQuaid

Thanks for the PR! Requesting some other reviews.

MikeMcQuaid · 2023-08-24T12:09:51Z

Library/Homebrew/extend/os/mac/keg.rb

@@ -28,7 +28,11 @@ def binary_executable_or_library_files

  def codesign_patched_binary(file)
    return if MacOS.version < :big_sur
-    return unless Hardware::CPU.arm?
+
+    unless Hardware::CPU.arm?


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.

AkihiroSuda · 2023-08-27T00:35:38Z

Is there any blocker to get this merged ? 🙏

Bo98 · 2023-08-27T03:59:42Z

Library/Homebrew/extend/os/mac/keg.rb

@@ -28,7 +28,11 @@ def binary_executable_or_library_files

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


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.

Bo98 · 2023-08-27T04:11:02Z

Given this is breaking things for a number of people, and the reviews so far have only been about expanding scope, I'll merge this now given it already covers the scope necessary to fix Qemu.

carlocab · 2023-08-27T07:54:25Z

Thanks @cho-m!

cho-m · 2023-08-27T16:31:30Z

Anyone know situation with bottle using absolute path vs rewriting to @loader_path?

I checked new qemu bottle after user comments in Homebrew/homebrew-core#140244 and noticed new bottle is back to using absolute path placeholder (@@HOMEBREW_PREFIX@@). EDIT: Looks like this time (even without @loader_path rewrite) the bottle's binaries have invalid signatures so resigning needs to happen during pour, which means fix is only there for users on brew HEAD.

I had tested this commit using HOMEBREW_RELOCATABLE_INSTALL_NAMES=1 brew install --build-bottle qemu --debug where rewrites to @loader_path did happen as seen in first post.

AkihiroSuda · 2023-08-28T00:21:18Z

Thank you for working on this.
The issue is now fixed on my machine, but some users say the issue is still unfixed in their machines.

qemu: invalid signature (code or signature have been modified) (regression in 8.0.4, fixed in 8.0.4-1, re-regression in 8.1.0) homebrew-core#140244

Could you take a look?

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

7412487

AkihiroSuda reviewed Aug 23, 2023

View reviewed changes

MikeMcQuaid requested a review from carlocab August 24, 2023 12:07

MikeMcQuaid reviewed Aug 24, 2023

View reviewed changes

MikeMcQuaid requested a review from woodruffw August 24, 2023 12:10

rfay mentioned this pull request Aug 25, 2023

Colima/Lima broken and unusable on Intel/amd64 ddev/ddev#5298

Closed

1 task

Bo98 reviewed Aug 27, 2023

View reviewed changes

Bo98 approved these changes Aug 27, 2023

View reviewed changes

Bo98 merged commit b0c5405 into Homebrew:master Aug 27, 2023
24 checks passed

cho-m deleted the codesign-intel-if-invalid branch August 27, 2023 16:26

scpeters mentioned this pull request Sep 8, 2023

Speed up keg installation with fewer code-signing calls #15980

Merged

7 tasks

github-actions bot added the outdated PR was locked due to age label Sep 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cho-m commented Aug 23, 2023

AkihiroSuda Aug 23, 2023

MikeMcQuaid Aug 24, 2023

woodruffw Aug 27, 2023

Bo98 Aug 27, 2023

MikeMcQuaid left a comment

MikeMcQuaid Aug 24, 2023

AkihiroSuda commented Aug 27, 2023

Bo98 Aug 27, 2023 •

edited

Bo98 commented Aug 27, 2023

carlocab commented Aug 27, 2023

cho-m commented Aug 27, 2023 •

edited

AkihiroSuda commented Aug 28, 2023

		@@ -28,7 +28,11 @@ def binary_executable_or_library_files

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

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

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

Conversation

cho-m commented Aug 23, 2023

AkihiroSuda Aug 23, 2023

Choose a reason for hiding this comment

MikeMcQuaid Aug 24, 2023

Choose a reason for hiding this comment

woodruffw Aug 27, 2023

Choose a reason for hiding this comment

Bo98 Aug 27, 2023

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Aug 24, 2023

Choose a reason for hiding this comment

AkihiroSuda commented Aug 27, 2023

Bo98 Aug 27, 2023 • edited

Choose a reason for hiding this comment

Bo98 commented Aug 27, 2023

carlocab commented Aug 27, 2023

cho-m commented Aug 27, 2023 • edited

AkihiroSuda commented Aug 28, 2023

Bo98 Aug 27, 2023 •

edited

cho-m commented Aug 27, 2023 •

edited