Merge #481

481: Do not let modifyRPath taint shared strings in strtab. Fix #315 r=Mic92 a=brenoguim



Co-authored-by: Breno Rodrigues Guimaraes <brenorg@gmail.com>

5 files changed, 155 insertions, 13 deletions

diff --git a/src/patchelf.cc b/src/patchelf.cc
index 1a90edb..ee00918 100644
--- a/src/patchelf.cc
+++ b/src/patchelf.cc
@@ -664,14 +664,14 @@ template<ElfFileParams>
 void ElfFile<ElfFileParamNames>::writeReplacedSections(Elf_Off & curOff,
     Elf_Addr startAddr, Elf_Off startOffset)
 {
-    /* Overwrite the old section contents with 'X's.  Do this
+    /* Overwrite the old section contents with 'Z's.  Do this
        *before* writing the new section contents (below) to prevent
        clobbering previously written new section contents. */
     for (auto & i : replacedSections) {
         const std::string & sectionName = i.first;
         const Elf_Shdr & shdr = findSectionHeader(sectionName);
         if (rdi(shdr.sh_type) != SHT_NOBITS)
-            memset(fileContents->data() + rdi(shdr.sh_offset), 'X', rdi(shdr.sh_size));
+            memset(fileContents->data() + rdi(shdr.sh_offset), 'Z', rdi(shdr.sh_size));
     }
 
     std::set<unsigned int> noted_phdrs = {};
@@ -1538,7 +1538,7 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
 
     /* !!! We assume that the virtual address in the DT_STRTAB entry
        of the dynamic section corresponds to the .dynstr section. */
-    auto shdrDynStr = findSectionHeader(".dynstr");
+    auto& shdrDynStr = findSectionHeader(".dynstr");
     char * strTab = (char *) fileContents->data() + rdi(shdrDynStr.sh_offset);
 
 
@@ -1621,24 +1621,39 @@ void ElfFile<ElfFileParamNames>::modifyRPath(RPathOp op,
     }
     changed = true;
 
-    /* Zero out the previous rpath to prevent retained dependencies in
-       Nix. */
+    bool rpathStrShared = false;
     size_t rpathSize = 0;
     if (rpath) {
-        rpathSize = strlen(rpath);
+        std::string_view rpathView {rpath};
+        rpathSize = rpathView.size();
+
+        size_t rpathStrReferences = 0;
+        forAllStringReferences(shdrDynStr, [&] (auto refIdx) {
+            if (rpathView.end() == std::string_view(strTab + rdi(refIdx)).end())
+                rpathStrReferences++;
+        });
+        assert(rpathStrReferences >= 1);
+        debug("Number of rpath references: %lu\n", rpathStrReferences);
+        rpathStrShared = rpathStrReferences > 1;
+    }
+
+    /* Zero out the previous rpath to prevent retained dependencies in
+       Nix. */
+    if (rpath && !rpathStrShared) {
+        debug("Tainting old rpath with Xs\n");
         memset(rpath, 'X', rpathSize);
     }
 
     debug("new rpath is '%s'\n", newRPath.c_str());
 
 
-    if (newRPath.size() <= rpathSize) {
+    if (!rpathStrShared && newRPath.size() <= rpathSize) {
         if (rpath) memcpy(rpath, newRPath.c_str(), newRPath.size() + 1);
         return;
     }
 
     /* Grow the .dynstr section to make room for the new RPATH. */
-    debug("rpath is too long, resizing...\n");
+    debug("rpath is too long or shared, resizing...\n");
 
     std::string & newDynStr = replaceSection(".dynstr",
         rdi(shdrDynStr.sh_size) + newRPath.size() + 1);
@@ -2293,6 +2308,42 @@ void ElfFile<ElfFileParamNames>::modifyExecstack(ExecstackMode op)
     printf("execstack: %c\n", result);
 }
 
+template<ElfFileParams>
+template<class StrIdxCallback>
+void ElfFile<ElfFileParamNames>::forAllStringReferences(const Elf_Shdr& strTabHdr, StrIdxCallback&& fn)
+{
+    for (auto& sym : tryGetSectionSpan<Elf_Sym>(".dynsym"))
+        fn(sym.st_name);
+
+    for (auto& dyn : tryGetSectionSpan<Elf_Dyn>(".dynamic"))
+        switch (rdi(dyn.d_tag))
+        {
+            case DT_NEEDED:
+            case DT_SONAME:
+            case DT_RPATH:
+            case DT_RUNPATH: fn(dyn.d_un.d_val);
+            default:;
+        }
+
+    if (auto verdHdr = tryFindSectionHeader(".gnu.version_d"))
+    {
+        if (&shdrs.at(rdi(verdHdr->get().sh_link)) == &strTabHdr)
+            forAll_ElfVer(getSectionSpan<Elf_Verdef>(*verdHdr),
+                [] (auto& /*vd*/) {},
+                [&] (auto& vda) { fn(vda.vda_name); }
+            );
+    }
+
+    if (auto vernHdr = tryFindSectionHeader(".gnu.version_r"))
+    {
+        if (&shdrs.at(rdi(vernHdr->get().sh_link)) == &strTabHdr)
+            forAll_ElfVer(getSectionSpan<Elf_Verneed>(*vernHdr),
+                [&] (auto& vn) { fn(vn.vn_file); },
+                [&] (auto& vna) { fn(vna.vna_name); }
+            );
+    }
+}
+
 static bool printInterpreter = false;
 static bool printOsAbi = false;
 static bool setOsAbi = false;
@@ -2397,9 +2448,9 @@ static void patchElf()
         const std::string & outputFileName2 = outputFileName.empty() ? fileName : outputFileName;
 
         if (getElfType(fileContents).is32Bit)
-            patchElf2(ElfFile<Elf32_Ehdr, Elf32_Phdr, Elf32_Shdr, Elf32_Addr, Elf32_Off, Elf32_Dyn, Elf32_Sym, Elf32_Verneed, Elf32_Versym, Elf32_Rel, Elf32_Rela, 32>(fileContents), fileContents, outputFileName2);
+            patchElf2(ElfFile<Elf32_Ehdr, Elf32_Phdr, Elf32_Shdr, Elf32_Addr, Elf32_Off, Elf32_Dyn, Elf32_Sym, Elf32_Versym, Elf32_Verdef, Elf32_Verdaux, Elf32_Verneed, Elf32_Vernaux, Elf32_Rel, Elf32_Rela, 32>(fileContents), fileContents, outputFileName2);
         else
-            patchElf2(ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, Elf64_Addr, Elf64_Off, Elf64_Dyn, Elf64_Sym, Elf64_Verneed, Elf64_Versym, Elf64_Rel, Elf64_Rela, 64>(fileContents), fileContents, outputFileName2);
+            patchElf2(ElfFile<Elf64_Ehdr, Elf64_Phdr, Elf64_Shdr, Elf64_Addr, Elf64_Off, Elf64_Dyn, Elf64_Sym, Elf64_Versym, Elf64_Verdef, Elf64_Verdaux, Elf64_Verneed, Elf64_Vernaux, Elf64_Rel, Elf64_Rela, 64>(fileContents), fileContents, outputFileName2);
     }
 }
 
diff --git a/src/patchelf.h b/src/patchelf.h
index 9fab18c..4e229d6 100644
--- a/src/patchelf.h
+++ b/src/patchelf.h
@@ -10,8 +10,8 @@
 
 using FileContents = std::shared_ptr<std::vector<unsigned char>>;
 
-#define ElfFileParams class Elf_Ehdr, class Elf_Phdr, class Elf_Shdr, class Elf_Addr, class Elf_Off, class Elf_Dyn, class Elf_Sym, class Elf_Verneed, class Elf_Versym, class Elf_Rel, class Elf_Rela, unsigned ElfClass
-#define ElfFileParamNames Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Verneed, Elf_Versym, Elf_Rel, Elf_Rela, ElfClass
+#define ElfFileParams class Elf_Ehdr, class Elf_Phdr, class Elf_Shdr, class Elf_Addr, class Elf_Off, class Elf_Dyn, class Elf_Sym, class Elf_Versym, class Elf_Verdef, class Elf_Verdaux, class Elf_Verneed, class Elf_Vernaux, class Elf_Rel, class Elf_Rela, unsigned ElfClass
+#define ElfFileParamNames Elf_Ehdr, Elf_Phdr, Elf_Shdr, Elf_Addr, Elf_Off, Elf_Dyn, Elf_Sym, Elf_Versym, Elf_Verdef, Elf_Verdaux, Elf_Verneed, Elf_Vernaux, Elf_Rel, Elf_Rela, ElfClass
 
 template<class T>
 struct span
@@ -237,6 +237,40 @@ private:
         }
     }
 
+    template<class StrIdxCallback>
+    void forAllStringReferences(const Elf_Shdr& strTabHdr, StrIdxCallback&& fn);
+
+    template<class T, class U>
+    auto follow(U* ptr, size_t offset) -> T* {
+        return offset ? (T*)(((char*)ptr)+offset) : nullptr;
+    };
+
+    template<class VdFn, class VaFn>
+    void forAll_ElfVer(span<Elf_Verdef> vdspan, VdFn&& vdfn, VaFn&& vafn)
+    {
+        auto* vd = vdspan.begin();
+        for (; vd; vd = follow<Elf_Verdef>(vd, rdi(vd->vd_next)))
+        {
+            vdfn(*vd);
+            auto va = follow<Elf_Verdaux>(vd, rdi(vd->vd_aux));
+            for (; va; va = follow<Elf_Verdaux>(va, rdi(va->vda_next)))
+                vafn(*va);
+        }
+    }
+
+    template<class VnFn, class VaFn>
+    void forAll_ElfVer(span<Elf_Verneed> vnspan, VnFn&& vnfn, VaFn&& vafn)
+    {
+        auto* vn = vnspan.begin();
+        for (; vn; vn = follow<Elf_Verneed>(vn, rdi(vn->vn_next)))
+        {
+            vnfn(*vn);
+            auto va = follow<Elf_Vernaux>(vn, rdi(vn->vn_aux));
+            for (; va; va = follow<Elf_Vernaux>(va, rdi(va->vna_next)))
+                vafn(*va);
+        }
+    }
+
     /* Convert an integer in big or little endian representation (as
        specified by the ELF header) to this platform's integer
        representation. */
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0b648f8..5dba804 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -49,6 +49,7 @@ src_TESTS = \
   modify-execstack.sh \
   rename-dynamic-symbols.sh \
   overlapping-segments-after-rounding.sh \
+  shared-rpath.sh \
   empty-note.sh
 
 build_TESTS = \
@@ -121,7 +122,7 @@ check_DATA = libbig-dynstr.debug
 # - with libtool, it is difficult to control options
 # - with libtool, it is not possible to compile convenience *dynamic* libraries :-(
 check_PROGRAMS += libfoo.so libfoo-scoped.so libbar.so libbar-scoped.so libsimple.so libsimple-execstack.so libbuildid.so libtoomanystrtab.so \
-                  phdr-corruption.so many-syms-main libmany-syms.so liboveralign.so
+                  phdr-corruption.so many-syms-main libmany-syms.so liboveralign.so libshared-rpath.so
 
 libbuildid_so_SOURCES = simple.c
 libbuildid_so_LDFLAGS = $(LDFLAGS_sharedlib) -Wl,--build-id
@@ -148,6 +149,9 @@ libsimple_so_LDFLAGS = $(LDFLAGS_sharedlib) -Wl,-z,noexecstack
 liboveralign_so_SOURCES = simple.c
 liboveralign_so_LDFLAGS = $(LDFLAGS_sharedlib) -Wl,-z,max-page-size=0x10000
 
+libshared_rpath_so_SOURCES = shared-rpath.c
+libshared_rpath_so_LDFLAGS = $(LDFLAGS_sharedlib) -Wl,-rpath=a_symbol_name
+
 libsimple_execstack_so_SOURCES = simple.c
 libsimple_execstack_so_LDFLAGS = $(LDFLAGS_sharedlib) -Wl,-z,execstack
 
diff --git a/tests/shared-rpath.c b/tests/shared-rpath.c
new file mode 100644
index 0000000..8a3ee7c
--- /dev/null
+++ b/tests/shared-rpath.c
@@ -0,0 +1,2 @@
+int a_symbol_name;
+int foo() { return a_symbol_name; }
diff --git a/tests/shared-rpath.sh b/tests/shared-rpath.sh
new file mode 100755
index 0000000..8e6e26f
--- /dev/null
+++ b/tests/shared-rpath.sh
@@ -0,0 +1,51 @@
+#! /bin/sh -e
+
+PATCHELF=$(readlink -f "../src/patchelf")
+SCRATCH="scratch/$(basename "$0" .sh)"
+READELF=${READELF:-readelf}
+
+LIB_NAME="${PWD}/libshared-rpath.so"
+
+rm -rf "${SCRATCH}"
+mkdir -p "${SCRATCH}"
+cd "${SCRATCH}"
+
+has_x() {
+    strings "$1" | grep -c "XXXXXXXX"
+}
+
+nm -D "${LIB_NAME}" | grep a_symbol_name
+previous_cnt="$(strings "${LIB_NAME}" | grep -c a_symbol_name)"
+
+echo "#### Number of a_symbol_name strings in the library: $previous_cnt"
+
+echo "#### Rename the rpath to something larger than the original"
+# Pathelf should detect that the rpath string is shared with the symbol name string and avoid
+# tainting the string with Xs
+"${PATCHELF}" --set-rpath a_very_big_rpath_that_is_larger_than_original  --output liblarge-rpath.so "${LIB_NAME}"
+
+echo "#### Checking symbol is still there"
+nm -D liblarge-rpath.so | grep a_symbol_name
+
+echo "#### Checking there are no Xs"
+[ "$(has_x liblarge-rpath.so)" -eq 0 ] || exit 1
+
+current_cnt="$(strings liblarge-rpath.so | grep -c a_symbol_name)"
+echo "#### Number of a_symbol_name strings in the modified library: $current_cnt"
+[ "$current_cnt" -eq "$previous_cnt" ] || exit 1
+
+echo "#### Rename the rpath to something shorter than the original"
+# Pathelf should detect that the rpath string is shared with the symbol name string and avoid
+# overwriting the existing string
+"${PATCHELF}" --set-rpath shrt_rpth  --output libshort-rpath.so "${LIB_NAME}"
+
+echo "#### Checking symbol is still there"
+nm -D libshort-rpath.so | grep a_symbol_name
+
+echo "#### Number of a_symbol_name strings in the modified library: $current_cnt"
+current_cnt="$(strings libshort-rpath.so | grep -c a_symbol_name)"
+[ "$current_cnt" -eq "$previous_cnt" ] || exit 1
+
+echo "#### Now liblarge-rpath.so should have its own rpath, so it should be allowed to taint it"
+"${PATCHELF}" --set-rpath a_very_big_rpath_that_is_larger_than_original__even_larger  --output liblarge-rpath2.so liblarge-rpath.so
+[ "$(has_x liblarge-rpath2.so)" -eq 1 ] || exit 1