From 6b4862581e651a49fe15af91af6231adef9ff979 Mon Sep 17 00:00:00 2001 From: Rafael Kitover Date: Fri, 29 Jun 2018 08:14:46 -0700 Subject: [PATCH] fix some ELF parsing vulnerabilities #255 Implement the recommendations described in issue #255 by @zzazzdzz: - Check bounds when reading ELF program header sections. - Skip reading ELF section headers if the string table pointer is NULL. - Increase the buffer size for dissassembled instructions in the dissassembly view and pass the buffer size to the disArm() and disThumb() functions so that rudimentary bounds checking can be done. Also add the constants WORK_RAM_SIZE and ROM_SIZE to reduce incidence of magic numbers and make the code a bit cleaner. --- src/gba/GBA.cpp | 28 +++++++++---------- src/gba/GBA.h | 3 ++ src/gba/armdis.cpp | 23 +++++++++++---- src/gba/armdis.h | 4 +-- src/gba/bios.cpp | 2 +- src/gba/elf.cpp | 66 +++++++++++++++++++++++++++----------------- src/sdl/debugger.cpp | 8 +++--- src/wx/viewers.cpp | 6 ++-- 8 files changed, 86 insertions(+), 54 deletions(-) diff --git a/src/gba/GBA.cpp b/src/gba/GBA.cpp index 25a376f8..c8e7e5c3 100644 --- a/src/gba/GBA.cpp +++ b/src/gba/GBA.cpp @@ -76,7 +76,7 @@ static profile_segment* profilSegment = NULL; #endif #ifdef BKPT_SUPPORT -uint8_t freezeWorkRAM[0x40000]; +uint8_t freezeWorkRAM[WORK_RAM_SIZE]; uint8_t freezeInternalRAM[0x8000]; uint8_t freezeVRAM[0x18000]; uint8_t freezePRAM[0x400]; @@ -459,7 +459,7 @@ variable_desc saveGameStruct[] = { { NULL, 0 } }; -static int romSize = 0x2000000; +static int romSize = ROM_SIZE; #ifdef PROFILING void cpuProfil(profile_segment* seg) @@ -596,7 +596,7 @@ unsigned int CPUWriteState(uint8_t* data, unsigned size) utilWriteMem(data, internalRAM, 0x8000); utilWriteMem(data, paletteRAM, 0x400); - utilWriteMem(data, workRAM, 0x40000); + utilWriteMem(data, workRAM, WORK_RAM_SIZE); utilWriteMem(data, vram, 0x20000); utilWriteMem(data, oam, 0x400); utilWriteMem(data, pix, 4 * 240 * 160); @@ -646,7 +646,7 @@ bool CPUReadState(const uint8_t* data, unsigned size) utilReadMem(internalRAM, data, 0x8000); utilReadMem(paletteRAM, data, 0x400); - utilReadMem(workRAM, data, 0x40000); + utilReadMem(workRAM, data, WORK_RAM_SIZE); utilReadMem(vram, data, 0x20000); utilReadMem(oam, data, 0x400); utilReadMem(pix, data, 4 * 240 * 160); @@ -710,7 +710,7 @@ static bool CPUWriteState(gzFile gzFile) utilGzWrite(gzFile, internalRAM, 0x8000); utilGzWrite(gzFile, paletteRAM, 0x400); - utilGzWrite(gzFile, workRAM, 0x40000); + utilGzWrite(gzFile, workRAM, WORK_RAM_SIZE); utilGzWrite(gzFile, vram, 0x20000); utilGzWrite(gzFile, oam, 0x400); utilGzWrite(gzFile, pix, 4 * 241 * 162); @@ -824,7 +824,7 @@ static bool CPUReadState(gzFile gzFile) utilGzRead(gzFile, internalRAM, 0x8000); utilGzRead(gzFile, paletteRAM, 0x400); - utilGzRead(gzFile, workRAM, 0x40000); + utilGzRead(gzFile, workRAM, WORK_RAM_SIZE); utilGzRead(gzFile, vram, 0x20000); utilGzRead(gzFile, oam, 0x400); if (version < SAVE_GAME_VERSION_6) @@ -1467,20 +1467,20 @@ void SetMapMasks() int CPULoadRom(const char* szFile) { - romSize = 0x2000000; + romSize = ROM_SIZE; if (rom != NULL) { CPUCleanUp(); } systemSaveUpdateCounter = SYSTEM_SAVE_NOT_UPDATED; - rom = (uint8_t*)malloc(0x2000000); + rom = (uint8_t*)malloc(romSize); if (rom == NULL) { systemMessage(MSG_OUT_OF_MEMORY, N_("Failed to allocate memory for %s"), "ROM"); return 0; } - workRAM = (uint8_t*)calloc(1, 0x40000); + workRAM = (uint8_t*)calloc(1, WORK_RAM_SIZE); if (workRAM == NULL) { systemMessage(MSG_OUT_OF_MEMORY, N_("Failed to allocate memory for %s"), "WRAM"); @@ -1527,7 +1527,7 @@ int CPULoadRom(const char* szFile) uint16_t* temp = (uint16_t*)(rom + ((romSize + 1) & ~1)); int i; - for (i = (romSize + 1) & ~1; i < 0x2000000; i += 2) { + for (i = (romSize + 1) & ~1; i < romSize; i += 2) { WRITE16LE(temp, (i >> 1) & 0xFFFF); temp++; } @@ -1593,20 +1593,20 @@ int CPULoadRom(const char* szFile) int CPULoadRomData(const char* data, int size) { - romSize = 0x2000000; + romSize = ROM_SIZE; if (rom != NULL) { CPUCleanUp(); } systemSaveUpdateCounter = SYSTEM_SAVE_NOT_UPDATED; - rom = (uint8_t*)malloc(0x2000000); + rom = (uint8_t*)malloc(romSize); if (rom == NULL) { systemMessage(MSG_OUT_OF_MEMORY, N_("Failed to allocate memory for %s"), "ROM"); return 0; } - workRAM = (uint8_t*)calloc(1, 0x40000); + workRAM = (uint8_t*)calloc(1, WORK_RAM_SIZE); if (workRAM == NULL) { systemMessage(MSG_OUT_OF_MEMORY, N_("Failed to allocate memory for %s"), "WRAM"); @@ -1620,7 +1620,7 @@ int CPULoadRomData(const char* data, int size) uint16_t* temp = (uint16_t*)(rom + ((romSize + 1) & ~1)); int i; - for (i = (romSize + 1) & ~1; i < 0x2000000; i += 2) { + for (i = (romSize + 1) & ~1; i < romSize; i += 2) { WRITE16LE(temp, (i >> 1) & 0xFFFF); temp++; } diff --git a/src/gba/GBA.h b/src/gba/GBA.h index c07bff02..9011e2ac 100644 --- a/src/gba/GBA.h +++ b/src/gba/GBA.h @@ -153,6 +153,9 @@ extern struct EmulatedSystem GBASystem; #define R14_FIQ 43 #define SPSR_FIQ 44 +#define WORK_RAM_SIZE 0x40000 +#define ROM_SIZE 0x2000000 + #include "Cheats.h" #include "EEprom.h" #include "Flash.h" diff --git a/src/gba/armdis.cpp b/src/gba/armdis.cpp index 423c34c4..081e00d0 100644 --- a/src/gba/armdis.cpp +++ b/src/gba/armdis.cpp @@ -2,6 +2,7 @@ /* Arm/Thumb command set disassembler */ /************************************************************************/ #include +#include #include "../System.h" #include "../common/Port.h" @@ -214,8 +215,13 @@ char* addHex(char* dest, int siz, uint32_t val) return dest; } -int disArm(uint32_t offset, char* dest, int flags) +int disArm(uint32_t offset, char* dest, unsigned dest_sz, int flags) { + if (dest_sz < 80) { + *dest = '\0'; + return 4; + } + uint32_t opcode = debuggerReadMemory(offset); const Opcodes* sp = armOpcodes; @@ -523,8 +529,15 @@ int disArm(uint32_t offset, char* dest, int flags) return 4; } -int disThumb(uint32_t offset, char* dest, int flags) +int disThumb(uint32_t offset, char* dest, unsigned dest_sz, int flags) { + if (dest_sz < 80) { + *dest = '\0'; + return 2; + } + + char* end = dest + dest_sz; + uint32_t opcode = debuggerReadHalfWord(offset); const Opcodes* sp = thumbOpcodes; @@ -597,7 +610,7 @@ int disThumb(uint32_t offset, char* dest, int flags) *dest++ = '$'; dest = addHex(dest, 32, value); const char* s = elfGetAddressSymbol(value); - if (*s) { + if (*s && (dest + strlen(s) + 1) < end) { *dest++ = ' '; dest = addStr(dest, s); } @@ -607,7 +620,7 @@ int disThumb(uint32_t offset, char* dest, int flags) *dest++ = '$'; dest = addHex(dest, 32, value); const char* s = elfGetAddressSymbol(value); - if (*s) { + if (*s && (dest + strlen(s) + 1) < end) { *dest++ = ' '; dest = addStr(dest, s); } @@ -694,7 +707,7 @@ int disThumb(uint32_t offset, char* dest, int flags) *dest++ = '$'; dest = addHex(dest, 32, offset + 4 + add); const char* s = elfGetAddressSymbol(offset + 4 + add); - if (*s) { + if (*s && (dest + strlen(s) + 3) < end) { *dest++ = ' '; *dest++ = '('; dest = addStr(dest, s); diff --git a/src/gba/armdis.h b/src/gba/armdis.h index fde12f18..64bd51dd 100644 --- a/src/gba/armdis.h +++ b/src/gba/armdis.h @@ -8,7 +8,7 @@ #define DIS_VIEW_ADDRESS 1 #define DIS_VIEW_CODE 2 -int disThumb(uint32_t offset, char* dest, int flags); -int disArm(uint32_t offset, char* dest, int flags); +int disThumb(uint32_t offset, char* dest, unsigned dest_sz, int flags); +int disArm(uint32_t offset, char* dest, unsigned dest_sz, int flags); #endif // __ARMDIS_H__ diff --git a/src/gba/bios.cpp b/src/gba/bios.cpp index 213bf3ba..7d6af842 100644 --- a/src/gba/bios.cpp +++ b/src/gba/bios.cpp @@ -852,7 +852,7 @@ void BIOS_RegisterRamReset(uint32_t flags) if (flags) { if (flags & 0x01) { // clear work RAM - memset(workRAM, 0, 0x40000); + memset(workRAM, 0, WORK_RAM_SIZE); } if (flags & 0x02) { // clear internal RAM diff --git a/src/gba/elf.cpp b/src/gba/elf.cpp index 4f45145a..19c74f85 100644 --- a/src/gba/elf.cpp +++ b/src/gba/elf.cpp @@ -843,6 +843,9 @@ uint8_t* elfReadSection(uint8_t* data, ELFSectionHeader* sh) ELFSectionHeader* elfGetSectionByName(const char* name) { + if (elfSectionHeadersStringTable == NULL) + return NULL; + for (int i = 0; i < elfSectionHeadersCount; i++) { if (strcmp(name, &elfSectionHeadersStringTable[READ32LE(&elfSectionHeaders[i]->name)]) @@ -2538,7 +2541,7 @@ void elfReadSymtab(uint8_t* data) // free(symtab); } -bool elfReadProgram(ELFHeader* eh, uint8_t* data, int& size, bool parseDebug) +bool elfReadProgram(ELFHeader* eh, uint8_t* data, unsigned long data_size, int& size, bool parseDebug) { int count = READ16LE(&eh->e_phnum); int i; @@ -2559,32 +2562,54 @@ bool elfReadProgram(ELFHeader* eh, uint8_t* data, int& size, bool parseDebug) // printf("PH %d %08x %08x %08x %08x %08x %08x %08x %08x\n", // i, ph->type, ph->offset, ph->vaddr, ph->paddr, // ph->filesz, ph->memsz, ph->flags, ph->align); + + unsigned address = READ32LE(&ph->paddr); + unsigned offset = READ32LE(&ph->offset); + unsigned section_size = READ32LE(&ph->filesz); + + if (offset > data_size || section_size > data_size || offset + section_size > data_size) + continue; + + uint8_t* source = data + offset; + if (cpuIsMultiBoot) { - if (READ32LE(&ph->paddr) >= 0x2000000 && READ32LE(&ph->paddr) <= 0x203ffff) { - memcpy(&workRAM[READ32LE(&ph->paddr) & 0x3ffff], - data + READ32LE(&ph->offset), - READ32LE(&ph->filesz)); - size += READ32LE(&ph->filesz); + unsigned effective_address = address - 0x2000000; + + if (effective_address + section_size < WORK_RAM_SIZE) { + memcpy(&workRAM[effective_address], source, section_size); + size += section_size; } } else { - if (READ32LE(&ph->paddr) >= 0x8000000 && READ32LE(&ph->paddr) <= 0x9ffffff) { - memcpy(&rom[READ32LE(&ph->paddr) & 0x1ffffff], - data + READ32LE(&ph->offset), - READ32LE(&ph->filesz)); - size += READ32LE(&ph->filesz); + unsigned effective_address = address - 0x8000000; + + if (effective_address + section_size < ROM_SIZE) { + memcpy(&rom[effective_address], source, section_size); + size += section_size; } } } - char* stringTable = NULL; + // these must be pre-declared or clang barfs on the goto statement + ELFSectionHeader** sh = NULL; + char* stringTable = NULL; + + // read section headers (if string table is good) + if (READ16LE(&eh->e_shstrndx) == 0) + goto end; - // read section headers p = data + READ32LE(&eh->e_shoff); + count = READ16LE(&eh->e_shnum); - ELFSectionHeader** sh = (ELFSectionHeader**) + sh = (ELFSectionHeader**) malloc(sizeof(ELFSectionHeader*) * count); + stringTable = (char*)elfReadSection(data, sh[READ16LE(&eh->e_shstrndx)]); + + elfSectionHeaders = sh; + elfSectionHeadersStringTable = stringTable; + elfSectionHeadersCount = count; + for (i = 0; i < count; i++) { sh[i] = (ELFSectionHeader*)p; p += sizeof(ELFSectionHeader); @@ -2592,15 +2617,6 @@ bool elfReadProgram(ELFHeader* eh, uint8_t* data, int& size, bool parseDebug) p += READ16LE(&eh->e_shentsize) - sizeof(ELFSectionHeader); } - if (READ16LE(&eh->e_shstrndx) != 0) { - stringTable = (char*)elfReadSection(data, - sh[READ16LE(&eh->e_shstrndx)]); - } - - elfSectionHeaders = sh; - elfSectionHeadersStringTable = stringTable; - elfSectionHeadersCount = count; - for (i = 0; i < count; i++) { // printf("SH %d %-20s %08x %08x %08x %08x %08x %08x %08x %08x\n", // i, &stringTable[sh[i]->name], sh[i]->name, sh[i]->type, @@ -2703,7 +2719,7 @@ extern bool parseDebug; bool elfRead(const char* name, int& siz, FILE* f) { fseek(f, 0, SEEK_END); - long size = ftell(f); + unsigned long size = ftell(f); elfFileData = (uint8_t*)malloc(size); fseek(f, 0, SEEK_SET); int res = fread(elfFileData, 1, size, f); @@ -2724,7 +2740,7 @@ bool elfRead(const char* name, int& siz, FILE* f) return false; } - if (!elfReadProgram(header, elfFileData, siz, parseDebug)) { + if (!elfReadProgram(header, elfFileData, size, siz, parseDebug)) { free(elfFileData); elfFileData = NULL; return false; diff --git a/src/sdl/debugger.cpp b/src/sdl/debugger.cpp index 85afb41d..635f76ed 100644 --- a/src/sdl/debugger.cpp +++ b/src/sdl/debugger.cpp @@ -1355,7 +1355,7 @@ static void debuggerBreakChange(int n, char** args) static void debuggerDisassembleArm(FILE* f, uint32_t pc, int count) { - char buffer[80]; + char buffer[4096]; int i = 0; uint32_t len = 0; char format[30]; @@ -1367,14 +1367,14 @@ static void debuggerDisassembleArm(FILE* f, uint32_t pc, int count) sprintf(format, "%%08x %%-%ds %%s\n", len); for (i = 0; i < count; i++) { uint32_t addr = pc; - pc += disArm(pc, buffer, 2); + pc += disArm(pc, buffer, 4096, 2); fprintf(f, format, addr, elfGetAddressSymbol(addr), buffer); } } static void debuggerDisassembleThumb(FILE* f, uint32_t pc, int count) { - char buffer[80]; + char buffer[4096]; int i = 0; uint32_t len = 0; char format[30]; @@ -1387,7 +1387,7 @@ static void debuggerDisassembleThumb(FILE* f, uint32_t pc, int count) for (i = 0; i < count; i++) { uint32_t addr = pc; - pc += disThumb(pc, buffer, 2); + pc += disThumb(pc, buffer, 4096, 2); fprintf(f, format, addr, elfGetAddressSymbol(addr), buffer); } } diff --git a/src/wx/viewers.cpp b/src/wx/viewers.cpp index a47b8ed1..b476439d 100644 --- a/src/wx/viewers.cpp +++ b/src/wx/viewers.cpp @@ -146,7 +146,7 @@ public: // what an unsafe calling convention // examination of disArm shows that max len is 69 chars // (e.g. 0x081cb6db), and I assume disThumb is shorter - char buf[80]; + char buf[4096]; dis->strings.clear(); dis->addrs.clear(); uint32_t addr = dis->topaddr; @@ -157,9 +157,9 @@ public: dis->addrs.push_back(addr); if (arm) - addr += disArm(addr, buf, DIS_VIEW_CODE | DIS_VIEW_ADDRESS); + addr += disArm(addr, buf, 4096, DIS_VIEW_CODE | DIS_VIEW_ADDRESS); else - addr += disThumb(addr, buf, DIS_VIEW_CODE | DIS_VIEW_ADDRESS); + addr += disThumb(addr, buf, 4096, DIS_VIEW_CODE | DIS_VIEW_ADDRESS); dis->strings.push_back(wxString(buf, wxConvLibc)); }