Re: [PATCH] chrpath: Bad assumption can corrupt shared libraries when setting RUNPATH

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.