fix: resolve libraries ldd reports as missing instead of dropping them - #19
fix: resolve libraries ldd reports as missing instead of dropping them#19dviejokfs wants to merge 4 commits into
Conversation
📓 Changelog previewThis is what your commits will add to the generated ## [Unreleased]
### Fixed
- Resolve libraries ldd reports as missing instead of dropping them
- Validate sonames and widen the library anchor
- Keep pathname expansion on for the ldd argument
- Escape regex metacharacters in a soname |
|
Marking this draft — a security review found blockers, two of which I have reproduced. The anchor is wrong, and it fails buildsI anchored to Any of those arriving as Why I missed it: I only tested The regex interpolation is injectable
That package name is written to the record file and installed as root into the runtime image, with the build exiting 0. A hostile transitive dependency can set it — a Crystal shard with Shell injection specifically is not present — Also flagged
PlanValidate sonames against The test I wrote pins the vulnerable pattern as a string literal and would not have caught any of this. It needs replacing with one that runs the generated script against a fixture binary carrying a hostile |
record_runtime_libraries filtered ldd output with /=> \//, which only
matches lines where the loader resolved the library to a path. A missing
one looks like
libatomic.so.1 => not found
with no path, so it was silently dropped: the recorded list came out
short and the runtime image was built without the package, with no error
anywhere. The helper exists precisely to avoid hardcoding package names,
and this is the case where it quietly stopped doing that.
Resolve those with apt-file, which maps a filename to the package
providing it without needing the file present. The query is anchored to
the multiarch library directory deliberately — a bare basename search for
libatomic.so.1 also matches lib32atomic1 and the -cross packages, and
libnss3.so matches firefox-esr and thunderbird, so an unanchored head -1
installs something wildly wrong. A library nothing provides now fails the
build with an explanation rather than producing an image that dies with
a loader error.
apt-file and its ~90MB index are only fetched when something is actually
missing.
Refs #14
Five problems a security review found in the previous commit. A soname reaches this from a binary the app produced, and it was interpolated raw into an extended regex. `|` has the lowest precedence, so DT_NEEDED of `x|usr/sbin/sshd` escaped the path anchor entirely, resolved to openssh-server, and had it installed as root in the runtime image with the build exiting 0. Validate the soname first. The anchor only covered /usr/lib/<triplet>/. On Debian the essential libraries are still recorded unmerged, so libz.so.1, libc.so.6, libgcc_s.so.1 and libtinfo.so.6 all resolved to nothing and failed the build — the opposite of the point. It also missed non-gnu triplets like arm-linux-gnueabihf. `for lib in $missing` was unquoted, so a soname of `*` expanded against the build directory. `set -f`. `head -1` was locale-dependent: the same source picked libavcodec-extra59 under LC_ALL=C and libavcodec59 under en_US.UTF-8. Pin the collation, and stop guessing — 24 sonames in bookworm have several providers, and for libc++.so.1 the first-sorted answer is the oldest ABI. Fail with the candidates and tell the user to pick one. The test pinned the vulnerable pattern as a literal and would not have caught any of this; it now asserts the guarantees instead.
|
Pushed Attacked the generated script directlyFixture ELF binaries built with No What changed
The testReplaced. The old one pinned the vulnerable regex as a string literal and would have caught none of this. It now asserts the guarantees — validation present, anchor covers The stronger evidence is the table above; that harness is worth landing as a fixture-based test rather than living in my shell history, and I would rather do that than claim the string assertions are sufficient. Not fixed here, flagged for follow-up: Conformance green on |
34b2194 to
c9462c0
Compare
set -f at the top of the script disabled globbing everywhere, including the line whose argument is documented as a glob. No current caller passes one — cobol and crystal name a single binary — but the PHP provider's own copy globs an extension directory, and a caller that inspects several binaries would have silently got no results. Turn expansion off only around the loop over sonames, which is the one place a crafted DT_NEEDED could expand against the build directory.
The character guard rejects what a library name cannot contain, but two characters it legitimately can are also regex metacharacters, and both were spliced raw into the apt-file query. `+` is a quantifier, so libxml++-2.6.so.2 matched nothing and the build failed claiming no package provides a library that plainly exists. 238 sonames in bookworm carry a + — libFLAC++, libMagick++, libIce++11. Escaped, they resolve: libxml++2.6-2v5, libflac++10. `.` matches any character including /, so a crafted DT_NEEDED of gio.modules.libgioremote-volume-monitor.so walked two directories below the anchor and reached gvfs, pulling 215 packages into the runtime image as root. A malicious transitive dependency can rewrite DT_NEEDED on a binary under /app, so this was reachable without the app author's knowledge. Escaped, it resolves to nothing and the build stops. Escaping both collapses the query to an exact basename match.
1 participant
Refs #14.
The bug
record_runtime_librariesfilteredlddoutput withawk '/=> \// { print $3 }'. That only matches lines where the loader resolved the library to a path. A missing one has no path:so it was silently dropped. The recorded list came out short, the runtime image was built without the package, and nothing errored. The helper exists precisely so that package names are not hardcoded — its own doc comment argues the case, citing the
t64transition — and this is the case where it quietly stopped doing that.It is also why neither of this month's runtime failures could be caught generically, and why both were fixed with hardcoded Debian package names instead:
libatomicfor pnpm 11 (#1) and the 15-package Chromium closure for Puppeteer/Playwright (#11).The fix
Resolve the
not foundshape withapt-file, which maps a filename to its providing package without needing the file present.Verified end to end against a binary that genuinely has a missing library — mise-installed pnpm 11, in
debian:bookworm-slim:libatomic1is exactly the package #1 had to hardcode.The part worth reviewing
The query is anchored to the multiarch directory, and it has to be. My first attempt searched the bare basename, and the results were actively dangerous:
An unanchored
head -1installs the 32-bit cross-compile package for one, and an entire web browser for the other. Anchoring to^/usr/lib/[a-z0-9_]*-linux-gnu/gives exactly one correct answer each:The pattern is arch-agnostic rather than using
dpkg-architecture, which is indpkg-devand is not present in the build image — confirmed.Other choices:
apt-fileand its index are fetched only when something is missing. The index is ~90MB and takes ~12s; builds with nothing unresolved should not pay for it.Test plan
bookworm-slim, output abovemissing_libraries_are_resolved_rather_than_dropped— asserts both branches, the anchored pattern, the failure path, and that the index fetch is behind the guardcrystal-serverandcobol-apppass,php-app/php-extensionscovered in CIcargo test --workspace→ 230 passedcargo fmt --all -- --checkandcargo clippy --workspace --all-targets -- -D warningscleanFollow-up, not in this PR
This makes the mechanism correct but does not yet migrate the two hardcoded lists onto it. Moving
libatomicandCHROMIUM_RUNTIMEover would close #15 as well, since the whole point is thatapt-fileanswers per release and survives a trixie bump. Both want their own change and their own verification — the Chromium set in particular is only reachable after the browser is downloaded, so thelddglob has to be pointed at it deliberately.