On Mon, Jun 06, 2022 at 03:01:34PM -0000, Frank R Dana Jr. wrote: > 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.
Thanks for the detail and history here. I played with this a bit locally and did hit the same problem you have encountered. What I don't understand is why you can't use patchelf here instead of chrpath. The only mention I found was combining the use of chrpath and patchelf on an ELF object can lead to bad things. You mention modifications to the DT_RUNPATH and with the exception of --delete-rpath, I think that patchelf only modifies DT_RPATH and not DT_RUNPATH. I did not verify this though. I ask this because I used to use chrpath until upstream dried up and migrated to using patchelf. In my cases, I am fine with modifications to DT_RPATH. You may have a different set of circumstances. Since chrpath is no longer active upstream, submitting a PR for patchelf may be the better long term way to go. -- David Cantrell <dcantr...@redhat.com> Red Hat, Inc. | Boston, MA | EST5EDT _______________________________________________ devel mailing list -- devel@lists.fedoraproject.org To unsubscribe send an email to devel-le...@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure
