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

link: don't link openssl. #597

Merged
merged 1 commit into from
Jul 29, 2016
Merged

link: don't link openssl. #597

merged 1 commit into from
Jul 29, 2016

Conversation

MikeMcQuaid
Copy link
Member

  • 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 successfully run brew tests with your changes locally?

See https://gist.github.com/tdsmith/4b502c5cc6e7d358acdf for reproduction instructions of the security hole this creates. Unfortunately this is recommended in locations like https://www.microsoft.com/net/core#macos (which provide no specific contact information) that we will unfortunately break but there's not really any other way around. This will scale better than the current system of "Homebrew maintainers notice sites recommend this and ask them to not do so".

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jul 27, 2016
Refusing to link: openssl
Linking keg-only OpenSSL means you may end up linking against the insecure,
deprecated system version while using the headers from the Homebrew version.
Instead, specify the full include/library paths to your compiler.
Copy link
Contributor

Choose a reason for hiding this comment

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

The “[…] to your compiler.” reads kinda awkward to me. Maybe “[…] during the build.” works better?

@UniqMartin
Copy link
Contributor

👍 and one minor nit.

@tdsmith
Copy link
Contributor

tdsmith commented Jul 28, 2016

Should we add something like "Consider running export CPPFLAGS=-I$(brew --prefix openssl)/include; export LDFLAGS=-L$(brew --prefix openssl)/include before running build scripts for projects which depend on openssl" to help folks along?

@tdsmith
Copy link
Contributor

tdsmith commented Jul 28, 2016

I think it should be allowed when prefix isn't exactly /usr/local too.

@tdsmith
Copy link
Contributor

tdsmith commented Jul 28, 2016

Blog post explaining this phenomenon: https://langui.sh/2015/07/24/osx-clang-include-lib-search-paths/

@MikeMcQuaid
Copy link
Member Author

Have tweaked message, added compiled path guidelines based on projects that do this properly and made this only apply to /usr/local Homebrew installations.

@MikeMcQuaid
Copy link
Member Author

Oh, and added that blog post to the commit message.

@MikeMcQuaid MikeMcQuaid merged commit b999edb into Homebrew:master Jul 29, 2016
@MikeMcQuaid MikeMcQuaid deleted the link-no-openssl branch July 29, 2016 23:00
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jul 29, 2016
@bartonjs
Copy link

@MikeMcQuaid https://github.com/dotnet/core

We'll happily accept different instructions to provide. We change the dylib reference to use rpath for the specific version; so what we care about the most is that libcrypto.1.0.0.dylib and libssl.1.0.0.dylib end up on the default probing path. We don't care about the unversioned library forwarder, or particularly care about the header file path (since the unlinked location is good enough to be found by cmake). So something like brew linkversionedfilesonly openssl would be fine.

@UniqMartin
Copy link
Contributor

We change the dylib reference to use rpath for the specific version; […]

Can you explain in a bit more detail what this means? I don't understand.

[…]; so what we care about the most is that libcrypto.1.0.0.dylib and libssl.1.0.0.dylib end up on the default probing path.

Why is this necessary? Why can't you look into an arbitrary user-provided path? Not everyone installs Homebrew to /usr/local and as you've learned from this PR and the linked resources, linking openssl files to /usr/local/{include,lib} is a bad idea. (And yes, I realize there's a difference between libcrypto.1.0.0.dylib and libcrypto.dylib.)

So something like brew linkversionedfilesonly openssl would be fine.

This looks like a very narrow use case for a single consumer, so I doubt we're going to provide something like this. (But as stated in the previous paragraph, I think you're limiting yourself unnecessarily by relying on the default lookup paths and it would be good to understand where this limitation comes from before we can discuss possible alternatives for you and your users.)

@bartonjs
Copy link

@UniqMartin We want to support a) homebrew users, b) macports users, and c) people who want to distribute a standalone copy of the library with their application. rpath seemed to be the best way to do this.

In particular, since we distribute precompiled assets (vs building on the end user machines) it would be the path it had on our build machines that was encoded for the library load, so we need to be flexible on the install destinations.

@UniqMartin
Copy link
Contributor

@bartonjs Have you considered using install_name_tool -change to adjust the dylib references inside your precompiled binaries once you learn where the libraries live you want to use? Or since you're referencing the libraries as @rpath/libcrypto.1.0.0.dylib and @rpath/libssl.1.0.0.dylib from System.Security.Cryptography.Native.dylib you could use install_name_tool -add_rpath to add an appropriate LC_RPATH load command to the latter.

On macOS you're expected to link to libraries with their full path (either by directly using an absolute path or by using paths relative to the executable or loading dylib). What you currently seem to rely on for production use is what is described as DYLD_FALLBACK_LIBRARY_PATH in dyld(1) which—as the name implies—is really just a fallback.

A more macOS-like experience would be of course to bundle all necessary libraries with your package. And if your crypto component relies on OpenSSL as a backend and cannot use the libraries natively provided by the OS, the most user-friendly solution would be to bundle those external dependencies to make your package fully self-contained.

In particular, since we distribute precompiled assets (vs building on the end user machines) it would be the path it had on our build machines that was encoded for the library load, so we need to be flexible on the install destinations.

The paths you've used during compilation are not set in stone. On macOS, it is fairly common for build systems to adjust those (usually absolute) paths after binaries have been built by using install_name_tool -change and install_name_tool -id (as necessary).

@MikeMcQuaid
Copy link
Member Author

This looks like a very narrow use case for a single consumer, so I doubt we're going to provide something like this. (But as stated in the previous paragraph, I think you're limiting yourself unnecessarily by relying on the default lookup paths and it would be good to understand where this limitation comes from before we can discuss possible alternatives for you and your users.)

As @UniqMartin says: unfortunately we're not going to provide this.

A more macOS-like experience would be of course to bundle all necessary libraries with your package. And if your crypto component relies on OpenSSL as a backend and cannot use the libraries natively provided by the OS, the most user-friendly solution would be to bundle those external dependencies to make your package fully self-contained.

Agreed.

@bartonjs
Copy link

Have you considered using install_name_tool -change to adjust the dylib references inside your precompiled binaries once you learn where the libraries live you want to use?

I'm not sure I follow. We certainly know where the libraries are on our build machines, but we have no idea where the user's machine would have put them. We already use install_name_tool -change to rewrite it to be an rpath reference.

On macOS you're expected to link to libraries with their full path (either by directly using an absolute path or by using paths relative to the executable or loading dylib).

If I recall correctly, the rpath/probing support is 'relatively' recent on MacOS, meaning that Apple saw a need for more flexibility (and to acting like all the other OSes). And CMake certainly seems to think that rpath-flexibility is better: https://cmake.org/cmake/help/v3.0/prop_tgt/MACOSX_RPATH.html.

A more macOS-like experience would be of course to bundle all necessary libraries with your package. And if your crypto component relies on OpenSSL as a backend and cannot use the libraries natively provided by the OS, the most user-friendly solution would be to bundle those external dependencies to make your package fully self-contained.

Yeah, but then we're in the boat of needing to release an updated Mac release every time there's an OpenSSL security fix, which (I'm sure you're aware) is a fairly regular occurrence.

So, since we've already shipped, and this change now makes our getting started instructions not function, do you have any particular recommendations? Is there an ABI-version-only path under a default brew install that we can use as the target of a symlink (I can't easily check since I don't have my Mac with me; and last time we found 'a working solution' it got taken away 😄)? That is, /usr/local/Cellar/openssl/1.0.2h/lib/libcrypto.1.0.0.dylib is a path using the patch version, which changes frequently. /usr/local/Cellar/openssl/1.0.2/lib/libcrypto.1.0.0.dylib (if guaranteed) is better, /usr/local/Cellar/openssl/lib/libcrypto.1.0.0.dylib is great, assuming that 1.1.0 (incompatible ABI) would be side-by-side in that location. /usr/local/Cellar/openssl/1.0.0/lib/libcrypto.1.0.0.dylib would also be good, using the library ABI version in the path (instead of the library release version).

Our goals:

  • MacOS users can use .NET Core
  • Security updates to OpenSSL can be delivered to users independent of our release schedule
  • While documentation may assume a single dependency source (e.g. homebrew), have no hard dependence on that dependency source
  • Have as simple of getting started instructions as possible given preceding goals

Heck, now that I've written this I feel like I have to ask: Why can't brew install openssl symlink in the (versioned) library assets already? Since they're versioned they don't compete with the old 0.9.8 library, and with no unversioned name it doesn't change default linker behaviors.

@MikeMcQuaid
Copy link
Member Author

Yeah, but then we're in the boat of needing to release an updated Mac release every time there's an OpenSSL security fix, which (I'm sure you're aware) is a fairly regular occurrence.

I don't think delegating the responsibility of that to Homebrew is particularly responsible to your users. I'm afraid regardless of need: your use-case is not something we're willing to support. Sorry.

@DomT4
Copy link
Member

DomT4 commented Jul 31, 2016

That is, /usr/local/Cellar/openssl/1.0.2h/lib/libcrypto.1.0.0.dylib is a path using the patch version, which changes frequently. /usr/local/Cellar/openssl/1.0.2/lib/libcrypto.1.0.0.dylib (if guaranteed) is better, /usr/local/Cellar/openssl/lib/libcrypto.1.0.0.dylib is great, assuming that 1.1.0 (incompatible ABI) would be side-by-side in that location. /usr/local/Cellar/openssl/1.0.0/lib/libcrypto.1.0.0.dylib would also be good, using the library ABI version in the path (instead of the library release version).

You're basically talking about /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib here, but again, a warning you can't always assume /usr/local.

We also don't promise API/ABI compatibility IRT to external projects; it won't happen immediately but OpenSSL 1.1.0 is due to land at some point & eventually will take the openssl name in all likelihood, and in the mean time SSLv3 may well end up being removed from the existing formula, which could cause issues outside brew usage.

If you want stability, bundling the libraries into your project is a reasonable step & the "done" thing on OS X/macOS. I understand it can be a frustration around having to issue updates purely for OpenSSL security issues, but that is part of the OS X/macOS ecosystem really & is what Apple themselves try to prod people towards.

@UniqMartin
Copy link
Contributor

If I recall correctly, the rpath/probing support is 'relatively' recent on MacOS, meaning that Apple saw a need for more flexibility (and to acting like all the other OSes).

Still they expect you to link to a library using it's absolute path, and relying on the default search path of the dynamic linker is mostly a developer feature (in my understanding). That you can use @rpath/ as a prefix in the linking information in Mach-O binaries doesn't change the fact you are expected to use an absolute path. It's just that you've got more flexibility with what @rpath/ will be substituted eventually: A list of paths that is populated with the contents of LC_RPATH load commands of binaries involved in the dynamic linking process.

And CMake certainly seems to think that rpath-flexibility is better: https://cmake.org/cmake/help/v3.0/prop_tgt/MACOSX_RPATH.html.

It's just stating that the system is more flexible (which is true). It's not saying that it's better.

Heck, now that I've written this I feel like I have to ask: Why can't brew install openssl symlink in the (versioned) library assets already? Since they're versioned they don't compete with the old 0.9.8 library, and with no unversioned name it doesn't change default linker behaviors.

Because that's not in the design of Homebrew. We either link everything outside of libexec (most formulae) or we don't link at all (keg-only formulae, of which openssl is one example). I'm not saying this will never change, but that's something to discuss and there needs to be a clear benefit for our user base that outweighs the added complexity.

Ironically, if Apple had just removed the /usr/lib/libcrypto.dylib and /usr/lib/libssl.dylib symlinks (they are not needed by binaries previously linked to the system OpenSSL) at the same time they stopped distributing the OpenSSL headers, everything would have been easier and we might not even have this discussion.

To be a bit frank, you've been relying on discouraged behavior from the start. That you had to use the --force option should have made you think. It's a power-user option to accommodate certain use cases, but it's not something I would suggest lightly to users that don't understand the consequences (and apparently nobody writing those instructions did). That's all okay and understandable, but I don't think it's fair to shift responsibility of fixing your use case to us because we noticed the abuse and made it harder to perform an unsafe and security-relevant operation (without an advance warning because there was no contact information).

It's obviously up to your project how you operate and where your priorities are, but I think on that matter you should really consider bundling OpenSSL as a short-term solution and then port your SSL/TLS layer to use Secure Transport on macOS as long-term solution (because that's what Apple expects you to use and where you are covered by the vendor's security updates).

@DomT4
Copy link
Member

DomT4 commented Jul 31, 2016

Ironically, if Apple had just removed the /usr/lib/libcrypto.dylib and /usr/lib/libssl.dylib symlinks (they are not needed by binaries previously linked to the system OpenSSL) at the same time they stopped distributing the OpenSSL headers, everything would have been easier and we might not even have this discussion.

I'm hoping they'll eventually move the remaining OpenSSL libraries to a private prefix inside lib, as they did initially for LibreSSL when that was introduced, or cut out the unversioned symlinks as they already do with LibreSSL's libraries & we can stop hiding away our OpenSSL, but good luck to anyone trying to get progress on that front 😕.

@bartonjs
Copy link

Alright, we seem to have come up with our immediate workaround. Sorry for the derailment.

OpenSSL 1.1.0 is due to land at some point & eventually will take the openssl name in all likelihood

Yeah, we're watching for that. I don't suppose that any of the package management-style systems are looking too fondly at the non-backwards-compatible release.

... if Apple had just removed the /usr/lib/libcrypto.dylib and /usr/lib/libssl.dylib symlinks ...

Wouldn't that've been grand?

That you had to use the --force option should have made you think.

It had a clear point of confusion/contention in 10.10, but on 10.11 it seemed to not (however, that seems to have been an incorrect conclusion). Since 10.11 was our minimum MacOS we followed the prevailing "how do I make this work?" advice from the internet. Though, clearly, "prevailing" isn't guaranteed "right".

because there was no contact information

It's (sort of) there, but I can see how it would get missed/not-understood. There's a Community link in the header which then lists all of the Github projects which feed into the effort; but it doesn't have an obvious "if you're trying to give us advanced warning" focus, which I'll pass on.

you should really consider bundling OpenSSL as a short-term solution and then port your SSL/TLS layer to use Secure Transport on macOS as long-term solution

FWIW, that is the long-term plan. Situations like this one certainly remind us of the importance of that effort.

BTW: If you hadn't noticed yet, some of your users are pretty clever at finding workarounds, and they found that homebrew/versions/openssl101 doesn't have the same restrictions. If you were aware of that vector, no worries. If you weren't, then I guess this paragraph is a weakly-filed issue.

@UniqMartin
Copy link
Contributor

Thanks for the detailed reply, for taking some action (e.g. more obvious contact information), and for understanding our point of view. This is much appreciated! ❤️

BTW: If you hadn't noticed yet, some of your users are pretty clever at finding workarounds, and they found that homebrew/versions/openssl101 doesn't have the same restrictions. If you were aware of that vector, no worries. If you weren't, then I guess this paragraph is a weakly-filed issue.

We noticed: #612.

I'm looking forward to the creative workarounds they will find next. Sadly, quite a few users (and I think they might be mostly your users and have installed Homebrew due to your instructions) will ignore all warnings and do the unsafe thing nonetheless, because they just want their toy to work and then move on. Spreading this bad advice, as has happened in a few of your GitHub issues and on StackOverflow, is unfortunate, but I guess there's nothing we can do about this.

MikeMcQuaid added a commit that referenced this pull request Jul 31, 2016
This extends the approach in #597 to further prevent linkage of formulae
that conflict with the system OpenSSL and can cause the issues
described in that issue.
@MikeMcQuaid
Copy link
Member Author

Since 10.11 was our minimum MacOS we followed the prevailing "how do I make this work?" advice from the internet. Though, clearly, "prevailing" isn't guaranteed "right".

I think this is mostly on us; Homebrew should (and, in this case, now does) actively stop people from doing things we know to be harmful rather than allowing them. Code is much easier to change than documentation here even if the impact isn't quite as pleasant, unfortunately.

It's (sort of) there, but I can see how it would get missed/not-understood. There's a Community link in the header which then lists all of the Github projects which feed into the effort; but it doesn't have an obvious "if you're trying to give us advanced warning" focus, which I'll pass on.

FWIW I was the one who looked at https://www.microsoft.com/net/core#macos, looked for a GitHub link, looked at https://github.com/dotnet/core/issues/new and didn't think any of those repos looked suitable to report to.

BTW: If you hadn't noticed yet, some of your users are pretty clever at finding workarounds, and they found that homebrew/versions/openssl101 doesn't have the same restrictions. If you were aware of that vector, no worries. If you weren't, then I guess this paragraph is a weakly-filed issue.

That's just been fixed as-of #612 being merged 😆

@bartonjs
Copy link

@MikeMcQuaid No worries. Either the prepopulated message there is new, or many people just erase it and post a new issue (possibly when they hit "uh.. which one?"). Honestly I can't even tell you which one I'd have thought the right answer is. dotnet/corefx is where the dependency comes from, but that's not the same as saying "the docs are bad".

But, in the end, we got it sorted out.

@MikeMcQuaid
Copy link
Member Author

Glad you got it sorted.

@talkingscott
Copy link

@bartonjs

Alright, we seem to have come up with our immediate workaround. Sorry for the derailment.

As someone who came to this discussion while trying to determine what to do when the instructions at https://www.microsoft.com/net/core#macos failed, I am wondering how and when I will learn of your immediate workaround. I'd rather wait for that than do something insecure with openssl.

@chenbojian
Copy link

So will the issue fixed by microsoft dotnet project?

@joshka
Copy link
Contributor

joshka commented Aug 1, 2016

@chenbojian @talkingscott you might want to follow https://github.com/dotnet/cli/issues/3964 rather than messing more with this on the Homebrew PR.

As an aside, a great way to keep developers happy and productive is to hit the subscribe button instead of asking when something will be fixed or commenting with a +1.

andyleejordan added a commit to PowerShell/PowerShell that referenced this pull request Aug 4, 2016
Due to Homebrew/brew#597,
.NET Core's installation instructions of `brew link --force openssl` are
now invalid (for good reasons, due to security holes). Until .NET Core
updates their libraries to find the OpenSSL libraries correctly, we can
update them as part of `Start-PSBootstrap`.

See https://github.com/dotnet/cli/issues/3964
@bartonjs
Copy link

bartonjs commented Aug 6, 2016

.NET Core for macOS users: In light of the Homebrew changes (and resultant/associated commentary) here we've increased the priority of the move off of OpenSSL on macOS. https://github.com/dotnet/corefx/issues/9394 is the main issue that will be tracking the incremental progress (which already has one linked PR), and any discussion should be there (rather than over here on the Homebrew side of things).

(Sorry for the somewhat-of-a-cross-post, but wanted to leave breadcrumbs for any latecomers)

@dorlandode
Copy link

dorlandode commented Oct 12, 2016

I ran the installer as Microsoft suggested.

brew update
brew install openssl
ln -s /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib /usr/local/lib/
ln -s /usr/local/opt/openssl/lib/libssl.1.0.0.dylib /usr/local/lib/

I had to add 2 symlinks.

1.) ln -s /usr/local/Cellar/openssl/1.0.2j/bin/openssl openssl
2.) ln -s /usr/local/share/dotnet/dotnet /usr/local/bin/dotnet

The first symlink made it so openssl version showed the right version.
The second one fixes dotnet missing from the path.

The installer still complains about the openssl version, but appears to have worked.
It is possible the installer didn't run properly since it didn't create the dotnet symlink.

@Homebrew Homebrew locked and limited conversation to collaborators Oct 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants