Skip to content

Commit

Permalink
linkage_checker: replace Fiddle.dlopen with libSystem call
Browse files Browse the repository at this point in the history
`dlopen`ing a library executes potentially untrusted code (e.g. if the
library has initialisers). We can avoid the `dlopen` call by asking
`libSystem` directly about whether a library can be found in the shared
cache.

Of course, the `dlopen` happens after a `ENOENT`, so the attack surface here
is relatively small. But relying on this still exposes us to a potential
TOCTOU[^1] bug. Let's avoid it entirely by skipping `dlopen` altogether.

Also: add RBI for `Fiddle` constants used in `linkage_checker`

Upstream don't have these definitions yet, so I've added an RBI for them
in the meantime.

[^1]: https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use
  • Loading branch information
carlocab committed Oct 3, 2024
1 parent 3994768 commit d77c039
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 7 deletions.
21 changes: 14 additions & 7 deletions Library/Homebrew/linkage_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ def check_dylibs(rebuild_cache:)

if (dep = dylib_to_dep(dylib))
@broken_deps[dep] |= [dylib]
elsif system_libraries_exist_in_cache? && dylib_found_via_dlopen(dylib)
elsif system_libraries_exist_in_cache? && dylib_found_in_shared_cache?(dylib)
# If we cannot associate the dylib with a dependency, then it may be a system library.
# If dlopen finds the dylib, then the linkage is not broken.
# Check the dylib shared cache for the library to verify this.
@system_dylibs << dylib
elsif !system_framework?(dylib) && !broken_dylibs_allowed?(file.to_s)
@broken_dylibs << dylib
Expand Down Expand Up @@ -195,11 +195,18 @@ def system_libraries_exist_in_cache?
end
alias generic_system_libraries_exist_in_cache? system_libraries_exist_in_cache?

def dylib_found_via_dlopen(dylib)
Fiddle.dlopen(dylib).close
true
rescue Fiddle::DLError
false
def dylib_found_in_shared_cache?(dylib)
@dyld_shared_cache_contains_path ||= begin
libc = Fiddle.dlopen("/usr/lib/libSystem.B.dylib")

Check warning on line 200 in Library/Homebrew/linkage_checker.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/linkage_checker.rb#L199-L200

Added lines #L199 - L200 were not covered by tests

Fiddle::Function.new(

Check warning on line 202 in Library/Homebrew/linkage_checker.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/linkage_checker.rb#L202

Added line #L202 was not covered by tests
libc["_dyld_shared_cache_contains_path"],
[Fiddle::TYPE_CONST_STRING],
Fiddle::TYPE_BOOL,
)
end

@dyld_shared_cache_contains_path.call(dylib)

Check warning on line 209 in Library/Homebrew/linkage_checker.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/linkage_checker.rb#L209

Added line #L209 was not covered by tests
end

def check_formula_deps
Expand Down
14 changes: 14 additions & 0 deletions Library/Homebrew/sorbet/rbi/upstream.rbi
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,17 @@

# This file contains temporary definitions for fixes that have
# been submitted upstream to https://github.com/sorbet/sorbet.

# Missing constants we use in `linkage_checker.rb`
# https://github.com/sorbet/sorbet/pull/8215
module Fiddle
# [`TYPE_BOOL`](https://github.com/ruby/fiddle/blob/v1.1.2/ext/fiddle/fiddle.h#L129)
#
# C type - bool
TYPE_BOOL = T.let(T.unsafe(nil).freeze, Integer)

# [`TYPE_CONST_STRING`](https://github.com/ruby/fiddle/blob/v1.1.2/ext/fiddle/fiddle.h#L128)
#
# C type - char\*
TYPE_CONST_STRING = T.let(T.unsafe(nil).freeze, Integer)
end

0 comments on commit d77c039

Please sign in to comment.