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.

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

Apply an ad-hoc signature after modifying IDs #260

Merged
merged 5 commits into from
Oct 11, 2020

Conversation

mistydemeo
Copy link
Contributor

If we make any changes to a signed binary, we invalidate the signature. Without resigning, the resulting binary is unusable. To my knowledge, there's no disadvantage to signing an unsigned binary using an ad-hoc signature, so we can just do this unconditionally to ensure we don't end up creating bad binaries. The ad-hoc signature should always be available and always work, so this should be safe to do in all circumstances.

I do still need to confirm which OS versions this will work with.

@mistydemeo mistydemeo force-pushed the sign_when_changing_dylib_id branch 2 times, most recently from da37443 to f1280cd Compare September 28, 2020 20:30
If we make any changes to a signed binary, we invalidate the signature.
Without resigning, the resulting binary is unusable. To my knowledge,
there's no disadvantage to signing an unsigned binary using an ad-hoc
signature, so we can just do this unconditionally to ensure we don't end
up creating bad binaries. The ad-hoc signature should always be available
and always work, so this should be safe to do in all circumstances.
@mistydemeo mistydemeo force-pushed the sign_when_changing_dylib_id branch from f1280cd to c55a574 Compare September 28, 2020 20:33
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM! Do you happen to have a before-and-after diff of the ad-hoc signed binary? I can look into reimplementing it in pure-Ruby at a later point 🙂

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me! A few style suggestions but something like this would be a good fit.

# @raise [ModificationError] if the operation fails
def self.codesign!(filename)
# codesign binary is not available on Linux
return if RUBY_PLATFORM !~ /darwin/
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think it might make sense to raise an UnsupportedError or similar here instead of silently returning. I can take care of that this afternoon, if that sounds good to others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking of having that be rescued in each of the Tools methods?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I guess it would have to be, to avoid breaking those methods on non-macOS platforms 😞

Hmm, that itself isn't ideal. I'm fine with leaving it as is for now 😅

@SMillerDev
Copy link
Member

I'm having an issue on the Big Sur beta that fails some ruby gem builds with this error, is it related?:

Error: Failed to read Mach-O binary: /usr/local/Cellar/test-kitchen/2.7.2/libexec/extensions/universal-darwin-20/2.6.0/bcrypt_pbkdf-1.0.1/bcrypt_pbkdf_ext.bundle
Error: An exception occurred within a child process:
  MachO::LoadCommandError: Unrecognized Mach-O load command: 0x80000034

@woodruffw
Copy link
Member

@SMillerDev That's unrelated, but it's also a new change to Mach-O:

#define LC_DYLD_EXPORTS_TRIE (0x33 | LC_REQ_DYLD) /* used with linkedit_data_command, payload is trie */
#define LC_DYLD_CHAINED_FIXUPS (0x34 | LC_REQ_DYLD) /* used with linkedit_data_command */

I'll create a PR for it in a moment.

@woodruffw woodruffw merged commit b44b172 into Homebrew:master Oct 11, 2020
@mistydemeo mistydemeo deleted the sign_when_changing_dylib_id branch October 12, 2020 18:38
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants