Hi all.
I know we're all busy people, but I'm hoping I can impose on someone with the
necessary rights to take a look at this, as it's been in limbo for several months
now.
Basically, I discovered a fairly nasty bug in chrpath — a package for which upstream has
vanished, so we're pretty much on our own — which fortunately only occurs under rare
circumstances. "Fortunately" because, when those circumstances arise and it DOES
occur, using chrpath to set the RUNPATH for a shared library can instead corrupt
(silently!) the library's dynamic string tables. While failing to actually modify the
RUNPATH.
My bug report about the issue, with steps to reproduce, is here:
https://bugzilla.redhat.com/show_bug.cgi?id=2022931
A few months after reporting, I revisited the problem, isolated the bug, and fixed (at
least in my testing) chrpath to no longer exhibit that buggy behavior. I submitted the
patch as a PR against our chrpath package repo, since the upstream URL points to an
unreachable host.
That PR can be found here:
https://src.fedoraproject.org/rpms/chrpath/pull-request/6
It's been open for five weeks now. As I said, the conditions that trigger the bug are
fairly rare, so I wouldn't say it's URGENT per se. But it does feel like it should
be addressed, and I even have a patch that (I believe) does so. Perhaps someone has a few
minutes to look it over, verify that I haven't done anything stupid, and help move the
process along? (Or provide some guidance on how I should go about doing so.)
Thanks!
(Everything past this point is additional detail for the morbidly curious. Bail out now if
you don't want to end up getting bored to tears with very tedious details, or
don't say I didn't warn you.)
Those fortunately-rare trigger conditions
---------------------------------------------------
The library being modified by chrpath will only be corrupted if its .dynstr section is
located at an unexpected location in the ELF header. chrpath blindly assumes that the
first section of type SHT_STRTAB it encounters when walking the headers must be the
.dynstr table, into which the RUNPATH is to be placed at the offset 'rpathoff'.
The assumption chrpath makes APPEARS to be a correct one, 99% of the time. For all
executables, and most if not all "normal" libraries, the header is laid out as
it assumes and it can update RUNPATH strings without incident.
However, should that assumption turn out to be INCORRECT, chrpath will insert a RUNPATH
string into the **wrong** table. Whichever section of type SHT_STRTAB it encounters first,
that table gets the RUNPATH inserted into it. When it's not the .dynstr table, some
other symbol name gets partly or completely overwritten with the new RUNPATH string.
Meanwhile, the library's *actual* RUNPATH doesn't change, since the real, correct
location in the .dynstr section still has the same value it did before running chrpath.
One of the ways a library with unexpected table ordering can be created is the series of
events that led me to initially discover the bug: Using a different library-editing tool,
patchelf, to insert a new RUNPATH into a library that previously lacked one, then
subsequently running chrpath on that library.
(Adding a RUNPATH to a library is something patchelf can do that chrpath cannot. chrpath
is limited to overwriting existing RUNPATH strings with one of the same length or shorter,
using space that's already allocated in th header. It has no ability to allocate
additional space for a RUNPATH where none previously existed. patchelf *can* carve out
space for a RUNPATH string in a library without one — it rewrites the ELF headers to
allocate additional space for the new string. In the process of doing so, it seems to
relocate (or create?) the dynamic string (.dynstr) table in a header section located
*after* the other, previously-allocated sections.)
So, when allocating new space to add a RUNPATH, patchelf places .dynstr somewhere that
chrpath assumes it will never be located. A patchelf-modified shared library is
effectively a trap waiting to be sprung by an unpatched chrpath, which will make a mess of
its string tables the moment it tries to modify them.
My proposed fix, as implemented in the submitted PR
-------------------------------------------------------------------
My patch is as simple as I could make it, and no simpler. It adds two functions to
chrpath:
read_section_names() retrieves the names for all of the sections from the file's ELF
header. The table contains the name for each section, located at an offset that's
stored in the .sh_name member of the section header struct.
get_section_name() takes a .sh_name value, and returns the corresponding string located at
that offset in the section names table.
Patched chrpath still walks the ELF headers to find the dynamic string table, when it
needs to write a new RUNPATH into a binary. But instead of stopping at the first
SHT_STRTAB section it finds and assuming that's the dynamic string table, the loop now
calls get_section_name(sh_name) and checks that the string returned is exactly
".dynstr". If it's not, the loop continues on. Only when both the section
type AND name are a match, will it proceed to write the new RUNPATH into the table.