1770484 - Make Mac child processes not depend on DYLD_LIBRARY_PATH to load libraries

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Haik Aftandilian [:haik]]

In order to enable a security hardening feature (bug 1562756) that prevents injection of libraries using dyld environment variables such as DYLD_INSERT_LIBRARIES, we need to change how Mac child processes (which use the plugin-container executable) load libraries.

Today, we launch child processes with DYLD_LIBRARY_PATH set to the directory containing the firefox executable. Without DYLD_LIBRARY_PATH, plugin-container can't load XUL, libnss3.dylib, or other dylibs in the main bundle directory because plugin-container's load paths use @executable_path/<dylib> when then need to use @executable_path/../../../<dylib>.

And the dylibs themselves that have dependent dylibs also use @executable_path. This needs to be changed to use @loader_path.

Directory structure:

$ find Firefox.app -type f |grep -E '(dylib$|XUL$|firefox$|plugin-container$)'
Firefox.app/Contents/MacOS/firefox
Firefox.app/Contents/MacOS/libfreebl3.dylib
Firefox.app/Contents/MacOS/liblgpllibs.dylib
Firefox.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container
Firefox.app/Contents/MacOS/libsoftokn3.dylib
Firefox.app/Contents/MacOS/XUL
Firefox.app/Contents/MacOS/libosclientcerts.dylib
Firefox.app/Contents/MacOS/libmozavutil.dylib
Firefox.app/Contents/MacOS/libmozglue.dylib
Firefox.app/Contents/MacOS/libipcclientcerts.dylib
Firefox.app/Contents/MacOS/libmozavcodec.dylib
Firefox.app/Contents/MacOS/libnssckbi.dylib
Firefox.app/Contents/MacOS/libnss3.dylib
Firefox.app/Contents/Resources/gmp-clearkey/0.1/libclearkey.dylib

plugin-container loader paths: (needs changes to use @executable_path/../../../<dylib>)

$ otool -arch arm64 -L Firefox.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container 
Firefox.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container:
	@executable_path/libnss3.dylib (compatibility version 1.0.0, current version 1.0.0)
	@executable_path/XUL (compatibility version 1.0.0, current version 1.0.0)
	@executable_path/libmozglue.dylib (compatibility version 1.0.0, current version 1.0.0)
	/usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 904.4.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.0.0)

libnss3.dylib loader paths: (needs changes to use @loader_path/<dylib>)

$ otool -arch arm64 -L Firefox.app/Contents/MacOS/libnss3.dylib 
Firefox.app/Contents/MacOS/libnss3.dylib:
	@executable_path/libnss3.dylib (compatibility version 1.0.0, current version 1.0.0)
	@executable_path/libmozglue.dylib (compatibility version 1.0.0, current version 1.0.0)
	/System/Library/Frameworks/CoreServices.framework/Versions/A/CoreServices (compatibility version 1.0.0, current version 1122.4.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.0.0)

Comment 1

[Haik Aftandilian [:haik]]

Attachment: Bug 1770484 - Make Mac processes not depend on DYLD_LIBRARY_PATH to load libraries r?glandium!,#mac-reviewers!,gsvelto!

Change XUL and other dylibs to be built with an @rpath/<dylib> install name (LC_ID_DYLIB) instead of @executable_path/<dylib>.

Change executables firefox, plugin-container, crashreporter, minidump-analyzer, pingsender, and xpcshell to be built with an @rpath dyld search path to allow loading of dylibs from the main/outer .app bundle without relying on DYLD_LIBRARY_PATH.

Previously, dylib install names were set as @executable_path/<dylib> allowing them to be resolved by dyld for the loading executable if the executable resided in the same directory as the dylib. For executables not in the same directory as the dylibs, dyld resolved these dylibs using DYLD_LIBRARY_PATH set before launching the process by Firefox code. With this change, loading does not rely on DYLD environment variables. Instead, dylibs have an install name set as @rpath/<dylib> and each executable loading a dylib has its @rpath set at compile-time to refer to dylib directory.

Comment 2

[Haik Aftandilian [:haik]]

When writing up the bug description, I thought the best way to fix this would be using @loader_path. I settled on using @rpath instead. Using @rpath seems like a more conventional way to setup dylib dependencies. Using @loader_path would requires extra steps in the build to run install_name_tool -- for example after linking plugin-container with libnss3.dylib where libnss3.dylib uses @loader_path for its install name, we would have to run install_name_tool to change the search path from @loader_path/libnss3.dylib to @executable_path/../../../libnss3.dylib.

The current patches set the @rpath on executables to be relative to @executable_path because the path from the executable to the dylib should be stable. If the executables are moved from their relative location in the bundle, the dylib resolution will fail.

Comment 3

[Stephen A Pohl [:spohl]]

To put this into real-world context, aside from being able to enable a new security hardening feature, the patch here will also address bugs such as bug 1771943. It may also be a factor in addressing bug 1766258, but this will need to be confirmed by the reporter first. Thanks, Haik, for putting this together!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bc4ee894015
Make Mac processes not depend on DYLD_LIBRARY_PATH to load libraries r=glandium,gsvelto,mac-reviewers,necko-reviewers,dragana,spohl

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/1bc4ee894015

Comment 6

[Andrej (:aglavic)]

== Change summary for alert #34335 (as of Tue, 07 Jun 2022 17:08:04 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s fission stylo webrender	108.08 -> 100.25
7%	cpstartup content-process-startup	macosx1015-64-shippable-qr	e10s fission stylo webrender	107.04 -> 100.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34335

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this something we may want on ESR102 eventually or is this edge-case enough that we can live with it? My main reason for asking is bug 1771943. I'm fine with letting it bake if we don't want to uplift to Beta102 in case that matters.

Comment 8

[Haik Aftandilian [:haik]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)

Is this something we may want on ESR102 eventually or is this edge-case enough that we can live with it? My main reason for asking is bug 1771943. I'm fine with letting it bake if we don't want to uplift to Beta102 in case that matters.

I think this would be good to get into ESR: it fixes the slash in directory name issue (bug 1771943) which is a corner case with a very bad user experience, it provides a small perf win for content process startup time (see comment 6), and if we pull in the blocker bug 1562756 we get a security benefit.

But I would prefer not to uplift to beta because the change affected linking for all our shipping executables and dylibs and there's elevated risk with such a change.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Thanks for the info. Putting this on the radar for a future ESR102 release after it's had some bake time. We'll check back later :-)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Circling back here, we're going to RC next week. Did you want to let this bake on Release for a cycle before we consider it for ESR102 or is most of a Beta cycle sufficient?

Comment 11

[Haik Aftandilian [:haik]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

Circling back here, we're going to RC next week. Did you want to let this bake on Release for a cycle before we consider it for ESR102 or is most of a Beta cycle sufficient?

I think the Beta cycle time it's had is sufficient and it can go to the ESR build.

Comment 12

[Haik Aftandilian [:haik]]

Comment on attachment 9278232 [details]
Bug 1770484 - Make Mac processes not depend on DYLD_LIBRARY_PATH to load libraries r?glandium!,#mac-reviewers!,gsvelto!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: It addresses bug 1771943 which is a corner-case with a very bad user experience, it provides a content process startup time improvement, and with 1562756 provides a security/hardening benefit.

The fix for 1562756 depends on this fix and should be included too.

• User impact if declined: We wouldn't be able to include bug 1562756 which has security benefits.
• Fix Landed on Version: 103
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change affects how Firefox launches child processes and how dylibs are resolved by Firefox and its supporting executables. There is some risk of a breakage, but with the time this has received on Beta, the test coverage should be sufficient having including crash reporter and updater.

The changes to the browser are limited to removing the use of DYLD_LIBRARY_PATH to launch child processes and the other changes are related to linkage.

Comment 13

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

Comment on attachment 9278232 [details]
Bug 1770484 - Make Mac processes not depend on DYLD_LIBRARY_PATH to load libraries r?glandium!,#mac-reviewers!,gsvelto!

Approved for ESR102.1, thanks.

Comment 14

[Donal Meehan [:dmeehan - PTO - Back Oct 13]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/dd4d355d7f5f