From 1dd96a76284f8aec9fb724b687282ede45637dd2 Mon Sep 17 00:00:00 2001 From: "Markus F.X.J. Oberhumer" Date: Mon, 23 Oct 2023 14:26:37 +0200 Subject: [PATCH] src/pefile: stricter reloc checks; cleanups --- .github/workflows/ci.yml | 4 +- .github/workflows/weekly-ci-cc-zigcc.yml | 4 +- CMakeLists.txt | 12 ++-- NEWS | 1 + src/pefile.cpp | 92 ++++++++++++++---------- src/pefile.h | 2 +- src/stub/Makefile | 2 +- src/stub/scripts/bin2h.py | 2 +- src/util/util.h | 5 +- src/util/xspan_fwd.h | 32 --------- src/version.h | 4 +- src/work.cpp | 14 ++-- 12 files changed, 83 insertions(+), 91 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2796075d..ba2ee3ab 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,8 +12,8 @@ env: CMAKE_REQUIRED_QUIET: OFF DEBIAN_FRONTEND: noninteractive UPX_CMAKE_BUILD_FLAGS: --verbose - # 2023-10-21 - ZIG_DIST_VERSION: 0.12.0-dev.1137+fbbccc9d5 + # 2023-10-22 + ZIG_DIST_VERSION: 0.12.0-dev.1153+45d7dfa83 jobs: job-rebuild-and-verify-stubs: diff --git a/.github/workflows/weekly-ci-cc-zigcc.yml b/.github/workflows/weekly-ci-cc-zigcc.yml index 53192236..ef146de6 100644 --- a/.github/workflows/weekly-ci-cc-zigcc.yml +++ b/.github/workflows/weekly-ci-cc-zigcc.yml @@ -10,8 +10,8 @@ on: env: CMAKE_REQUIRED_QUIET: OFF DEBIAN_FRONTEND: noninteractive - # 2023-10-21 - ZIG_DIST_VERSION: 0.12.0-dev.1137+fbbccc9d5 + # 2023-10-22 + ZIG_DIST_VERSION: 0.12.0-dev.1153+45d7dfa83 jobs: job-linux-zigcc: # uses cmake + make diff --git a/CMakeLists.txt b/CMakeLists.txt index 1c1e100a..6d84752d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -168,8 +168,8 @@ endif() #*********************************************************************** include(CheckCCompilerFlag) -include(CheckIncludeFile) include(CheckFunctionExists) +include(CheckIncludeFile) include(CheckStructHasMember) include(CheckSymbolExists) @@ -181,11 +181,11 @@ if(NOT DEFINED HAVE_UTIMENSAT) if(HAVE_UTIMENSAT_FUNCTION__) check_symbol_exists(utimensat "sys/types.h;fcntl.h;sys/stat.h" HAVE_UTIMENSAT_SYMBOL__) if(HAVE_UTIMENSAT_SYMBOL__) - CHECK_STRUCT_HAS_MEMBER("struct stat" st_mtim.tv_nsec "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC) - if (NOT HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC) - CHECK_STRUCT_HAS_MEMBER("struct stat" st_mtimespec.tv_nsec "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC) + CHECK_STRUCT_HAS_MEMBER("struct stat" "st_mtim.tv_nsec" "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC) + if(NOT HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC) + CHECK_STRUCT_HAS_MEMBER("struct stat" "st_mtimespec.tv_nsec" "sys/types.h;fcntl.h;sys/stat.h" HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC) endif() - if (HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC OR HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC) + if(HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC OR HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC) set(HAVE_UTIMENSAT 1) endif() endif() @@ -452,7 +452,7 @@ if(NOT UPX_CONFIG_DISABLE_ZSTD) endif() if(HAVE_UTIMENSAT) target_compile_definitions(${t} PRIVATE USE_UTIMENSAT=1) - if (HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC) + if(HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC) target_compile_definitions(${t} PRIVATE HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC=1) endif() endif() diff --git a/NEWS b/NEWS index 39b662ac..d576468d 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,7 @@ User visible changes for UPX Changes in 4.2.0 (XX XXX 2023): * disable macOS support until we fix compatibility with macOS 13+ + * win32/pe and win64/pe: stricter relocation checks; please report regressions * new option '--link' to preserve hard-links (Unix only; use with care) * add support for NO_COLOR env var; see https://no-color.org/ * bug fixes - see https://github.com/upx/upx/milestone/13 diff --git a/src/pefile.cpp b/src/pefile.cpp index cba67de1..9576d9f2 100644 --- a/src/pefile.cpp +++ b/src/pefile.cpp @@ -295,7 +295,7 @@ struct FixDeleter { // don't leak memory on exceptions }; } // namespace -static constexpr unsigned RELOC_BUF_OFFSET = 64 * 1024; +static constexpr unsigned RELOC_INPLACE_OFFSET = 64 * 1024; PeFile::Reloc::~Reloc() noexcept { COMPILE_TIME_ASSERT(sizeof(BaseReloc) == 8) @@ -318,8 +318,6 @@ PeFile::Reloc::Reloc(byte *s, unsigned si) : start(s), start_size_in_bytes(si) { if (x == 0 || x == BRS) // ignore strange empty relocs return; } - if (si < BRS + sizeof(LE16)) // 10 - throwCantPack("bad reloc size %d", si); // fill counts unsigned pos, type; while (next(pos, type)) @@ -327,7 +325,7 @@ PeFile::Reloc::Reloc(byte *s, unsigned si) : start(s), start_size_in_bytes(si) { } PeFile::Reloc::Reloc(unsigned relocnum) { - start_size_in_bytes = mem_size(4, relocnum, RELOC_BUF_OFFSET, 8192); + start_size_in_bytes = mem_size(4, relocnum, RELOC_INPLACE_OFFSET, 8192); start = new byte[start_size_in_bytes]; // => transfer to oxrelocs[] in finish() start_did_alloc = true; initSpans(); @@ -341,13 +339,22 @@ void PeFile::Reloc::initSpans() { rel1 = nullptr; } -void PeFile::Reloc::advanceBaseRelocPos(void *p) { +void PeFile::Reloc::setBaseRelocPos(void *p, bool check_size_of_block) { // set rel and rel1 unsigned off = ptr_udiff_bytes(p, start); + assert((off & 3) == 0); + rel = (BaseReloc *) p; if (!start_did_alloc && off == start_size_in_bytes) { // final entry - rel = (BaseReloc *) p; rel1 = nullptr; } else { - rel = (BaseReloc *) p; + if (off + 10 > start_size_in_bytes) + throwCantPack("bad relocs"); + if (check_size_of_block && !opt->force) { + const unsigned s = rel->size_of_block; + if (s < sizeof(BaseReloc) + sizeof(LE16)) // == 10 + throwCantPack("bad reloc size_of_block %u (try --force)", s); + if ((s & 3) != 0) + throwCantPack("unaligned reloc size_of_block %u (try --force)", s); + } rel1 = (LE16 *) ((byte *) p + sizeof(BaseReloc)); } } @@ -357,7 +364,7 @@ bool PeFile::Reloc::next(unsigned &result_pos, unsigned &result_type) { unsigned pos, type; do { if (rel == nullptr) - advanceBaseRelocPos(start); + setBaseRelocPos(start, true); if (ptr_udiff_bytes(rel, start) >= start_size_in_bytes) { rel = nullptr; // rewind rel1 = nullptr; // rewind @@ -367,7 +374,7 @@ bool PeFile::Reloc::next(unsigned &result_pos, unsigned &result_type) { type = *rel1++ >> 12; NO_printf("%x %d\n", pos, type); if (ptr_udiff_bytes(rel1, rel) >= rel->size_of_block) - advanceBaseRelocPos(raw_bytes(rel1, 0)); + setBaseRelocPos(raw_bytes(rel1, 0), true); } while (type == 0); result_pos = pos; result_type = type; @@ -376,53 +383,66 @@ bool PeFile::Reloc::next(unsigned &result_pos, unsigned &result_type) { void PeFile::Reloc::add(unsigned pos, unsigned type) { assert(start_did_alloc); - set_le32(start_buf + (RELOC_BUF_OFFSET + 4 * counts[0]), (pos << 4) + type); + set_le32(start_buf + (RELOC_INPLACE_OFFSET + 4 * counts[0]), (pos << 4) + type); counts[0] += 1; } void PeFile::Reloc::finish(byte *(&result_ptr), unsigned &result_size) { assert(start_did_alloc); - // sentinel to force final advanceBaseRelocPos() - set_le32(start_buf + (RELOC_BUF_OFFSET + 4 * counts[0]), 0xfff00000); - counts[0] += 1; - upx_qsort(raw_index_bytes(start_buf, RELOC_BUF_OFFSET, 4 * counts[0]), counts[0], 4, + upx_qsort(raw_index_bytes(start_buf, RELOC_INPLACE_OFFSET, 4 * counts[0]), counts[0], 4, le32_compare); + auto align_size_of_block = [](SPAN_S(BaseReloc) xrel) { + assert(xrel->size_of_block >= 10); + auto end = SPAN_TYPE_CAST(byte, xrel) + xrel->size_of_block; + while ((xrel->size_of_block & 3) != 0) { + *end++ = 0; // clear byte + xrel->size_of_block += 1; + } + }; + rel = nullptr; rel1 = nullptr; - unsigned prev = 0xffffffff; + unsigned prev = 0; for (unsigned ic = 0; ic < counts[0]; ic++) { - const unsigned pos = get_le32(start_buf + (RELOC_BUF_OFFSET + 4 * ic)); - if (rel == (BaseReloc *) (void *) start) - if (ptr_udiff_bytes(rel1, rel) > RELOC_BUF_OFFSET - sizeof(*rel1)) - throwCantPack("too many relocs"); - if (ic == 0) { + const auto pos_ptr = start_buf + (RELOC_INPLACE_OFFSET + 4 * ic); + if (rel1 != nullptr && ptr_diff_bytes(rel1, pos_ptr) >= 0) { + // info: if this is indeed a valid file we must increase RELOC_INPLACE_OFFSET + throwCantPack("too many relocs"); + } + const unsigned pos = get_le32(pos_ptr); + if (ic > 0 && get_le32(pos_ptr - 4) == pos) // XXX: should we check for duplicates? + if (!opt->force) + throwCantPack("duplicate relocs (try --force)"); + if (ic == 0 || (pos ^ prev) >= 0x10000) { prev = pos; - advanceBaseRelocPos(start); + if (ic == 0) + setBaseRelocPos(start, false); + else { + align_size_of_block(rel); + setBaseRelocPos((byte *) raw_bytes(rel, rel->size_of_block) + rel->size_of_block, + false); + } rel->pagestart = (pos >> 4) & ~0xfff; - rel->size_of_block = unsigned(-1); // to be filled later - } else if ((pos ^ prev) >= 0x10000) { - prev = pos; - *rel1 = 0; // clear align-up memory - rel->size_of_block = ALIGN_UP(ptr_udiff_bytes(rel1, rel), 4u); // <= FILL - advanceBaseRelocPos((char *) raw_bytes(rel, rel->size_of_block) + rel->size_of_block); - rel->pagestart = (pos >> 4) & ~0xfff; - rel->size_of_block = unsigned(-1); // to be filled later + rel->size_of_block = 8; } *rel1++ = (pos << 12) + ((pos >> 4) & 0xfff); + rel->size_of_block += 2; + } + result_size = 0; // result_size can be 0 in 64-bit mode + if (counts[0] != 0) { + align_size_of_block(rel); + result_size = ptr_udiff_bytes(rel, start) + rel->size_of_block; } - assert(ptr_udiff_bytes(rel1, rel) == 10); // sentinel - result_size = ptr_udiff_bytes(rel, start); assert((result_size & 3) == 0); - // assert(result_size > 0); // result_size can be 0 in 64-bit mode // transfer ownership assert(start_did_alloc); result_ptr = start; - start = nullptr; start_did_alloc = false; - SPAN_INVALIDATE(start_buf); // safety - SPAN_INVALIDATE(rel); // safety - SPAN_INVALIDATE(rel1); // safety + ptr_invalidate_and_poison(start); // safety + SPAN_INVALIDATE(start_buf); // safety + SPAN_INVALIDATE(rel); // safety + SPAN_INVALIDATE(rel1); // safety } void PeFile::processRelocs(Reloc *rel) // pass2 diff --git a/src/pefile.h b/src/pefile.h index 6d625c66..af727c95 100644 --- a/src/pefile.h +++ b/src/pefile.h @@ -396,7 +396,7 @@ protected: }; SPAN_0(BaseReloc) rel = nullptr; SPAN_0(LE16) rel1 = nullptr; - void advanceBaseRelocPos(void *p); + void setBaseRelocPos(void *p, bool check_size_of_block); // set rel and rel1 unsigned counts[16] = {}; diff --git a/src/stub/Makefile b/src/stub/Makefile index 8a6a2645..a04bc979 100644 --- a/src/stub/Makefile +++ b/src/stub/Makefile @@ -4,7 +4,7 @@ # # NOTE: see misc/podman/rebuild-stubs/20-image-run-shell.sh -# how to rebuild the UPX stubs +# how to rebuild the UPX stubs from source code MAKEFLAGS += -rR .NOTPARALLEL: diff --git a/src/stub/scripts/bin2h.py b/src/stub/scripts/bin2h.py index 14b09385..55bf6b31 100644 --- a/src/stub/scripts/bin2h.py +++ b/src/stub/scripts/bin2h.py @@ -349,7 +349,7 @@ def main(argv): if opts.mode == "c": if opts.verbose >= 0: w_header_c(w, ifile, ofile, len(idata)) - w("/* clang" + "-format off */\n\n") + w("/* clang" + "-format" + " off */\n\n") for i in range(len(mdata)): write_stub(w, mdata_odata[mdata[i]], i, mdata) if ofp: diff --git a/src/util/util.h b/src/util/util.h index 6a6259ba..027d7654 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -156,8 +156,9 @@ void upx_std_stable_sort(void *array, size_t n, upx_compare_func_t compare); // #define UPX_CONFIG_USE_STABLE_SORT 1 #if UPX_CONFIG_USE_STABLE_SORT -// use std::stable_sort(); requires that "element_size" is constexpr! -#define upx_qsort(a, n, element_size, compare) upx_std_stable_sort<(element_size)>(a, n, compare) +// use std::stable_sort(); NOTE: requires that "element_size" is constexpr! +#define upx_qsort(a, n, element_size, compare) \ + upx_std_stable_sort<(element_size)>((a), (n), (compare)) #else // use libc qsort(); good enough for our use cases #define upx_qsort qsort diff --git a/src/util/xspan_fwd.h b/src/util/xspan_fwd.h index 2b8c238f..81ab0863 100644 --- a/src/util/xspan_fwd.h +++ b/src/util/xspan_fwd.h @@ -54,38 +54,6 @@ template inline typename std::enable_if::value, void *>::type operator+(U, const C &) XSPAN_DELETED_FUNCTION; -#if 0 // already handled by member functions -XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C &a, const U *b) { return a.raw_bytes(0) == b; } -XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C &a, const C &b) { - return a.raw_bytes(0) == b.raw_bytes(0); -} -#ifdef D -XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C &a, const D &b) { - return a.raw_bytes(0) == b.raw_bytes(0); -} -#endif -#ifdef E -XSPAN_FWD_TU_CONVERTIBLE(bool) operator==(const C &a, const E &b) { - return a.raw_bytes(0) == b.raw_bytes(0); -} -#endif - -XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C &a, const U *b) { return a.raw_bytes(0) != b; } -XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C &a, const C &b) { - return a.raw_bytes(0) != b.raw_bytes(0); -} -#ifdef D -XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C &a, const D &b) { - return a.raw_bytes(0) != b.raw_bytes(0); -} -#endif -#ifdef E -XSPAN_FWD_TU_CONVERTIBLE(bool) operator!=(const C &a, const E &b) { - return a.raw_bytes(0) != b.raw_bytes(0); -} -#endif -#endif // if 0 // already handled by member functions - #endif // XSPAN_FWD_C_IS_MEMBUFFER /************************************************************************* diff --git a/src/version.h b/src/version.h index 96529af0..7e3d8251 100644 --- a/src/version.h +++ b/src/version.h @@ -2,6 +2,6 @@ #define UPX_VERSION_HEX 0x040200 /* 04.02.00 */ #define UPX_VERSION_STRING "4.2.0" #define UPX_VERSION_STRING4 "4.20" -#define UPX_VERSION_DATE "Oct 5th 2023" -#define UPX_VERSION_DATE_ISO "2023-10-05" +#define UPX_VERSION_DATE "Oct 22nd 2023" +#define UPX_VERSION_DATE_ISO "2023-10-22" #define UPX_VERSION_YEAR "2023" diff --git a/src/work.cpp b/src/work.cpp index a5cfda7f..05092024 100644 --- a/src/work.cpp +++ b/src/work.cpp @@ -108,9 +108,11 @@ static void copy_file_attributes(const struct stat *st, const char *oname, bool struct timespec times[2]; memset(×[0], 0, sizeof(times)); #if HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC + // macOS times[0] = st->st_atimespec; times[1] = st->st_mtimespec; #else + // POSIX.1-2008 times[0] = st->st_atim; times[1] = st->st_mtim; #endif @@ -118,7 +120,7 @@ static void copy_file_attributes(const struct stat *st, const char *oname, bool IGNORE_ERROR(r); #elif USE_UTIME struct utimbuf u; - memset(&u, 0, sizeof(u)); + mem_clear(&u); u.actime = st->st_atime; u.modtime = st->st_mtime; int r = utime(oname, &u); @@ -235,7 +237,7 @@ void do_one_file(const char *const iname, char *const oname) may_throw { if (opt->output_name) { strcpy(tname, opt->output_name); if ((opt->force_overwrite || opt->force >= 2) && !preserve_link) - (void) FileBase::unlink_noexcept(tname); + (void) FileBase::unlink_noexcept(tname); // IGNORE_ERROR } else { if (st.st_nlink < 2) preserve_link = false; // not needed @@ -253,8 +255,8 @@ void do_one_file(const char *const iname, char *const oname) may_throw { preserve_link = ost.st_nlink >= 2; } else if (r == 0 && S_ISLNK(ost.st_mode)) { // output_name is a symlink (valid or dangling) - (void) FileBase::unlink_noexcept(tname); - preserve_link = false; // not needed + (void) FileBase::unlink_noexcept(tname); // IGNORE_ERROR + preserve_link = false; // not needed } else { preserve_link = false; // not needed } @@ -361,8 +363,8 @@ void do_one_file(const char *const iname, char *const oname) may_throw { static void unlink_ofile(char *oname) noexcept { if (oname && oname[0]) { - (void) FileBase::unlink_noexcept(oname); - oname[0] = 0; // done with oname + (void) FileBase::unlink_noexcept(oname); // IGNORE_ERROR + oname[0] = 0; // done with oname } }