From db60b71c524c040a55cc7700ec3c66c8a6f4abb5 Mon Sep 17 00:00:00 2001 From: Roee Toledano Date: Fri, 22 Nov 2024 04:25:02 +0200 Subject: [PATCH] Fix memory leaks in PE format (#4727) * Fix freeing of relocation array when no relocations are present * Reimplement canary checking for PE for speed + getting rid of memory leak when calling imports() --- librz/bin/format/pe/pe.h | 2 ++ librz/bin/format/pe/pe_info.c | 4 ++++ librz/bin/format/pe/pe_relocs.c | 2 +- librz/bin/p/bin_pe.inc | 35 +++++++++++++-------------------- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/librz/bin/format/pe/pe.h b/librz/bin/format/pe/pe.h index 3e8830664e2..3b37b47259b 100644 --- a/librz/bin/format/pe/pe.h +++ b/librz/bin/format/pe/pe.h @@ -191,6 +191,7 @@ struct PE_(rz_bin_pe_obj_t) { Sdb *kv; RzCMS *cms; RzSpcIndirectDataContent *spcinfo; + bool has_canary; char *authentihash; bool is_authhash_valid; bool is_signed; @@ -244,6 +245,7 @@ int PE_(rz_bin_pe_is_stripped_local_syms)(RzBinPEObj *bin); int PE_(rz_bin_pe_is_stripped_debug)(RzBinPEObj *bin); int PE_(bin_pe_get_claimed_checksum)(RzBinPEObj *bin); int PE_(bin_pe_get_actual_checksum)(RzBinPEObj *bin); +bool PE_(rz_bin_pe_has_canary)(const RzBinPEObj *bin); struct rz_bin_pe_addr_t *PE_(check_unknow)(RzBinPEObj *bin); struct rz_bin_pe_addr_t *PE_(check_msvcseh)(RzBinPEObj *bin); struct rz_bin_pe_addr_t *PE_(check_mingw)(RzBinPEObj *bin); diff --git a/librz/bin/format/pe/pe_info.c b/librz/bin/format/pe/pe_info.c index a670605ac34..67b3d6178a7 100644 --- a/librz/bin/format/pe/pe_info.c +++ b/librz/bin/format/pe/pe_info.c @@ -21,6 +21,10 @@ static inline int is_arm(RzBinPEObj *bin) { return 0; } +bool PE_(rz_bin_pe_has_canary)(const RzBinPEObj *bin) { + return bin->has_canary; +} + // TODO: make it const! like in elf char *PE_(rz_bin_pe_get_machine)(RzBinPEObj *bin) { char *machine = NULL; diff --git a/librz/bin/format/pe/pe_relocs.c b/librz/bin/format/pe/pe_relocs.c index 7112427b24b..cdc87987dd5 100644 --- a/librz/bin/format/pe/pe_relocs.c +++ b/librz/bin/format/pe/pe_relocs.c @@ -61,7 +61,7 @@ int PE_(bin_pe_init_relocs)(RZ_NONNULL RzBinPEObj *bin) { } if (PE_(rz_bin_pe_is_stripped_relocs)(bin) || !get_relocs_from_data_dir(bin, ret)) { - rz_vector_free(bin->relocs); + rz_vector_free(ret); bin->relocs = NULL; return false; } diff --git a/librz/bin/p/bin_pe.inc b/librz/bin/p/bin_pe.inc index a9a28464ddd..bbde4de6dea 100644 --- a/librz/bin/p/bin_pe.inc +++ b/librz/bin/p/bin_pe.inc @@ -429,14 +429,17 @@ static RzPVector /**/ *imports(RzBinFile *bf) { if (!bf || !bf->o || !bf->o->bin_obj) { return NULL; } + RzBinPEObj *bin = (RzBinPEObj *)bf->o->bin_obj; + bin->has_canary = false; RzPVector *ret = rz_pvector_new((RzListFree)rz_bin_import_free); if (!ret) { return NULL; } - if (!(imports = PE_(rz_bin_pe_get_imports)(bf->o->bin_obj))) { + if (!(imports = PE_(rz_bin_pe_get_imports)(bin))) { return ret; } + bool has_canary = false; for (i = 0; !imports[i].last; i++) { if (!(ptr = RZ_NEW0(RzBinImport))) { break; @@ -451,7 +454,15 @@ static RzPVector /**/ *imports(RzBinFile *bf) { // index for the import. There is no point in exposing it. // ptr->hint = imports[i].hint; rz_pvector_push(ret, ptr); + + // __security_init_cookie is a function imported from msvcrt.dll (libc) that when called + // initiliazes the stack canary. So if the function is imported, we can + // conclude the binary uses stack canary. + if (!has_canary && !strcmp(ptr->name, "__security_init_cookie")) { + has_canary = true; + } } + bin->has_canary = has_canary; free(imports); return ret; } @@ -637,24 +648,6 @@ err: return NULL; } -static int has_canary(RzBinFile *bf) { - void **it; - const RzPVector *imports_vec = imports(bf); - RzBinImport *imp; - if (imports_vec) { - rz_pvector_foreach (imports_vec, it) { - imp = *it; - // __security_init_cookie is a function imported from msvcrt.dll (libc) that when called - // initiliazes the stack canary. So if the function is imported, we can - // assume the binary uses stack canary. - if (!strcmp(imp->name, "__security_init_cookie")) { - return true; - } - } - } - return false; -} - static inline bool haschr(const struct PE_(rz_bin_pe_obj_t) * bin, ut16 dllCharacteristic) { return bin->nt_headers->optional_header.DllCharacteristics & dllCharacteristic; } @@ -703,7 +696,7 @@ static RzBinInfo *info(RzBinFile *bf) { ret->bits = PE_(rz_bin_pe_get_bits)(bf->o->bin_obj); ret->big_endian = PE_(rz_bin_pe_is_big_endian)(bf->o->bin_obj); ret->dbg_info = 0; - ret->has_canary = has_canary(bf); + ret->has_canary = PE_(rz_bin_pe_has_canary)(bf->o->bin_obj); ret->has_nx = haschr(bin, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT); ret->has_pi = haschr(bin, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE); ret->claimed_checksum = rz_str_newf("0x%08x", claimed_checksum); @@ -711,7 +704,7 @@ static RzBinInfo *info(RzBinFile *bf) { ret->pe_overlay = pe_overlay > 0; ret->signature = bin ? bin->is_signed : false; Sdb *db = sdb_ns(bf->sdb, "pe", true); - sdb_bool_set(db, "canary", has_canary(bf)); + sdb_bool_set(db, "canary", ret->has_canary); sdb_bool_set(db, "highva", haschr(bin, IMAGE_DLLCHARACTERISTICS_HIGH_ENTROPY_VA)); sdb_bool_set(db, "aslr", haschr(bin, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE)); sdb_bool_set(db, "forceintegrity", haschr(bin, IMAGE_DLL_CHARACTERISTICS_FORCE_INTEGRITY));